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.

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

nodejs: switch from 14.x to 16.x to keep up with the lts release #142915

Merged
merged 2 commits into from
Nov 25, 2021

Conversation

bobrik
Copy link
Contributor

@bobrik bobrik commented Oct 26, 2021

Motivation for this change

See: https://nodejs.org/en/about/releases/

It's also the only version that works properly on aarch64-darwin.

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 via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.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
  • Fits CONTRIBUTING.md.

@happysalada
Copy link
Contributor

@marsam you might be interested by this.
I think this is a good idea, however, many packages using the default nodejs might break with this.
Not sure what is the best way to do this. Wait for the 21.11 release then merge ?
Should we try to contact individual nodejs packages maintainers ?
I think the main problem here will be node-packages, If I'm not wrong, node2nix will break with node 16.

@marsam
Copy link
Contributor

marsam commented Oct 26, 2021

Unfortunately, we can't upgrade to 16.x because we are blocked by node2nix #132456

@happysalada
Copy link
Contributor

Could we just use node_14 with all the node2nix packages ?
Just to unblock the others, since it doesn't seem likely that node2nix will receive any major update soon.

@bobrik
Copy link
Contributor Author

bobrik commented Oct 26, 2021

@happysalada I like your suggestion, so I added a commit. Let me know if that's what you had in mind.

@happysalada
Copy link
Contributor

That is what I had in mind.
Doing a brief search, I see that many people in their nixos modules use pkgs.nodejs without specifying the version. Those are likely to break. This should wait until the 21.11 release, but after that, It seems good to me. (I'll have time to think about this, I will update if I find anything).
@billewanick we have to remember to fix the nodejs version for lemmy-ui.

@CMCDragonkai
Copy link
Member

It's been fixed for node2nix: svanderburg/node2nix@e73c616

@happysalada
Copy link
Contributor

We could do an unstable release of node2nix, but I feel it's better to keep what we have and change when node2nix makes a new release.

@bobrik
Copy link
Contributor Author

bobrik commented Nov 24, 2021

#144627 (comment) mentions that the branch off happened already. Is it time to revisit this?

@happysalada
Copy link
Contributor

btw, there is an update in progress of node2nix https://github.com/NixOS/nixpkgs/pull/146440/files
but yeah I think this can potentially be merged to staging. Whenever there is time to rebase and fix the conflict, I personally think this is mergeable, unless anyone sees an objection that I have missed.

@bobrik
Copy link
Contributor Author

bobrik commented Nov 25, 2021

Rebased

@happysalada
Copy link
Contributor

I just noticed, node16 fails to build on aarch64-darwin. @marsam is this normal or is this just a transitive failure ? (no worries if you don't know). Given that nixos supports darwin on tier1, we can't just merge this then ? (let me know if I'm wrong).

@bobrik
Copy link
Contributor Author

bobrik commented Nov 25, 2021

@happysalada
Copy link
Contributor

@bobrik thanks for checking this out. Let's merge then.

@happysalada happysalada merged commit 4c60ee3 into NixOS:staging Nov 25, 2021
@bobrik bobrik deleted the ivan/nodejs-16-lts branch December 4, 2021 03:37
@turboMaCk
Copy link
Member

This PR cause this regression #152745 As far as I know we still lack support for nodejs 16 in node2nix which makes me wonder - What was tested before merge here? I have a feeling there might be more things broken at the moment.

@turboMaCk turboMaCk mentioned this pull request Dec 30, 2021
13 tasks
turboMaCk added a commit to turboMaCk/nixpkgs that referenced this pull request Dec 30, 2021
    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
@happysalada
Copy link
Contributor

I thought the latest version of node2nix supports node 16 (version 1.10). There is a current PR to regenerate and test nodePackages #146440 (still WIP).
Since it was impossible to test all the packages, if I remember correctly nodePackages were tested. We indeed missed elmPackages indeed.
I personally use

      nodePackages.prettier
      nodePackages.pnpm
      nodePackages.node2nix
      nodePackages.npm-check-updates
      nodePackages.svelte-language-server
      nodePackages.yarn

and those work like a charm.

@turboMaCk
Copy link
Member

We indeed missed elmPackages

don't worry about that. It's intentionally designed in a way it doesn't block work on other parts of nixpkgs. I'm happy to take care of it. That's being said I think there might be other packages which rely on expressions produced by node2nix using nodejs argument which might need updating. It's not just nodePackages (which are btw independent of what is defined as nodejs in nixpkgs so I think they don't even use node 16 but I might be mistaken) - there can be applications using node2nix for packaging and using nodejs argument.

Anyway I think we can deal with all the potential issues if there are any.

@turboMaCk
Copy link
Member

To clarify. I think that if intention of this PR was to update nodejs version things defined within nodePackages are using I don't think it accomplished that. This updated nodejs version for everything which uses nodejs attribute directly but I think nodePackages are using explicit attribute name which was updated in #149120 to version 14. Anyway take this with grain of salt - I did not check the code base this is just based on my memories of working with that part of nixpkgs about a year ago.

In order to update nodejs version for things that use node2nix we need to wait for support upstream. Current highest version node2nix supports is 14. However this should update nodejs for derivations that don't depend on node2nix (don't use npm dependecies, just node).

jerith666 pushed a commit to jerith666/nixpkgs that referenced this pull request Jan 2, 2022
    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)
@arcnmx arcnmx mentioned this pull request Jan 29, 2022
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants