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: move to nodejs-14_x, regenerate #149120

Merged
merged 3 commits into from
Dec 9, 2021
Merged

nodePackages: move to nodejs-14_x, regenerate #149120

merged 3 commits into from
Dec 9, 2021

Conversation

mweinelt
Copy link
Member

@mweinelt mweinelt commented Dec 6, 2021

Motivation for this change

We should probably try to track the active LTS release of NodeJS for the package set, if that works.

If not, then we should at least move to nodejs 14.x, since 12.x will be EOL before the 22.05 release.

I added an optioniated reformatting commit for generate.sh, but that's totally optional and I don't mind if we don't want to reformat things unnecesarily.

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.05 Release Notes (or backporting 21.11 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.

@ofborg ofborg bot requested a review from midchildan December 6, 2021 20:31
@mweinelt
Copy link
Member Author

mweinelt commented Dec 6, 2021 •
edited
Loading

Result of nixpkgs-review pr 149120 run on x86_64-linux 1

1 package marked as broken and skipped:
  • hyperspace-cli
1 packages failed to build:
  • vscode-extensions.matklad.rust-analyzer
46 packages built:
  • antora
  • balanceofsatoshis
  • bat-extras.prettybat
  • commitlint
  • create-cycle-app
  • deltachat-desktop
  • discourse
  • discourseAllPlugins
  • epgstation
  • fast-cli
  • gtop
  • iosevka
  • joplin
  • lumo
  • pm2
  • postcss-cli
  • pyright
  • python38Packages.batchspawner
  • python38Packages.dockerspawner
  • python38Packages.jupyterhub
  • python38Packages.jupyterhub-ldapauthenticator
  • python38Packages.jupyterhub-systemdspawner
  • python38Packages.jupyterhub-tmpauthenticator
  • python39Packages.batchspawner
  • python39Packages.dockerspawner
  • python39Packages.jupyterhub
  • python39Packages.jupyterhub-ldapauthenticator
  • python39Packages.jupyterhub-systemdspawner
  • python39Packages.jupyterhub-tmpauthenticator
  • python38Packages.oauthenticator
  • python39Packages.oauthenticator
  • redoc-cli
  • shepherd
  • sloc
  • teck-programmer
  • thelounge
  • vimPlugins.coc-diagnostic
  • vimPlugins.coc-metals
  • vimPlugins.coc-prettier
  • vimPlugins.coc-pyright
  • vimPlugins.coc-stylelint
  • vimPlugins.coc-vetur
  • vscode-extensions.ms-python.vscode-pylance
  • vscode-extensions.vadimcn.vscode-lldb
  • yaml-language-server
  • zerobin

@Mic92
Copy link
Member

Mic92 commented Dec 6, 2021

Logs for rust-analyzer: https://p.krebsco.de/0nhf2a6
cc @matklad @oxalica

@mweinelt
Copy link
Member Author

mweinelt commented Dec 6, 2021

After 6-7 nixpkgs-review runs I got rid of all flaky build errors. Just wow.

Anyway, rust-analyzer is already broken on master, so not a blocker for this PR.

@DeeUnderscore
Copy link
Contributor

#142915 (currently in staging) moved nodejs to Node 16, but had to override nodePackages to use 14, since node2nix output is broken with 16 right now. Setting the default to Node 16 might be less useful than 14, at least while #132456 is unresolved.

@matklad
Copy link
Member

matklad commented Dec 7, 2021

rust-analyzer should be build by the same node that VS Code is using and that if I am not mistaken is still nodejs-14. So, rather than upgrading, it makes sense to add a comment to that effect instead.

@mweinelt
Copy link
Member Author

mweinelt commented Dec 7, 2021 •
edited
Loading

#142915 (currently in staging) moved nodejs to Node 16, but had to override nodePackages to use 14, since node2nix output is broken with 16 right now. Setting the default to Node 16 might be less useful than 14, at least while #132456 is unresolved.

Hmm, nodePackages.svgo doesn't build when I regenerate nodePackages with nodejs-14 either. What's the relationship here?

@oxalica
Copy link
Contributor

oxalica commented Dec 7, 2021

I fix the rust-analyzer build failure in oxalica@3a85e13 . Please cherry-pick into this PR.

I seems more like a workaround since it's actually keytar, a transitive dependency of vsce, fails to compile some C code. The issue would affect all packages have keytar in their closure. I'm not sure how to mark these dependencies on keytar directly. It's deep in the closure and could not be simply overrided in pkgs/development/node-packages/default.nix.

@mweinelt
Copy link
Member Author

mweinelt commented Dec 7, 2021

Please cherry-pick into this PR.

The build is already broken on master, so it would be better to create a separate PR.

@DeeUnderscore
Copy link
Contributor

Hmm, nodePackages.svgo doesn't build when I regenerate nodePackages with nodejs-14 either. What's the relationship here?

It seems that changes to node2nix are ultimately needed so that it outputs package lock files in the format expected by Node 16. Right now it outputs the older format, so packages can only be built with 14.

@oxalica
Copy link
Contributor

oxalica commented Dec 8, 2021

Please cherry-pick into this PR.

The build is already broken on master, so it would be better to create a separate PR.

Does it? I checked 5753b89 and vscode-extensions.matklad.rust-analyzer is even available from cache.

mweinelt and others added 3 commits December 8, 2021 11:42
@mweinelt
nodePackages: move to nodejs-14_x
621163d 
NodeJS 12.x will not be supported when we release 22.05.

https://nodejs.org/en/about/releases/
@mweinelt
nodePackages: reformat generate.sh
e6368af 
The shell script felt somewhat crammed, with commands and comments on
every line. Grouping commands and comments and giving them some space
feels like an improvement to me.
@oxalica @mweinelt
vscode-extensions.matklad.rust-analyzer: fix build
a6d04cc
@mweinelt mweinelt force-pushed the nodepackages-nodejs-16_x branch from 952f287 to a6d04cc Compare December 8, 2021 10:47
@mweinelt mweinelt requested a review from jonringer as a code owner December 8, 2021 10:47
@mweinelt
Copy link
Member Author

mweinelt commented Dec 8, 2021

Regenerated with nodejs-14_x.

@mweinelt mweinelt changed the title nodePackages: move to nodejs-16_x, regenerate nodePackages: move to nodejs-14_x, regenerate Dec 8, 2021
@ofborg ofborg bot requested a review from oxalica December 8, 2021 11:00
@Mic92 Mic92 merged commit 2c3b3e6 into NixOS:master Dec 9, 2021
@Mic92 Mic92 mentioned this pull request Dec 9, 2021
Merged
13 tasks
@mweinelt mweinelt deleted the nodepackages-nodejs-16_x branch December 9, 2021 14:16
@mstone mstone mentioned this pull request Dec 26, 2021
Closed
@turboMaCk turboMaCk mentioned this pull request Dec 30, 2021
Closed
turboMaCk added a commit to turboMaCk/nixpkgs that referenced this pull request Dec 30, 2021
@turboMaCk
elmPackages: fix nodejs packages
26b74d2 
    Fixes regression caused by
    4c60ee3 (pull: NixOS#142915)
    following patch of nodePackages using nodejs-14_x
    2c3b3e6 (pull: NixOS#149120)

    - clenups and updates in generate-node-packages.sh
    - specify nodejs version in default.nix

    This makes elmPackages.* build with nodejs-14
    which resolves the issue with npm installation failing
@turboMaCk turboMaCk mentioned this pull request Dec 30, 2021
Merged
12 tasks
jerith666 pushed a commit to jerith666/nixpkgs that referenced this pull request Jan 2, 2022
@turboMaCk @jerith666
elmPackages: fix nodejs packages
122a040 
    Fixes regression caused by
    4c60ee3 (pull: NixOS#142915)
    following patch of nodePackages using nodejs-14_x
    2c3b3e6 (pull: NixOS#149120)

    - clenups and updates in generate-node-packages.sh
    - specify nodejs version in default.nix

    This makes elmPackages.* build with nodejs-14
    which resolves the issue with npm installation failing

(cherry picked from commit 26b74d2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@midchildan midchildan Awaiting requested review from midchildan

@jonringer jonringer Awaiting requested review from jonringer

@oxalica oxalica Awaiting requested review from oxalica

Assignees
No one assigned
Labels
10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mweinelt @Mic92 @DeeUnderscore @matklad @oxalica