Skip to content
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.

Sign up for GitHub

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

Jump to bottom

nodePackages: patch node2nix for npm v7+ and switch to building package set with current nodejs #193337

Merged
merged 11 commits into from
Sep 28, 2022
Merged

nodePackages: patch node2nix for npm v7+ and switch to building package set with current nodejs #193337

merged 11 commits into from
Sep 28, 2022

Conversation

lilyinstarlight
Copy link
Member

@lilyinstarlight lilyinstarlight commented Sep 28, 2022 •
edited
Loading

Description of changes

This PR updates the nodePackages set to the latest Node.js LTS of the nodejs package, which is now nodejs-18_x. It does this by importing the fix from svanderburg/node2nix#302 that implements compatbility with NPM v7+ / Node.js 16+

Some notes:

  • I tried to make sure any packages that were currently building in nodePackages did not regress after this update. Only one package does not build in this PR that built before, nodePackages.mdctl-cli, since it does not support Node.js >14 yet, so I have marked it broken (but it still can be used via nodejs-14_x.pkgs.mdctl-cli as needed)
  • Some packages did not seem to be fully building during the npm rebuild step before, and while I'm not sure if they were still somehow working properly (I didn't test), I updated their build inputs to allow them to build with newer Node.js
  • I spot checked a number of CLI packages to make sure they all work as intended, but I could have missed some so feel free to check any packages you personally use

Note: I did not target this towards staging since the rebuild count is not too big, but if it should be targeted towards staging to help breakages get caught there first, I'm happy to retarget this branch

Closes #132456, closes #145432, closes #146440, closes #170722, closes #187337

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: vim 6.topic: vscode 8.has: changelog 8.has: documentation This PR adds or changes documentation labels Sep 28, 2022
@lilyinstarlight lilyinstarlight changed the title Fix/node2nix npmv7 nodePackages: patch node2nix for npm v7+ and switch to building package set with current nodejs Sep 28, 2022
@lilyinstarlight lilyinstarlight mentioned this pull request Sep 28, 2022
Merged
@lilyinstarlight
Copy link
Member Author

The manual is failing build on master too so the CI failure seems unrelated to this change (fix for that is in #193338)

This was referenced Sep 28, 2022
Closed
Closed
Closed
SuperSandro2000
SuperSandro2000 reviewed Sep 28, 2022
View reviewed changes
pkgs/tools/graphics/ldgallery/viewer/default.nix
@@ -9,16 +9,13 @@ let
sourcePkg = fetchFromGitHub {
owner = "pacien";
repo = "ldgallery";
rev = "v2.0";
sha256 = "1a82wy6ns1434gdba2l04crvr5waf03y02bappcxqci2cfb1cznz";
rev = "v2.1";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be derived from ldgallery-compiler.version?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be if you'd rather! I tried to make mostly minimal changes to get apps to build again after the update, so I left patterns like that as they were before. I'm happy to make the change though to avoid version desync for this particular package

@happysalada
Copy link
Contributor

CI build failure https://github.com/NixOS/nixpkgs/actions/runs/3144204430/jobs/5109902065
My opinion on this personally is to merge as soon as the CI passes. This is important and it is prone to conflicts. We can fix the small details in subsequent PRs (if you don't mind of course)

@lilyinstarlight
Copy link
Member Author

lilyinstarlight commented Sep 28, 2022 •
edited
Loading

CI build failure https://github.com/NixOS/nixpkgs/actions/runs/3144204430/jobs/5109902065 My opinion on this personally is to merge as soon as the CI passes. This is important and it is prone to conflicts. We can fix the small details in subsequent PRs (if you don't mind of course)

I've rebased the PR to pull in the manual fix from #193338 (unlucky timing I guess that I rebased initially in the 1 hour manual was broken)

@happysalada happysalada merged commit 07b207c into NixOS:master Sep 28, 2022
@happysalada
Copy link
Contributor

There will absolutely be some failures to fix, feel free to update if you find anything odd, happy to help.

@lilyinstarlight lilyinstarlight deleted the fix/node2nix-npmv7 branch September 28, 2022 16:43
@lilyinstarlight
Copy link
Member Author

There will absolutely be some failures to fix, feel free to update if you find anything odd, happy to help.

Thank you!

I noticed some of the aarch64-linux and -darwin builds failed on ofborg (I don't have a Mac) so I was actually looking through those logs now to see if something broke that shouldn't have

I will additionally watch Hydra for new failures in the next evaluation, in case I missed anything and can trace back any new failures to this PR

@winterqt
Copy link
Member

I noticed some of the aarch64-linux and -darwin builds failed on ofborg (I don't have a Mac) so I was actually looking through those logs now to see if something broke that shouldn't have

Skimming one of those logs, it looks like it's just timeouts :)

@CMCDragonkai CMCDragonkai mentioned this pull request Sep 29, 2022
Closed
@lilyinstarlight lilyinstarlight mentioned this pull request Sep 29, 2022
Merged
13 tasks
lilyinstarlight added a commit to lilyinstarlight/nixpkgs that referenced this pull request Sep 29, 2022
@lilyinstarlight
nodePackages: fix builds on x86_64-darwin
2a61392 
Follow-up to NixOS#193337 to fix several x86_64-darwin build failures due to
missing xcrun/xcodebuild
@lilyinstarlight
Copy link
Member Author

I've fixed all of the Hydra regressions due to this PR from https://hydra.nixos.org/eval/1782541#tabs-now-fail in follow-up PR #193533

happysalada pushed a commit that referenced this pull request Sep 30, 2022
@lilyinstarlight
nodePackages: fix builds on x86_64-darwin
dc642a9 
Follow-up to #193337 to fix several x86_64-darwin build failures due to
missing xcrun/xcodebuild
@marsam
Copy link
Contributor

marsam commented Oct 3, 2022

I just noticed this PR. Great work! Thanks a lot for working on this!
If you have a ko-fi or such, drop me a link to buy you a coffee

@happysalada
Copy link
Contributor

I realise I didn't say this properly, but I'm hugely grateful too. Before this lits of npm packages were simply broken because they deprecated the node 14 APIs. Thanks again !

@lilyinstarlight
Copy link
Member Author

I realise I didn't say this properly, but I'm hugely grateful too. Before this lits of npm packages were simply broken because they deprecated the node 14 APIs. Thanks again !

I'm glad I could help! I know having nodePackages/node2nix not working with current Node.js versions was starting to get in my way a bunch for packaging stuff (even if I don't use Node much personally), so I implemented this update when I finally had the time and mental bandwidth to

If you have a ko-fi or such, drop me a link to buy you a coffee

I don't have a ko-fi or anything like that and I don't have a good way to accept money right now, but I do really appreciate the offer! ❤️

@brprice brprice mentioned this pull request Oct 5, 2022
Closed
@lilyinstarlight lilyinstarlight mentioned this pull request Feb 17, 2023
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@jonringer jonringer Awaiting requested review from jonringer

@midchildan midchildan Awaiting requested review from midchildan

@dotlambda dotlambda Awaiting requested review from dotlambda

@jagajaga jagajaga Awaiting requested review from jagajaga

@winterqt winterqt Awaiting requested review from winterqt

@expipiplus1 expipiplus1 Awaiting requested review from expipiplus1

@Luflosi Luflosi Awaiting requested review from Luflosi

@MarcWeber MarcWeber Awaiting requested review from MarcWeber

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: vim 6.topic: vscode 8.has: changelog 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 101-500 10.rebuild-linux: 101-500
Projects
None yet
Milestone
No milestone
5 participants
@lilyinstarlight @happysalada @winterqt @marsam @SuperSandro2000