GNU bug report logs - #36602
[PATCH] Add node-build-system.

Previous Next

Package: guix-patches;

Reported by: Julien Lepiller <julien <at> lepiller.eu>

Date: Thu, 11 Jul 2019 16:48:01 UTC

Severity: normal

Tags: patch

Done: Julien Lepiller <julien <at> lepiller.eu>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 36602 in the body.
You can then email your comments to 36602 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to guix-patches <at> gnu.org:
bug#36602; Package guix-patches. (Thu, 11 Jul 2019 16:48:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Julien Lepiller <julien <at> lepiller.eu>:
New bug report received and forwarded. Copy sent to guix-patches <at> gnu.org. (Thu, 11 Jul 2019 16:48:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Julien Lepiller <julien <at> lepiller.eu>
To: guix-patches <at> gnu.org
Subject: [PATCH] Add node-build-system.
Date: Thu, 11 Jul 2019 18:46:53 +0200
[Message part 1 (text/plain, inline)]
Hi Guix!

This patch adds a node-build-system. I wasn't sure if it was ready yet,
but I think, since I didn't change it in the last months, that it might
actually be :)

The patch was initially made by Jelle Licht, and I improved a bit on
it. Note that packages built with this build system will embed symlinks
to their dependencies, but not devDependencies (build-only
dependencies) according to the information in the metadata. Each
package is installed in lib/node_modules/package-name and symlinks are
added to lib/node_modules/package-name/node_modules. This allows us to
use only inputs instead of propagated inputs. Executables are installed
in bin according to metadata, and they should work even if called
directly from their store path.
[0001-build-Add-node-build-system.patch (text/x-patch, attachment)]

Information forwarded to guix-patches <at> gnu.org:
bug#36602; Package guix-patches. (Thu, 11 Jul 2019 18:26:01 GMT) Full text and rfc822 format available.

Message #8 received at 36602 <at> debbugs.gnu.org (full text, mbox):

From: Jelle Licht <jlicht <at> fsfe.org>
To: Julien Lepiller <julien <at> lepiller.eu>, 36602 <at> debbugs.gnu.org
Subject: Re: [bug#36602] [PATCH] Add node-build-system.
Date: Thu, 11 Jul 2019 20:25:06 +0200
Hello Julien,

Julien Lepiller <julien <at> lepiller.eu> writes:

> Hi Guix!
>
> This patch adds a node-build-system. I wasn't sure if it was ready yet,
> but I think, since I didn't change it in the last months, that it might
> actually be :)
>
> The patch was initially made by Jelle Licht, and I improved a bit on
> it. Note that packages built with this build system will embed symlinks
> to their dependencies, but not devDependencies (build-only
> dependencies) according to the information in the metadata. Each
> package is installed in lib/node_modules/package-name and symlinks are
> added to lib/node_modules/package-name/node_modules. This allows us to
> use only inputs instead of propagated inputs. Executables are installed
> in bin according to metadata, and they should work even if called
> directly from their store path.

I am probably a bit of a hypocrite for the following nitpicks, as I am
quite sure I was the one that introduced pretty much all of them them,
so I offer my apologies in advance :-).

> From 38158940be0ef4780cdbb553cfa039d21fcdda9b Mon Sep 17 00:00:00 2001
> From: Jelle Licht <jlicht <at> fsfe.org>
> Date: Tue, 23 Aug 2016 05:23:55 +0200
> Subject: [PATCH] build: Add node-build-system.
>
> * guix/build/node-build-system.scm: New file.
> * guix/build-system/node.scm: New file.
> * guix/build/json.scm: New file.
> * doc/guix.texi: Document it.
> * Makefile.am: Added new files.
>
> Co-Authored-By: Julien Lepiller <julien <at> lepiller.eu>
> ---
>  Makefile.am                      |   2 +
>  doc/guix.texi                    |  11 +
>  guix/build-system/node.scm       | 139 +++++++++++
>  guix/build/json.scm              | 387 +++++++++++++++++++++++++++++++
>  guix/build/node-build-system.scm | 159 +++++++++++++
>  5 files changed, 698 insertions(+)
>  create mode 100644 guix/build-system/node.scm
>  create mode 100644 guix/build/json.scm
>  create mode 100644 guix/build/node-build-system.scm
>
> diff --git a/Makefile.am b/Makefile.am
> index 82eda6042a..38f2d7e690 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -125,6 +125,7 @@ MODULES =					\
>    guix/build-system/guile.scm			\
>    guix/build-system/haskell.scm			\
>    guix/build-system/linux-module.scm		\
> +  guix/build-system/node.scm			\
>    guix/build-system/perl.scm			\
>    guix/build-system/python.scm			\
>    guix/build-system/ocaml.scm			\
> @@ -170,6 +171,7 @@ MODULES =					\
>    guix/build/gnu-build-system.scm		\
>    guix/build/gnu-dist.scm			\
>    guix/build/guile-build-system.scm		\
> +  guix/build/node-build-system.scm		\

We are missing the `json.scm' file in this listing.

> [snip]

> +(define* (node-build store name inputs
> +                     #:key
> +                     (npm-flags ''())
> +                     (global? #f)
I am not quite sure if this is needed. Put another way: would we not
want all package builds to have `global? #t' in Guix?

> +                     (test-target "test")
This one is no longer in use. 

> +                     (tests? #f)
I know that for most modules we will not even be able to run tests, but
it seems silly to disable them by default, as that would hide the issue.

> [snip]

> +(define* (install #:key outputs inputs global? #:allow-other-keys)
> +  "Install the node module to the output store item. MODULENAME defines
> +under which name the module will be installed, GLOBAL? determines whether this
> +is an npm global install."
> +  (let* ((out          (assoc-ref outputs "out"))
> +         (src-dir      (getcwd))
> +         (tgt-dir      (string-append out "/lib"))
> +         (bin-dir      (string-append out "/bin"))
> +         (modulename   (string-append  (assoc-ref (read-package-data) "name")))
> +         (data         (read-package-data))
> +         (bin-conf     (assoc-ref data "bin"))
> +         (dependencies (match (assoc-ref data "dependencies")
> +                         ((@ deps ...) deps)
> +                         (#f #f))))
It might be better to write out most of these
names. I think we could also move `modulename' one line lower, so it can
become `(modulename (assoc-ref data "name"))'.

If you want me to tidy up these things, let me know; I can do it first
thing after the weekend.

Thanks

Jelle




Information forwarded to guix-patches <at> gnu.org:
bug#36602; Package guix-patches. (Sat, 13 Jul 2019 12:43:02 GMT) Full text and rfc822 format available.

Message #11 received at 36602 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Jelle Licht <jlicht <at> fsfe.org>
Cc: Julien Lepiller <julien <at> lepiller.eu>, 36602 <at> debbugs.gnu.org
Subject: Re: [bug#36602] [PATCH] Add node-build-system.
Date: Sat, 13 Jul 2019 14:42:38 +0200
Hello!

Nice work!  I wonder if this could be used for the ‘node-semver’ package
that arrived at about the same time: <https://issues.guix.gnu.org/issue/36599>.

Jelle Licht <jlicht <at> fsfe.org> skribis:

>> +(define* (install #:key outputs inputs global? #:allow-other-keys)
>> +  "Install the node module to the output store item. MODULENAME defines
>> +under which name the module will be installed, GLOBAL? determines whether this
>> +is an npm global install."
>> +  (let* ((out          (assoc-ref outputs "out"))
>> +         (src-dir      (getcwd))
>> +         (tgt-dir      (string-append out "/lib"))
>> +         (bin-dir      (string-append out "/bin"))
>> +         (modulename   (string-append  (assoc-ref (read-package-data) "name")))
>> +         (data         (read-package-data))
>> +         (bin-conf     (assoc-ref data "bin"))
>> +         (dependencies (match (assoc-ref data "dependencies")
>> +                         ((@ deps ...) deps)

Note that ‘@’ here matches anything.  Did you mean '@, which would match
the @ symbol?

>> +                         (#f #f))))
> It might be better to write out most of these
> names. I think we could also move `modulename' one line lower, so it can
> become `(modulename (assoc-ref data "name"))'.

I’m gratuitously nitpicking as well :-), but I think we should spell out
names in general, so I’d propose s/src-dir/source/, s/tgt-dir/target/,
etc.

  https://www.gnu.org/software/guix/manual/en/html_node/Formatting-Code.html

Anyway, thanks for working on it!

Ludo’.




Information forwarded to guix-patches <at> gnu.org:
bug#36602; Package guix-patches. (Sat, 13 Jul 2019 15:34:02 GMT) Full text and rfc822 format available.

Message #14 received at 36602 <at> debbugs.gnu.org (full text, mbox):

From: goodoldpaul <at> autistici.org
To: 36602 <at> debbugs.gnu.org
Subject: Re: [PATCH] Add node-build-system.
Date: Sat, 13 Jul 2019 15:32:55 +0000
Hello!

I'm the author of: https://issues.guix.gnu.org/issue/36599 . It should 
be really easy to rewrite the package using this new build system.

I submitted the patch before asking in #guix if someone was already 
working on node's ecosystem, but when I did Julien pointed me to his 
development branch which contains a definition for node-semver using the 
new node-build system: 
https://framagit.org/tyreunom/guix/commit/5c4708bfae0955999bca1edfd459aeb39980ab0e 
.

If this is merged I can refactor my patch (which contains node-semver 
6.2.0), based on Julien's package definition (which contains version 
5.6.0), in no time!

Very cool work by the way,

Giacomo




Information forwarded to guix-patches <at> gnu.org:
bug#36602; Package guix-patches. (Sat, 13 Jul 2019 20:21:02 GMT) Full text and rfc822 format available.

Message #17 received at 36602 <at> debbugs.gnu.org (full text, mbox):

From: Julien Lepiller <julien <at> lepiller.eu>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Jelle Licht <jlicht <at> fsfe.org>, 36602 <at> debbugs.gnu.org
Subject: Re: [bug#36602] [PATCH] Add node-build-system.
Date: Sat, 13 Jul 2019 22:20:14 +0200
[Message part 1 (text/plain, inline)]
Le Sat, 13 Jul 2019 14:42:38 +0200,
Ludovic Courtès <ludo <at> gnu.org> a écrit :

> Hello!
> 
> Nice work!  I wonder if this could be used for the ‘node-semver’
> package that arrived at about the same time:
> <https://issues.guix.gnu.org/issue/36599>.
> 
> Jelle Licht <jlicht <at> fsfe.org> skribis:
> 
> >> +(define* (install #:key outputs inputs global? #:allow-other-keys)
> >> +  "Install the node module to the output store item. MODULENAME
> >> defines +under which name the module will be installed, GLOBAL?
> >> determines whether this +is an npm global install."
> >> +  (let* ((out          (assoc-ref outputs "out"))
> >> +         (src-dir      (getcwd))
> >> +         (tgt-dir      (string-append out "/lib"))
> >> +         (bin-dir      (string-append out "/bin"))
> >> +         (modulename   (string-append  (assoc-ref
> >> (read-package-data) "name")))
> >> +         (data         (read-package-data))
> >> +         (bin-conf     (assoc-ref data "bin"))
> >> +         (dependencies (match (assoc-ref data "dependencies")
> >> +                         ((@ deps ...) deps)  
> 
> Note that ‘@’ here matches anything.  Did you mean '@, which would
> match the @ symbol?
> 
> >> +                         (#f #f))))  
> > It might be better to write out most of these
> > names. I think we could also move `modulename' one line lower, so
> > it can become `(modulename (assoc-ref data "name"))'.  
> 
> I’m gratuitously nitpicking as well :-), but I think we should spell
> out names in general, so I’d propose s/src-dir/source/,
> s/tgt-dir/target/, etc.
> 
>   https://www.gnu.org/software/guix/manual/en/html_node/Formatting-Code.html
> 
> Anyway, thanks for working on it!
> 
> Ludo’.

Hopefully, the attached patch fixes your (and Jelle's) points. I also
fixed the default node package (it was node-lts, but that doesn't exist
anymore), made the configure phase return #t and fixed the
binary-configuration. I tested again on a few packages and they built
without issue.
[0001-build-Add-node-build-system.patch (text/x-patch, attachment)]

Information forwarded to guix-patches <at> gnu.org:
bug#36602; Package guix-patches. (Sat, 13 Jul 2019 21:31:02 GMT) Full text and rfc822 format available.

Message #20 received at 36602 <at> debbugs.gnu.org (full text, mbox):

From: Jelle Licht <jlicht <at> fsfe.org>
To: Julien Lepiller <julien <at> lepiller.eu>, Ludovic Courtès
 <ludo <at> gnu.org>
Cc: 36602 <at> debbugs.gnu.org
Subject: Re: [bug#36602] [PATCH] Add node-build-system.
Date: Sat, 13 Jul 2019 23:30:12 +0200
Julien Lepiller <julien <at> lepiller.eu> writes:

> [...]
> +(define* (install #:key outputs inputs #:allow-other-keys)
> +  "Install the node module to the output store item. MODULENAME defines
> +under which name the module will be installed, GLOBAL? determines whether this
> +is an npm global install."

`global?' is no longer , so it could probably be removed from the
docstring as well. LGTM otherwise :-).




Reply sent to Julien Lepiller <julien <at> lepiller.eu>:
You have taken responsibility. (Sun, 14 Jul 2019 10:36:02 GMT) Full text and rfc822 format available.

Notification sent to Julien Lepiller <julien <at> lepiller.eu>:
bug acknowledged by developer. (Sun, 14 Jul 2019 10:36:02 GMT) Full text and rfc822 format available.

Message #25 received at 36602-done <at> debbugs.gnu.org (full text, mbox):

From: Julien Lepiller <julien <at> lepiller.eu>
To: Jelle Licht <jlicht <at> fsfe.org>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 36602-done <at> debbugs.gnu.org
Subject: Re: [bug#36602] [PATCH] Add node-build-system.
Date: Sun, 14 Jul 2019 12:34:53 +0200
Le Sat, 13 Jul 2019 23:30:12 +0200,
Jelle Licht <jlicht <at> fsfe.org> a écrit :

> Julien Lepiller <julien <at> lepiller.eu> writes:
> 
> > [...]
> > +(define* (install #:key outputs inputs #:allow-other-keys)
> > +  "Install the node module to the output store item. MODULENAME
> > defines +under which name the module will be installed, GLOBAL?
> > determines whether this +is an npm global install."  
> 
> `global?' is no longer , so it could probably be removed from the
> docstring as well. LGTM otherwise :-).

I've reworked that docstring and pushed as
09a1f92f61d1ab11d2cf9f7a0983f4fc9f436f57, thank you!




Information forwarded to guix-patches <at> gnu.org:
bug#36602; Package guix-patches. (Sun, 14 Jul 2019 11:10:02 GMT) Full text and rfc822 format available.

Message #28 received at 36602 <at> debbugs.gnu.org (full text, mbox):

From: Robert Vollmert <rob <at> vllmrt.net>
To: 36602 <at> debbugs.gnu.org
Subject: guix/build/json.scm
Date: Sun, 14 Jul 2019 13:09:18 +0200
Hi,

just had a look at 09a1f92f61d1ab11d2cf9f7a0983f4fc9f436f57, some questions:

- the link to https://github.com/cwebber/activitystuff/blob/master/activitystuff/contrib/json.scm
  is a 404, and I can’t find (ice-9 json) elsewhere (also, shouldn’t cwebber be listed in the
  copyright line?)
- how does this embedded json parsing module differ from (json parser) as used by the cargo
  build system? wouldn’t it be better to have both use the same json modules?

Cheers
Robert





Information forwarded to guix-patches <at> gnu.org:
bug#36602; Package guix-patches. (Sun, 14 Jul 2019 12:47:02 GMT) Full text and rfc822 format available.

Message #31 received at submit <at> debbugs.gnu.org (full text, mbox):

From: swedebugia <swedebugia <at> riseup.net>
To: guix-patches <at> gnu.org
Subject: Re: bug#36602: [PATCH] Add node-build-system.
Date: Sun, 14 Jul 2019 14:46:47 +0200
On 2019-07-14 12:34, Julien Lepiller wrote:

> I've reworked that docstring and pushed as
> 09a1f92f61d1ab11d2cf9f7a0983f4fc9f436f57, thank you!

Wohoo! First small step to working node applications.

-- 
Cheers
Swedebugia




Information forwarded to guix-patches <at> gnu.org:
bug#36602; Package guix-patches. (Sun, 14 Jul 2019 13:00:02 GMT) Full text and rfc822 format available.

Message #34 received at 36602 <at> debbugs.gnu.org (full text, mbox):

From: Julien Lepiller <julien <at> lepiller.eu>
To: Robert Vollmert <rob <at> vllmrt.net>
Cc: 36602 <at> debbugs.gnu.org
Subject: Re: [bug#36602] guix/build/json.scm
Date: Sun, 14 Jul 2019 14:59:45 +0200
Le Sun, 14 Jul 2019 13:09:18 +0200,
Robert Vollmert <rob <at> vllmrt.net> a écrit :

> Hi,
> 
> just had a look at 09a1f92f61d1ab11d2cf9f7a0983f4fc9f436f57, some
> questions:
> 
> - the link to
> https://github.com/cwebber/activitystuff/blob/master/activitystuff/contrib/json.scm
> is a 404, and I can’t find (ice-9 json) elsewhere (also, shouldn’t
> cwebber be listed in the copyright line?)
> - how does this embedded json parsing module differ from (json
> parser) as used by the cargo build system? wouldn’t it be better to
> have both use the same json modules?
> 
> Cheers
> Robert
> 

You're right, I thought we didn't use guile-json, but since we already
use it in the cargo build system, I removed (guix build json) and fixed
the node-build-system to use (json parser) instead. I tested a few
packages, so it should be working. I pushed the changes in
8eb0ba532ebbebef23180e666e0607ea735f9c1a.




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Mon, 12 Aug 2019 11:24:03 GMT) Full text and rfc822 format available.

This bug report was last modified 4 years and 230 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.