Message ID | 20211114130409.49241-2-philip@philipmcgrath.com |
---|---|
State | Accepted |
Headers | show |
Series | None | expand |
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
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)