diff mbox series

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

Message ID 20211114130409.49241-2-philip@philipmcgrath.com
State New
Headers show
Series None | expand

Commit Message

Philip McGrath Nov. 14, 2021, 1:04 p.m. UTC
* 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(-)

Comments

Liliana Marie Prikler Nov. 14, 2021, 8:44 p.m. UTC | #1
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
Philip McGrath Nov. 20, 2021, 4:26 a.m. UTC | #2
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
Philip McGrath Nov. 20, 2021, 5:10 a.m. UTC | #3
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
Liliana Marie Prikler Nov. 20, 2021, 7:28 a.m. UTC | #4
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
Timothy Sample Nov. 20, 2021, 8:08 p.m. UTC | #5
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
diff mbox series

Patch

diff --git a/gnu/packages/node.scm b/gnu/packages/node.scm
index 89a9bc7533..ad5179426a 100644
--- a/gnu/packages/node.scm
+++ b/gnu/packages/node.scm
@@ -237,21 +237,20 @@  (define-public node
                             python
                             (string-append python "3")))
                       "configure" flags))))
-         (add-after 'patch-shebangs 'patch-npm-shebang
-           (lambda* (#:key outputs #:allow-other-keys)
-             (let* ((bindir (string-append (assoc-ref outputs "out")
-                                           "/bin"))
-                    (npm    (string-append bindir "/npm"))
-                    (target (readlink npm)))
-               (with-directory-excursion bindir
-                 (patch-shebang target (list bindir))))))
-         (add-after 'install 'patch-node-shebang
-           (lambda* (#:key outputs #:allow-other-keys)
-             (let* ((bindir (string-append (assoc-ref outputs "out")
-                                           "/bin"))
-                    (npx    (readlink (string-append bindir "/npx"))))
-               (with-directory-excursion bindir
-                 (patch-shebang npx (list bindir)))))))))
+         (add-after 'patch-shebangs 'patch-nested-shebangs
+           (lambda* (#:key inputs outputs #:allow-other-keys)
+             (let* ((prefix (assoc-ref outputs "out"))
+                    (path (map (lambda (dir)
+                                 (string-append dir "/bin"))
+                               (list prefix
+                                     (assoc-ref inputs "python-for-target")))))
+               (for-each
+                (lambda (file)
+                  (patch-shebang file path))
+                (find-files (string-append prefix "/lib/node_modules")
+                            (lambda (file stat)
+                              (executable-file? file))
+                            #:stat lstat))))))))
     (native-inputs
      `(;; Runtime dependencies for binaries used as a bootstrap.
        ("c-ares" ,c-ares)
@@ -274,6 +273,7 @@  (define-public node
     (inputs
      `(("bash" ,bash-minimal)
        ("coreutils" ,coreutils)
+       ("python-for-target" ,python-wrapper) ;; for node-gyp (supports python3)
        ("c-ares" ,c-ares)
        ("http-parser" ,http-parser)
        ("icu4c" ,icu4c)
@@ -795,6 +795,7 @@  (define-public node-lts
     (inputs
      `(("bash" ,bash-minimal)
        ("coreutils" ,coreutils)
+       ("python-for-target" ,python-wrapper) ;; for node-gyp (supports python3)
        ("c-ares" ,c-ares)
        ("icu4c" ,icu4c-67)
        ("libuv" ,libuv-for-node)
@@ -813,5 +814,4 @@  (define-public libnode
         `(cons* "--shared" "--without-npm" ,flags))
        ((#:phases phases '%standard-phases)
         `(modify-phases ,phases
-           (delete 'patch-npm-shebang)
-           (delete 'patch-node-shebang)))))))
+           (delete 'patch-nested-shebangs)))))))
diff --git a/guix/build-system/node.scm b/guix/build-system/node.scm
index 98f63f87ef..fee4142a99 100644
--- 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)
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)
+
 (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))
+
 (define* (build #:key inputs #:allow-other-keys)
   (let ((package-meta (call-with-input-file "package.json" read-json)))
     (if (and=> (assoc-ref package-meta "scripts")
@@ -144,9 +165,11 @@  (define* (install #:key outputs inputs #:allow-other-keys)
 
 (define %standard-phases
   (modify-phases gnu:%standard-phases
+    (add-after 'set-paths 'set-node-gyp-paths set-node-gyp-paths)
     (add-after 'unpack 'set-home set-home)
     (add-before 'configure 'patch-dependencies patch-dependencies)
     (replace 'configure configure)
+    (add-after 'configure 'configure-gyp configure-gyp)
     (replace 'build build)
     (replace 'check check)
     (add-before 'install 'repack repack)