[bug#51838,03/11] guix: node-build-system: Support compiling add-ons with node-gyp.

* gnu/packages/node.scm (node)[arguments]: Replace 'patch-npm-shebang
and 'patch-node-shebang with a new 'patch-nested-shebangs that also
handles node-gyp and other shebangs under "/lib/node_modules".
[inputs]: Add Python for node-gyp as "python-for-target".
(node-lts)[inputs]: Likewise.
(libnode)[arguments]: Adjust to delete 'patch-nested-shebangs rather
than 'patch-npm-shebang and 'patch-node-shebang.
* guix/build-system/node.scm (lower): Add optional #:python argument
and corresponding implicit input. Add the version of libuv used
as an input to the #:node package as a new implicit input.
* guix/build/node-build-system.scm (set-node-gyp-paths): New
function. Sets the "npm_config_nodedir" and "npm_config_python"
environment variables. Adds the "node-gyp-bin" directory to "PATH".
(configure-gyp): New function. Run `node-gyp configure` if we see
a `binding.gyp` file.
(%standard-phases): Add 'set-node-gyp-paths after 'set-paths.
Add 'configure-gyp after 'configure.

Co-authored-by: Pierre Langlois <pierre.langlois@gmx.com>
---
 gnu/packages/node.scm            | 34 ++++++++++++++++----------------
 guix/build-system/node.scm       | 11 +++++++++--
 guix/build/node-build-system.scm | 23 +++++++++++++++++++++
 3 files changed, 49 insertions(+), 19 deletions(-)

Hi Philip

Am Sonntag, den 14.11.2021, 08:04 -0500 schrieb Philip McGrath:
> * gnu/packages/node.scm (node)[arguments]: Replace 'patch-npm-shebang
> and 'patch-node-shebang with a new 'patch-nested-shebangs that also
> handles node-gyp and other shebangs under "/lib/node_modules".
> [inputs]: Add Python for node-gyp as "python-for-target".
> (node-lts)[inputs]: Likewise.
> (libnode)[arguments]: Adjust to delete 'patch-nested-shebangs rather
> than 'patch-npm-shebang and 'patch-node-shebang.
> * guix/build-system/node.scm (lower): Add optional #:python argument
> and corresponding implicit input. Add the version of libuv used
> as an input to the #:node package as a new implicit input.
> * guix/build/node-build-system.scm (set-node-gyp-paths): New
> function. Sets the "npm_config_nodedir" and "npm_config_python"
> environment variables. Adds the "node-gyp-bin" directory to "PATH".
> (configure-gyp): New function. Run `node-gyp configure` if we see
> a `binding.gyp` file.
> (%standard-phases): Add 'set-node-gyp-paths after 'set-paths.
> Add 'configure-gyp after 'configure.
> 
> Co-authored-by: Pierre Langlois <pierre.langlois@gmx.com>
Looking at this patch, it does *a lot* at once and could probably be
separated into more than one.  Particularly, I'd suggest providing
capabilities in node-build-system first, then switching over to the new
thing in node.

> [...]
> --- a/guix/build-system/node.scm
> +++ b/guix/build-system/node.scm
> @@ -1,6 +1,8 @@
>  ;;; GNU Guix --- Functional package management for GNU
>  ;;; Copyright © 2016 Jelle Licht <jlicht@fsfe.org>
>  ;;; Copyright © 2019 Timothy Sample <samplet@ngyro.com>
> +;;; Copyright © 2021 Pierre Langlois <pierre.langlois@gmx.com>
> +;;; Copyright © 2021 Philip McGrath <philip@philipmcgrath.com>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -24,6 +26,7 @@ (define-module (guix build-system node)
>    #:use-module (guix search-paths)
>    #:use-module (guix build-system)
>    #:use-module (guix build-system gnu)
> +  #:use-module (guix build-system python)
>    #:use-module (ice-9 match)
>    #:export (%node-build-system-modules
>              node-build
> @@ -44,11 +47,12 @@ (define (default-node)
>  (define* (lower name
>                  #:key source inputs native-inputs outputs system
> target
>                  (node (default-node))
> +                (python (default-python)) ;; for node-gyp
>                  #:allow-other-keys
>                  #:rest arguments)
>    "Return a bag for NAME."
>    (define private-keywords
> -    '(#:source #:target #:node #:inputs #:native-inputs))
> +    '(#:source #:target #:node #:python #:inputs #:native-inputs))
>  
>    (and (not target)                    ;XXX: no cross-compilation
>         (bag
> @@ -58,10 +62,13 @@ (define private-keywords
>                                `(("source" ,source))
>                                '())
>                          ,@inputs
> -
>                          ;; Keep the standard inputs of 'gnu-build-
> system'.
>                          ,@(standard-packages)))
>           (build-inputs `(("node" ,node)
> +                         ("python" ,python)
> +                        ;; We don't always need libuv, but the libuv
> and
> +                        ;; node versions need to match:
> +                        ("libuv" ,@(assoc-ref (package-inputs node)
> "libuv"))
>                           ,@native-inputs))
>           (outputs outputs)
>           (build node-build)
Will this python input always or often enough be needed?  What's the
build system ratio on node like, gyp vs. anything else, particularly
with packages close to the node core?

> diff --git a/guix/build/node-build-system.scm b/guix/build/node-
> build-system.scm
> index 70a367618e..6aeb0149dd 100644
> --- a/guix/build/node-build-system.scm
> +++ b/guix/build/node-build-system.scm
> @@ -2,6 +2,8 @@
>  ;;; Copyright © 2015 David Thompson <davet@gnu.org>
>  ;;; Copyright © 2016, 2020 Jelle Licht <jlicht@fsfe.org>
>  ;;; Copyright © 2019, 2021 Timothy Sample <samplet@ngyro.com>
> +;;; Copyright © 2021 Pierre Langlois <pierre.langlois@gmx.com>
> +;;; Copyright © 2021 Philip McGrath <philip@philipmcgrath.com>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -46,6 +48,19 @@ (define (set-home . _)
>                (format #t "set HOME to ~s~%" (getenv "HOME")))))))
>    #t)
>  
> +(define* (set-node-gyp-paths #:key inputs #:allow-other-keys)
> +  "Initialize environment variables needed for building native
> addons."
> +  (setenv "npm_config_nodedir" (assoc-ref inputs "node"))
> +  (setenv "npm_config_python" (assoc-ref inputs "python"))
> +  (setenv "PATH"
> +          (string-append (getenv "PATH")
> +                         ":"
> +                         ;; Put this at the end to make it easier to
> override,
> +                         ;; just in case that should ever be
> necessary:
> +                         (assoc-ref inputs "node")
> +                         "/lib/node_modules/npm/bin/node-gyp-bin"))
> +  #t)
> +
Is this a necessary step to build node modules?  If not can we skip
this step when packages don't need gyp?

>  (define (module-name module)
>    (let* ((package.json (string-append module "/package.json"))
>           (package-meta (call-with-input-file package.json read-
> json)))
> @@ -101,6 +116,12 @@ (define* (configure #:key outputs inputs
> #:allow-other-keys)
>      (invoke npm "--offline" "--ignore-scripts" "install")
>      #t))
>  
> +(define* (configure-gyp #:key inputs #:allow-other-keys)
> +  "Run 'node-gyp configure' if we see a 'binding.gyp' file."
> +  (if (file-exists? "binding.gyp")
> +      (invoke (which "node-gyp") "configure")
> +      #t))
> +
You might want to make this part of configure itself, though I'm not
sure what the consensus is there when mixing different build system
styles.  (invoke (which ...) ) is also a pretty rare pattern, used in
only four locations so far.

Also, while better than the previous thing in that it actually checks
whether we have something gyp-esque at hand, I'd still prefer users
being able to not run this portion through some flag.  See e.g. #:use-
setuptools? in python or #:glib-or-gtk? in meson.

Cheers

Hi!

On 11/14/21 15:44, Liliana Marie Prikler wrote:
> Looking at this patch, it does *a lot* at once and could probably be
> separated into more than one.  Particularly, I'd suggest providing
> capabilities in node-build-system first, then switching over to the new
> thing in node.

Thanks for these comments. Some of the things to which you drew my 
attention seem to have been workarounds for problems that had since been 
solved at a deeper level. Then, in particular, this comment:


 >> +(define* (configure-gyp #:key inputs #:allow-other-keys)
 >> +  "Run 'node-gyp configure' if we see a 'binding.gyp' file."
 >> +  (if (file-exists? "binding.gyp")
 >> +      (invoke (which "node-gyp") "configure")
 >> +      #t))
 >> +
 > You might want to make this part of configure itself, though I'm not
 > sure what the consensus is there when mixing different build system
 > styles.  (invoke (which ...) ) is also a pretty rare pattern, used in
 > only four locations so far.

prompted me to look more closely at why so much manual work was needed 
in the first place.

It turns out that the `npm install` in the `'configure` phase should 
have handled most of it automatically, but the Guix packages were 
deleting the configure phase to avoid checking for devDependencies that 
aren't in Guix (or that we just don't want, e.g. some dependencies of 
node-sqlite3).

In v2 of this series (which will follow this email), I've removed all of 
the `node-gyp`-specific build-side code and tried a more general 
solution, adding an `#:absent-packages` argument to instruct the 
`'patch-dependencies` phase to remove the specified packages from the 
"package.json" file. This means that the Guix package can still run 
`'configure`/`npm install`, checking properly for the packages that we 
*do* have/want and doing all of the other automatic work `npm install` 
does. I also like that listing the missing packages in the Guix package 
definition provides a sort of documentation of what is missing: for 
example, it is clear which packages could have their tests enabled with 
the addition of a `node-tap` package, without having to inspect all of 
the individual "package.json" files.

I've updated all of the existing Node.js packages that deleted their 
`'configure` phase to use `#:absent-dependencies` instead.

>> @@ -58,10 +62,13 @@ (define private-keywords
>>                                 `(("source" ,source))
>>                                 '())
>>                           ,@inputs
>> -
>>                           ;; Keep the standard inputs of 'gnu-build-
>> system'.
>>                           ,@(standard-packages)))
>>            (build-inputs `(("node" ,node)
>> +                         ("python" ,python)
>> +                        ;; We don't always need libuv, but the libuv
>> and
>> +                        ;; node versions need to match:
>> +                        ("libuv" ,@(assoc-ref (package-inputs node)
>> "libuv"))
>>                            ,@native-inputs))
>>            (outputs outputs)
>>            (build node-build)
> Will this python input always or often enough be needed?  What's the
> build system ratio on node like, gyp vs. anything else, particularly
> with packages close to the node core?

GYP is a Python program, and it (or at least node's fork of it) expects 
to have a python executable available to invoke with sub-programs. Since 
npm depends on node-gyp and GYP, it is pretty close to the core.

In v2, I've just had packages that use node-gyp add Python to their 
inputs. IIRC, that used to not be enough, but it seems underlying 
problems were fixed in the mean time.

An alternative approach would be to configure it using the npmrc file, 
as I do for `nodedir` in v2. I'm not sure that makes much difference 
either way, but in v2 I've tried to minimize the amount of 
`node-gyp`-specific handling.

-Philip

On 11/19/21 23:26, Philip McGrath wrote> In v2 of this series (which 
will follow this email)

I'm not sure what went wrong, but I, at least, received these patches 
out of order. Let me know if I should resend them as v3. They are 
definitely in the right order here: 
https://gitlab.com/philip1/guix-patches/-/tree/guix-issue-51838-v2

-Philip

Am Samstag, den 20.11.2021, 00:10 -0500 schrieb Philip McGrath:
> On 11/19/21 23:26, Philip McGrath wrote> In v2 of this series (which 
> will follow this email)
> 
> I'm not sure what went wrong, but I, at least, received these
> patches 
> out of order. Let me know if I should resend them as v3. They are 
> definitely in the right order here: 
> https://gitlab.com/philip1/guix-patches/-/tree/guix-issue-51838-v2
> 
> -Philip
Our mumi frontend to debbugs should be able to deliver them to you as
one convenient mbox: 
https://issues.guix.gnu.org/issue/51838/patch-set/2

Cheers

Hi Philip,

There are some really valuable changes in here, thanks!  I would like to
start understanding and reviewing the changes so that we can get some of
this good stuff merged in.  However, I have one question that I couldn’t
answer from reading here or at <https://issues.guix.gnu.org/49946>: is
there an agreement between you and Pierre that these patches are the
“right” way to do native addons for Node.js.  More importantly, is the
general plan that we merge these changes, and then Pierre rebases their
Tree-sitter changes on top these?

Pierre, maybe you could weigh in here?

Sorry if I missed something.  I assume everything is OK, but I want to
be sure before I start digging into the details of the patches –
especially those first few more complicated ones.  :)

Thanks!


-- Tim

Hi Timothy,

Timothy Sample <samplet@ngyro.com> writes:

> Hi Philip,
>
> There are some really valuable changes in here, thanks!  I would like to
> start understanding and reviewing the changes so that we can get some of
> this good stuff merged in.  However, I have one question that I couldn’t
> answer from reading here or at <https://issues.guix.gnu.org/49946>: is
> there an agreement between you and Pierre that these patches are the
> “right” way to do native addons for Node.js.  More importantly, is the
> general plan that we merge these changes, and then Pierre rebases their
> Tree-sitter changes on top these?
>
> Pierre, maybe you could weigh in here?

The overall approach looks good to me, it's better than what I
originally proposed for sure :-).  That being said, I'm not very
familiar with the Node.js ecosystem so I don't know if it's necessarily
the right way, but I suspect the correct way for node isn't very Guix-y
so I'm not too worried about that.

It's on my TODO list to take another look at the patches as well :-),
then yes, I'm planning on rebasing my tree-sitter series on top.

> Sorry if I missed something.  I assume everything is OK, but I want to
> be sure before I start digging into the details of the patches –
> especially those first few more complicated ones.  :)

Thanks for taking a look!

Pierre

Hello,

Pierre Langlois <pierre.langlois@gmx.com> writes:

> Hi Timothy,
>
> Timothy Sample <samplet@ngyro.com> writes:
>
>> More importantly, is the general plan that we merge these changes,
>> and then Pierre rebases their Tree-sitter changes on top these?
>>
>> Pierre, maybe you could weigh in here?
>
> The overall approach looks good to me, it's better than what I
> originally proposed for sure :-).  That being said, I'm not very
> familiar with the Node.js ecosystem so I don't know if it's necessarily
> the right way, but I suspect the correct way for node isn't very Guix-y
> so I'm not too worried about that.

The whole Node.js bundles NPM, which bundles node-gyp, which bundles a
fork of GYP [1] is not very Guix-y at all, no.  :/  This is one of those
problems (like bootstrapping GCC) that will take years of incremental
improvements and side projects and all that.

[1] Not to get too off topic, but isn’t “gyp” a slur?  How did Google
ever call something that?

> It's on my TODO list to take another look at the patches as well :-),
> then yes, I'm planning on rebasing my tree-sitter series on top.

Excellent!

>> Sorry if I missed something.  I assume everything is OK, but I want to
>> be sure before I start digging into the details of the patches –
>> especially those first few more complicated ones.  :)
>
> Thanks for taking a look!

I have an idea to simplify the patch series a bit: if we can answer my
question here <https://issues.guix.gnu.org/51838#57> and come to a
conclusion about deleting lock files
<https://issues.guix.gnu.org/51838#58>, I could merge the
‘#:absent-dependencies’ part of the patch series.  I think this might
make future re-rolls easier and help rein in the scope a bit.

Thoughts?  Philip?

Thanks!


-- Tim

Hi,

On 11/28/21 14:59, Timothy Sample wrote:
> Pierre Langlois <pierre.langlois@gmx.com> writes:
>> The overall approach looks good to me, it's better than what I
>> originally proposed for sure :-).  That being said, I'm not very
>> familiar with the Node.js ecosystem so I don't know if it's necessarily
>> the right way, but I suspect the correct way for node isn't very Guix-y
>> so I'm not too worried about that.
> 
> The whole Node.js bundles NPM, which bundles node-gyp, which bundles a
> fork of GYP [1] is not very Guix-y at all, no.  :/  This is one of those
> problems (like bootstrapping GCC) that will take years of incremental
> improvements and side projects and all that.

It's something of a tangent, but, as I discussed briefly with Pierre in 
e.g. <https://issues.guix.gnu.org/49946#73>, I think it would make a lot 
of sense, even if we can't unbundle/bootstrap everything, to make the 
355 dependencies of npm we are already distributing available as Guix 
packages. For example, I think I found a bootstrapping cycle with the 
Unicode data packages: even if the version we're currently using is 
messy, letting other packages use it, too, might unlock much more of the 
JavaScript universe. I've experimented a bit (partially looking at what 
Debian does), and I may look at that some time after this.


> I have an idea to simplify the patch series a bit: if we can answer my
> question here <https://issues.guix.gnu.org/51838#57> and come to a
> conclusion about deleting lock files
> <https://issues.guix.gnu.org/51838#58>, I could merge the
> ‘#:absent-dependencies’ part of the patch series.  I think this might
> make future re-rolls easier and help rein in the scope a bit.

That sounds like a great approach! I had not expected this patch series 
to end up touching every JavaScript package in Guix :)

I was offline a bit for holidays here, and just before that I'd noticed 
a problem with npm generating implicit build commands that tried to 
rebuild node-gyp packages when they are installed. I'll page this stuff 
back in and send a v3 soon that hopefully has at least the 
'#:absent-dependencies' part ready to merge.

-Philip