| Message ID | 20211120043406.952350-10-philip@philipmcgrath.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [bug#51838,v2,01/26] gnu: node: Avoid duplicating build phases. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
Am Freitag, den 19.11.2021, 23:33 -0500 schrieb Philip McGrath: > gnu/packages/node.scm (node-llparse-builder-bootstrap)[arguments]: > Add `#:absent-dependencies`. Stop deleting the `'configure` phase. > Add a new phase `#:delete-package-lock` to remove the > problematic "package-lock.json". > --- > gnu/packages/node.scm | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/gnu/packages/node.scm b/gnu/packages/node.scm > index 98a51276e7..9d4903a8ca 100644 > --- a/gnu/packages/node.scm > +++ b/gnu/packages/node.scm > @@ -479,9 +479,21 @@ (define-public node-llparse-builder-bootstrap > (arguments > `(#:node ,node-bootstrap > #:tests? #f > + #:absent-dependencies > + `("@types/mocha" > + "@types/node" > + "mocha" > + "ts-node" > + "tslint" > + "typescript") > #:phases > (modify-phases %standard-phases > - (delete 'configure) > + (add-before 'configure 'remove-package-lock > + ;; Having package-lock.json seems to cause npm > + ;; to look for things on the internet in the configure > phase, > + ;; even if we have them properly installed. > + (lambda args > + (delete-file-recursively "package-lock.json"))) I think a simple delete-file ought to work. This should also be done by configure or similar, compare cargo-build-system.
On 11/20/21 02:44, Liliana Marie Prikler wrote: > Am Freitag, den 19.11.2021, 23:33 -0500 schrieb Philip McGrath: >> (modify-phases %standard-phases >> - (delete 'configure) >> + (add-before 'configure 'remove-package-lock >> + ;; Having package-lock.json seems to cause npm >> + ;; to look for things on the internet in the configure >> phase, >> + ;; even if we have them properly installed. >> + (lambda args >> + (delete-file-recursively "package-lock.json"))) > I think a simple delete-file ought to work. This should also be done > by configure or similar, compare cargo-build-system. Yes, I thought about having the build system automatically delete "package-lock.json", but I'm not 100% sure it's always a problem to have it, or if there might even be some circumstance where we want to keep it. I'd prefer to wait until we see a significant number of node packages deleting their "package-lock.json" files before trying to abstract over the pattern.
Hey Philip, Philip McGrath <philip@philipmcgrath.com> writes: > gnu/packages/node.scm (node-llparse-builder-bootstrap)[arguments]: Add > `#:absent-dependencies`. Stop deleting the `'configure` phase. > Add a new phase `#:delete-package-lock` to remove the > problematic "package-lock.json". > --- > gnu/packages/node.scm | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/gnu/packages/node.scm b/gnu/packages/node.scm > index 98a51276e7..9d4903a8ca 100644 > --- a/gnu/packages/node.scm > +++ b/gnu/packages/node.scm > @@ -479,9 +479,21 @@ (define-public node-llparse-builder-bootstrap > (arguments > `(#:node ,node-bootstrap > #:tests? #f > + #:absent-dependencies > + `("@types/mocha" > + "@types/node" > + "mocha" > + "ts-node" > + "tslint" > + "typescript") > #:phases > (modify-phases %standard-phases > - (delete 'configure) > + (add-before 'configure 'remove-package-lock > + ;; Having package-lock.json seems to cause npm > + ;; to look for things on the internet in the configure phase, > + ;; even if we have them properly installed. package-lock.json lists exact versions _and integrity hashes_; since it seems unlikely that after node-build-system's finaggling we end up with an identical hash, we will always have a mismatch and fetch 'proper' sources online accordingly. As far as npm + package-lock.json are concerned, we don't have them properly installed. From what I have seen package-lock.json offers us no benefits (because we track exact dependency information via the guix store) and can (as you have seen) prevent builds from working. My 2c: always remove it in a phase in the build system. It's simply a preference though, so your please go with what you think is the right choice. You might want to update the comment nonetheless so it's clear we know what's going on. > + (lambda args > + (delete-file-recursively "package-lock.json"))) > (replace 'build > (lambda* (#:key inputs #:allow-other-keys) > (let ((esbuild (string-append (assoc-ref inputs "esbuild") > -- > 2.32.0
Hello! Jelle Licht <jlicht@fsfe.org> writes: > Philip McGrath <philip@philipmcgrath.com> writes: > >> Add a new phase `#:delete-package-lock` to remove the >> problematic "package-lock.json". > > package-lock.json lists exact versions _and integrity hashes_; since it > seems unlikely that after node-build-system's finaggling we end up with > an identical hash, we will always have a mismatch and fetch 'proper' > sources online accordingly. As far as npm + package-lock.json are > concerned, we don't have them properly installed. > > From what I have seen package-lock.json offers us no benefits (because > we track exact dependency information via the guix store) and can (as > you have seen) prevent builds from working. My 2c: always remove it in a > phase in the build system. I’m inclined to agree with Jelle and Liliana. I can’t imagine a situation in which we would want the lock files. We could be wrong, but we can always adjust the build system later if something surprising happens (e.g., ‘#:keep-lock-file?’ or whatever). -- Tim
On 11/28/21 14:35, Timothy Sample wrote: > Jelle Licht <jlicht@fsfe.org> writes: >> From what I have seen package-lock.json offers us no benefits (because >> we track exact dependency information via the guix store) and can (as >> you have seen) prevent builds from working. My 2c: always remove it in a >> phase in the build system. > > I’m inclined to agree with Jelle and Liliana. I can’t imagine a > situation in which we would want the lock files. We could be wrong, but > we can always adjust the build system later if something surprising > happens (e.g., ‘#:keep-lock-file?’ or whatever). This makes sense to me. Should we also delete "yarn.lock" (respected as of npm v7)[1] and "npm-shrinkwrap.json"[2]? It seems like the same reasons probably apply. We might want to handle "node_modules/.package-lock.json"[3], too, but that seems less likely to exist. -Philip [1]: https://blog.npmjs.org/post/621733939456933888/npm-v7-series-why-keep-package-lockjson.html [2]: https://docs.npmjs.com/cli/v8/configuring-npm/package-lock-json#package-lockjson-vs-npm-shrinkwrapjson [3]: https://docs.npmjs.com/cli/v8/configuring-npm/package-lock-json#package-lockjson-vs-npm-shrinkwrapjson
Hi, Am Donnerstag, den 02.12.2021, 16:18 -0500 schrieb Philip McGrath: > On 11/28/21 14:35, Timothy Sample wrote: > > Jelle Licht <jlicht@fsfe.org> writes: > > > From what I have seen package-lock.json offers us no benefits > > > (because we track exact dependency information via the guix > > > store) and can (as you have seen) prevent builds from working. My > > > 2c: always remove it in a phase in the build system. > > > > I’m inclined to agree with Jelle and Liliana. I can’t imagine a > > situation in which we would want the lock files. We could be > > wrong, but we can always adjust the build system later if something > > surprising happens (e.g., ‘#:keep-lock-file?’ or whatever). > > This makes sense to me. Should we also delete "yarn.lock" (respected > as of npm v7)[1] and "npm-shrinkwrap.json"[2]? It seems like the > same reasons probably apply. If it breaks builds, then yes. > We might want to handle "node_modules/.package-lock.json"[3], too, > but that seems less likely to exist. What are node_modules in this context? To me that sounds like "vendored packages" and if so, they ought to (ideally) be deleted in a snippet way before unpack. Meaning you wouldn't have to handle that case in configure. Cheers
Hi! Here is a v3 of this patch series, hopefully incorporating all of the suggestions. In particular, as Timothy suggested in <https://issues.guix.gnu.org/51838#59>, I've tried to make sure the earlier patches, at least through #:absent-dependencies, are able to be applied even if there is more discussion needed on the later patches. As I mentioned in <https://issues.guix.gnu.org/51838#64>, while experimenting with v2 of this series, I discovered an issue with the install script automatically generated by npm. Patch 21 ``guix: node-build-system: Add avoid-node-gyp-rebuild phase.'' fixed the issue: I explain the details in comments there. This series also adds two additional leaf packages, `node-segfault-handler' and `node-serialport', that confirm these patches can support additional styles of native addons. In particular, because the native addon for `node-serialport' is actually in one of the intermediate packages, `node-serialport-bindings', it serves as a test case for the new `avoid-node-gyp-rebuild' phase. I've also put these patches up at <https://gitlab.com/philip1/guix-patches/-/tree/guix-issue-51838-v3>, if anyone finds that useful. -Philip Philip McGrath (43): gnu: node: Avoid duplicating build phases. gnu: node: Update to 10.24.1 for bootstrapping. gnu: node: Patch shebangs in node_modules. gnu: node: Add an npmrc file to set nodedir. guix: node-build-system: Refactor patch-dependencies phase. guix: node-build-system: Add #:absent-dependencies argument. gnu: node-semver-bootstrap: Use #:absent-dependencies. gnu: node-ms-bootstrap: Use #:absent-dependencies. gnu: node-binary-search-bootstrap: Use #:absent-dependencies. gnu: node-debug-bootstrap: Use #:absent-dependencies. gnu: node-llparse-builder-bootstrap: Use #:absent-dependencies. gnu: node-llparse-frontend-bootstrap: Use #:absent-dependencies. gnu: node-llparse-bootstrap: Use #:absent-dependencies. gnu: node-semver: Use #:absent-dependencies. gnu: node-wrappy: Use #:absent-dependencies. gnu: node-once: Use #:absent-dependencies. gnu: node-irc-colors: Use #:absent-dependencies. gnu: node-irc: Use #:absent-dependencies. guix: node-build-system: Add implicit libuv input. guix: node-build-system: Add delete-lockfiles phase. guix: node-build-system: Add avoid-node-gyp-rebuild phase. gnu: Add node-inherits. gnu: Add node-safe-buffer. gnu: Add node-string-decoder. gnu: Add node-readable-stream. gnu: Add node-nan. gnu: Add node-openzwave-shared. gnu: Add node-addon-api. gnu: Add node-sqlite3. gnu: Add node-file-uri-to-path. gnu: Add node-bindings. gnu: Add node-segfault-handler. gnu: Add node-serialport-binding-abstract. gnu: Add node-serialport-parser-delimiter. gnu: Add node-serialport-parser-readling. gnu: Add node-serialport-bindings. gnu: Add node-serialport-parser-regex. gnu: Add node-serialport-parser-ready. gnu: Add node-serialport-parser-inter-byte-timeout. gnu: Add node-serialport-parser-cctalk. gnu: Add node-serialport-parser-byte-length. gnu: Add node-serialport-stream. gnu: Add node-serialport. gnu/packages/node-xyz.scm | 833 +++++++++++++++++++++++++++++-- gnu/packages/node.scm | 219 ++++---- gnu/packages/zwave.scm | 65 +++ guix/build-system/node.scm | 28 +- guix/build/node-build-system.scm | 129 ++++- 5 files changed, 1119 insertions(+), 155 deletions(-)
Hi Philip, Philip McGrath <philip@philipmcgrath.com> writes: > Hi! > > Here is a v3 of this patch series, hopefully incorporating all of the > suggestions. In particular, as Timothy suggested > in <https://issues.guix.gnu.org/51838#59>, I've tried to make sure the earlier > patches, at least through #:absent-dependencies, are able to be applied even > if there is more discussion needed on the later patches. > > As I mentioned in <https://issues.guix.gnu.org/51838#64>, while experimenting > with v2 of this series, I discovered an issue with the install script > automatically generated by npm. Patch 21 ``guix: node-build-system: Add > avoid-node-gyp-rebuild phase.'' fixed the issue: I explain the details in > comments there. > > This series also adds two additional leaf packages, `node-segfault-handler' > and `node-serialport', that confirm these patches can support additional > styles of native addons. In particular, because the native addon for > `node-serialport' is actually in one of the intermediate packages, > `node-serialport-bindings', it serves as a test case for the new > `avoid-node-gyp-rebuild' phase. > > I've also put these patches up > at <https://gitlab.com/philip1/guix-patches/-/tree/guix-issue-51838-v3>, > if anyone finds that useful. Thanks for working on this! I've tested the series and rebased my own work on top locally, it's working for me so feel free to add: Tested-by: Pierre Langlois <pierre.langlois@gmx.com> The series looks good to me overall, I'll add comments in each patch. Thanks, Pierre
Pierre Langlois <pierre.langlois@gmx.com> writes: > [[PGP Signed Part:Undecided]] > Hi Philip, > > Philip McGrath <philip@philipmcgrath.com> writes: > >> Hi! >> >> Here is a v3 of this patch series, hopefully incorporating all of the >> suggestions. In particular, as Timothy suggested >> in <https://issues.guix.gnu.org/51838#59>, I've tried to make sure the earlier >> patches, at least through #:absent-dependencies, are able to be applied even >> if there is more discussion needed on the later patches. >> >> As I mentioned in <https://issues.guix.gnu.org/51838#64>, while experimenting >> with v2 of this series, I discovered an issue with the install script >> automatically generated by npm. Patch 21 ``guix: node-build-system: Add >> avoid-node-gyp-rebuild phase.'' fixed the issue: I explain the details in >> comments there. >> >> This series also adds two additional leaf packages, `node-segfault-handler' >> and `node-serialport', that confirm these patches can support additional >> styles of native addons. In particular, because the native addon for >> `node-serialport' is actually in one of the intermediate packages, >> `node-serialport-bindings', it serves as a test case for the new >> `avoid-node-gyp-rebuild' phase. >> >> I've also put these patches up >> at <https://gitlab.com/philip1/guix-patches/-/tree/guix-issue-51838-v3>, >> if anyone finds that useful. > > Thanks for working on this! I've tested the series and rebased my own > work on top locally, it's working for me so feel free to add: > > Tested-by: Pierre Langlois <pierre.langlois@gmx.com> > > The series looks good to me overall, I'll add comments in each patch. OK, I'm done with my round of comments :-) I'm not a maintainer but I do have commit access, so I can volonteer to push this on your behalf if maintainers are happy with the series. Hopefully with some of my suggestions incorporated if you agree with them. Thanks, Pierre
Hi! On 12/12/21 11:36, Pierre Langlois wrote: > > Pierre Langlois <pierre.langlois@gmx.com> writes: >> Thanks for working on this! I've tested the series and rebased my own >> work on top locally, it's working for me so feel free to add: >> >> Tested-by: Pierre Langlois <pierre.langlois@gmx.com> >> >> The series looks good to me overall, I'll add comments in each patch. > > OK, I'm done with my round of comments :-) > > I'm not a maintainer but I do have commit access, so I can volonteer to > push this on your behalf if maintainers are happy with the series. > Hopefully with some of my suggestions incorporated if you agree with > them. Thanks for the review! I'll send a v4 incorporating your comments, probably later today or tomorrow, and hopefully that will be ready to merge. It will also include node-debug, because I discovered that the lack of it causes some problems for the node-serialport packages, and it turned out not to be too difficult to add. -Philip
diff --git a/gnu/packages/node.scm b/gnu/packages/node.scm index 98a51276e7..9d4903a8ca 100644 --- a/gnu/packages/node.scm +++ b/gnu/packages/node.scm @@ -479,9 +479,21 @@ (define-public node-llparse-builder-bootstrap (arguments `(#:node ,node-bootstrap #:tests? #f + #:absent-dependencies + `("@types/mocha" + "@types/node" + "mocha" + "ts-node" + "tslint" + "typescript") #:phases (modify-phases %standard-phases - (delete 'configure) + (add-before 'configure 'remove-package-lock + ;; Having package-lock.json seems to cause npm + ;; to look for things on the internet in the configure phase, + ;; even if we have them properly installed. + (lambda args + (delete-file-recursively "package-lock.json"))) (replace 'build (lambda* (#:key inputs #:allow-other-keys) (let ((esbuild (string-append (assoc-ref inputs "esbuild")