-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
buildNpmPackage: init #189539
buildNpmPackage: init #189539
Conversation
# TODO: how do we expose these like Rust? `npmHooks`? | ||
inherit (callPackage ./hooks { inherit nodejs; }) npmConfigHook npmBuildHook npmInstallHook; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure what the best name for exposing the hooks would be, which is why I didn't do it here.
|
||
outputHashMode = "recursive"; | ||
outputHash = hash; | ||
} // args); # TODO: removeAttrs? what attrs? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw this in fetchCargoTarball
-- what's the benefit of removing the args it does? Should we remove any here?
pkgs/servers/jellyfin/web.nix
Outdated
@@ -62,6 +34,5 @@ stdenv.mkDerivation rec { | |||
homepage = "https://jellyfin.org/"; | |||
license = licenses.gpl2Plus; | |||
maintainers = with maintainers; [ nyanloutre minijackson purcell jojosch ]; | |||
platforms = nodejs.meta.platforms; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: set this by default in buildNpmPackage
.
pkgs/top-level/all-packages.nix
Outdated
@@ -4112,7 +4112,9 @@ with pkgs; | |||
|
|||
jellyfin-mpv-shim = python3Packages.callPackage ../applications/video/jellyfin-mpv-shim { }; | |||
|
|||
jellyfin-web = callPackage ../servers/jellyfin/web.nix { }; | |||
jellyfin-web = callPackage ../servers/jellyfin/web.nix { | |||
buildNpmPackage = buildNpmPackage.override { nodejs = nodejs-14_x; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should probably comment why this is done (Jellyfin's web client uses lockfile v1 at this tag, which doesn't work well on newer versions with our setup).
Maybe I can also add a hint for this somewhere, based on the version? Or maybe just in docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it'd be better to have an attribute along the lines of nodeJs
which defines the version used.
That would make it more clear from the child derivation which version is used, and make changing it (upon a package update or something) more intuative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused -- what do you mean by this, where would that attribute be located? Can you provide an example of what that API would look like?
Ah, I see what you mean -- define the Node copy used in the arguments to the builder. My only issue with that is that we don't do it for any other language, see rustPlatform
and buildGo117Module
. We should probably strive for consistency with other languages, in this case. How about at least defining aliases like buildNpmPackageNode14
(I can't come up with any clear naming, but you get what I mean), so overrides for common cases don't have to be done manually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, I forgot other builders do it like that. Creating buildNpmPackage14
etc sounds like a good idea 👍
Maybe buildNpm14Package
would be better naming? I'm not entirely sure.
{ name ? "npm-deps" | ||
, src ? null | ||
, srcs ? [ ] | ||
, patches ? [ ] | ||
, sourceRoot ? [ ] | ||
, hash ? "" | ||
, ... | ||
} @ args: stdenv.mkDerivation ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why these are set to their default/empty values in fetchCargoTarball
... I did it here anyways, though I'm guessing they can probably be removed (both here and in fetchCargoTarball
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think { foo ? {} } @ args:
doesn't keep the {}
for foo
, args
removes the default.
A |
let lock: PackageLock = serde_json::from_str(&fs::read_to_string(args.next().unwrap())?)?; | ||
|
||
let out = PathBuf::from(args.next().unwrap()).join("_cacache"); | ||
|
||
let print_hash = args.next().is_some(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a help message would be good, considering this will be used by update scripts.
, srcs ? [ ] | ||
, patches ? [ ] | ||
, sourceRoot ? [ ] | ||
, hash ? "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should support for sha256
be added, to match other fetchers? (As well as adding npmDepsSha256
to buildNpmPackage
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, i think that's a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is looking great, I'm very happy to see npm build support being improved this substantially :)
Left you some comments/suggestions.
pkgs/build-support/node/build-npm-package/hooks/npm-config-hook.sh
Outdated
Show resolved
Hide resolved
pkgs/build-support/node/build-npm-package/hooks/npm-config-hook.sh
Outdated
Show resolved
Hide resolved
pkgs/build-support/node/build-npm-package/hooks/npm-config-hook.sh
Outdated
Show resolved
Hide resolved
echo "Finished npmSetupHook" | ||
} | ||
|
||
configurePhase=npmSetupHook |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think its a good idea to make this overridable.
configurePhase=npmSetupHook | |
if [ -z "$configurePhase" ]; do | |
configurePhase=npmSetupHook | |
fi |
Same goes for the other phases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only phase that that wasn't added -- I'm not sure when you'd want to override this when using buildNpmPackage
(and duplicate the effort in the phase), since it sets up everything, but for consistency I guess that makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth adding corresponding dont
options (or options that act as that) to all these phases as well (which I only currently do in two of them).
Though, now that I think about it, having both conditionals creates confusion:
- When using
buildNpmPackage
, setting a customconfigurePhase
will cause the npm phase not to be used - When using this hook on its own, why would you set
dontNpmConfigure
instead of just... not including the hook?
I don't know, I'm confused. Cargo does this in its build hook, and I don't really see why both are needed.
@jonringer What do you think about this, as the person who added these options in #114716? It seems like these options don't really provide much benefit when hooks are used properly -- maybe I'm missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth adding corresponding dont options (or options that act as that) to all these phases as well (which I only currently do in two of them).
I definitely agree, that's the most consistent with other builders as well AFAIK. Can't hurt to have a mechanism to disable these when using buildNpmPackage
:)
When using buildNpmPackage, setting a custom configurePhase will cause the npm phase not to be used
Yes, which might be preferable in some scenarios. That way you can have complete control over the build process, you can override the phase with custom behaviour that produces a similar result yourself.
When using this hook on its own, why would you set dontNpmConfigure instead of just... not including the hook?
That does seem useless yeah, I can't imagine any scenario you'd do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't hurt to have a mechanism to disable these when using
buildNpmPackage
To be clear, what I mean is that all of these should be disabled if a custom phase is already set -- having dont
options is useless as long as those conditionals exist.
pkgs/build-support/node/build-npm-package/hooks/npm-config-hook.sh
Outdated
Show resolved
Hide resolved
pkgs/build-support/node/build-npm-package/hooks/npm-config-hook.sh
Outdated
Show resolved
Hide resolved
pkgs/build-support/node/build-npm-package/hooks/npm-config-hook.sh
Outdated
Show resolved
Hide resolved
, srcs ? [ ] | ||
, patches ? [ ] | ||
, sourceRoot ? [ ] | ||
, hash ? "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, i think that's a good idea.
{ name ? "npm-deps" | ||
, src ? null | ||
, srcs ? [ ] | ||
, patches ? [ ] | ||
, sourceRoot ? [ ] | ||
, hash ? "" | ||
, ... | ||
} @ args: stdenv.mkDerivation ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think { foo ? {} } @ args:
doesn't keep the {}
for foo
, args
removes the default.
I just saw this is still marked as a draft, it appears i got a bit excited with the review 😅 Hope you dont mind |
These are the kind of reviews I want, hah -- these'll get it ready to actually be merged. |
@IvarWithoutBones I've migrated to I can't respond to some of your comments for whatever reason... any clue why? This is all I see: These look like responses to my other comments, but GitHub... doesn't properly show them as responses? 🧐 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't have the spoons to look at this in detail anytime soon.
I really like the overall design of the fetcher, it is similar to fetchYarnDeps which is already proven, just for npm. Much less risks for FOD reproducibility compared to running npm ci
in an FOD.
Doing the second part - buildNpmPackage - right is pretty hard though. I feel like Javascript builds don't really match the stdenv model with its phases well. I wonder if the fetcher could be useful already without a specialized build environment, or just with an npm install hook. This could reduce the scope of this PR a lot.
Anyways, thanks for working on this!
Oh, there is a critical feature for helping debug FOD issues that we have in fetchYarnDeps, and I'd also like to see here: Copy the package-lock.json to the FOD output. |
Do you have an example as to what could cause problems with our stdenv's phases? I'm not very familiar with Javascript, but the current implementation looks like it fits in fairly well.
I do think using just the fetcher would be more work in the end. Packages could be refactored to use it but would still end up having a fair bit of code duplication. They would have to be refactored again once The added complexity of the configure and build hooks is fairly small, I'd say it is definitely worth trying to get those additions merged as well if we're gonna include the install hook. I do think reducing the scope of this PR is something that needs to happen though. In my opinion we should drop the commits converting packages to this new builder. Those will all need to be reviewed by the individual packages maintainers, which will greatly increase the time and care needed to get this merged. Without those changes I think the diff is very reasonable. |
Neat, this looks a lot nicer IMO :)
I think that's because I was responding to comments you posted in my review. Github just shows my one comment but doesn't display any additional context or let you reply in that case. It's kinda weird. If you scroll up to your original comment you should be able to reply. |
Can you provide an example as to what you mean? I feel like the default phases I have here are pretty representative of what projects that use npm do:
Of course, these can always be overridden by packages when needed, but I feel like these are good defaults. |
This is already done, by the way. :) |
Still, putting complex logic inside an FOD always comes with some risk. If at any time, there appears to be a bug in your fetcher, you might not be able to fix it without breaking peoples hashes. Also, adding features later might be hard, because those must never change the output for existing users. It would be good if critical inputs, like the rust code of the fetcher, would trigger a re-build of the FOD once changed. |
I already do this for a cherry-picked set of test cases, which will be run automatically by OfBorg whenever the code is changed (see here). |
8fc8ac2
to
90250e2
Compare
Hi all. This push addresses a majority of comments surrounding the docs and flags, as well as fixes using old lockfiles with modern npm. As such, I've marked this PR as ready for review. I still need to find a clean way to make a For future reference: that one Nix test is failing even when marked as flaky. Triggered a re-run. |
I am super excited about this getting merged to nixpkgs. Thank you so much for this amazing work! How hard do you think it would be to write something like this but for the |
I'm also excited to try this tbh, this might need more work, but I think it's ready for testing, we can add subsequent PRs later. |
I would appreciate you having asked me if this was acceptable to be merged, @happysalada. I was waiting on some reviews. Maybe I should have just kept this as a draft? 😕 |
My bad, there usually isnt a lot of action on some MRs, i didnt want to let this go to waste. Lets keep the discussion going here to see if any changes are requested. Ill be sure to ask you before a merge on any follow ups |
Please be more diligent in reading the (most recent) discussion.
I don't see what you mean? This would have been merged in the next few weeks. There is still active interest in it. It wouldn't have gone to waste in any circumstance. |
There were several previous attempts at this that just died out after inactivity. Its often quite hard to know if something will just die or if there is still legitimate interest. Being eager about this i didnt read the comments well enough. Please accept my apology for merging too early. |
We can still revert and re-open, to avoid locking users into an unfinished API. Just sad to lose some context of then discussion. It's your PR @winterqt, what do you prefer? |
FWIW I would not have found this if it wasn't merged today, and it was exactly what I needed for a new package (etherpad-lite) that just wasn't going to work with node2nix. Leaving some feedback here from my experiments, lmk if you'd rather collect this somewhere else. Ordered from most important (bugs -> missing features) to least important (docs/confusion -> notes):
I'm super excited to see something making nodejs/npm app packaging in nixpkgs better! Let me know if any of my previous points are unclear and if I can help with repro or anything. https://gist.github.com/delroth/e7b67c8cc514a9de62f6c53656d1c96a is my current draft package with which I found these issues :) |
@happysalada It's alright. @fricklerhandwerk I'm going to keep it like this, but I'd really appreciate a review of the documentation (as you didn't respond to my previous request for one). I want it to be ~perfect before branch-off/before 21.11, hopefully. @delroth I've addressed your issues in #200470, but I also wanted to comment on a few things that we didn't fix in #dev or the linked PR:
This was deliberate. I don't see a reason for packages that don't use node-gyp to needlessly depend on Python 3 (yay, closure size).
:) I'll get this switched over.
❤️ Yeah, me too. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/announcing-js2nix-scale-your-node-js-project-builds-with-nix/23111/8 |
Thanks for this. Sorry if I'm missing something obvious, but if I'd like to use this not just to package someone else's Node package for Nix, but also to develop/maintain my own, how does one get a Nix shell with the package's dependencies in it? For example, if I've got a Node package in a git repo, and I'd like to use % nix develop
...
bash-5.1$ npm build where all the Node dependencies needed for the |
Description of changes
This PR introduces
buildNpmPackage
, a new builder for npm-based projects. Alongside it isfetchNpmDeps
, a new fetcher for npm dependencies inspired byfetchYarnDeps
in that it constructs a cache for npm to install from using a small purpose-built fetcher.I've tested this with both npm 8 (shipped in Node 18, the latest version) as well as npm 6 (shipped in Node 14). The cache format has been stable for 5 years, which makes me confident in using it as an output for a FOD.
I believe this is an improvement in terms of reproducibility over existing solutions to this issue, most notably #128749. This is notably more reproducible for a few reasons, but the main one is that it's guaranteed to have the same output across systems. This is because npm allows installing dependencies only on a specific platform, which obviously causes irreproducibility.
This will be released in npm 9, but I'll need to backport it to Node versions that don't include it.npmInstallHook
depends on npm/cli#5430 when using packages with{pre,post}pack
scripts.Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes