-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
[24.05 backport] nodejs changes #336570
[24.05 backport] nodejs changes #336570
Conversation
Hm, did the commit that switches the build system to |
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 that we’d also need #337360 as a followup fix
Given the significant changes to the build-system and the fact that there is in fact fall-out already, I'd like to raise the question whether it's actually a good idea to backport all of that.
Would it be an option to just backport the bumps and bugfixes (if any?)
I agree with Maximilian, not backporting that
Unless I'm missing something, that's currently the case. The changes are either version bump, bug fixes, or "ineffective" changes included just to minimize the amount of conflicts (i.e. all test-related changes don't have any impact on the 24.05 branches because we don't run the tests there, see 21e3782). |
I don't think this needs to be backported at all: yes, further version bumps and all of that will be more painful, but I'd argue it's less of a risk than backporting all the build-system changes. Also, the situation will resolve itself by the end of the year when 24.05 gets retired.
I thought this wasn't the case before since the build-system changes were included? But on a second glance, this isn't the case anymore (I guess the In that case it's fine I'd say. |
Yes, I was a bit confused by this as well and was half-expecting other ninja-related changes 😅
Perhaps we shouldn’t backport 4b90b5e since it is a non-trivial change to the Node.js build process (i.e. using an emulator instead of building for multiple platforms)? Otherwise the commits in this PR look good to me. |
Marking as draft until #339252 can be included (because of #336556 (comment)), and 4b90b5e is removed from this PR. |
(cherry picked from commit 7c1177c)
(cherry picked from commit f75d503)
(cherry picked from commit 070791f)
Previously, the nodejs `passthru.pkgs` attribute refers to the package fixed point derived in the current context, without considering potential overrides to the main nodejs package. This is fixed here with the `.finalPackage` attribute, which refers to the final package fixed point, after potential overrides. For example, previously, after overriding nodejs with `nodejs.override`, `nodejs.pkgs` would still refer to the original package, instead of the overridden one. After this fix, `nodejs.pkgs` would be correctly based on the overridden `nodejs` package. (cherry picked from commit ee9a623)
(cherry picked from commit d7f46fb)
(cherry picked from commit 31d2381)
(cherry picked from commit 35b556d)
(cherry picked from commit 4c38d8a)
(cherry picked from commit c455cc3)
(cherry picked from commit 45db1ff)
(cherry picked from commit 235ae9c)
15ab207
to
4682120
Compare
4682120
to
64c2100
Compare
I guess I should wait for |
64c2100
to
4682120
Compare
Description of changes
Generated with
I only picked the commits that did not break the builds.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.