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: use python3 if possible #84599

Merged
merged 2 commits into from Apr 13, 2020
Merged

Conversation

doronbehar
Copy link
Contributor

Motivation for this change
  1. Python 2 is EOL so I think we should use python3 by default for the nodejs versions that support it.
  2. Allow choosing a nodejs build flavor / version with the programs.npm. nixos config.

cc @cillianderoiste @gilligan @cko

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Copy link
Contributor

@marsam marsam left a comment

Choose a reason for hiding this comment

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

LGTM. I think this can go directly to master

@doronbehar doronbehar changed the base branch from staging to master April 13, 2020 07:45
@doronbehar
Copy link
Contributor Author

Hmm interesting it doesn't trigger a rebuild of all nodePackages. Thanks for your approval @marsam.

@marsam marsam merged commit 66e43c6 into NixOS:master Apr 13, 2020
@calbrecht
Copy link
Member

I have declared nodePackages = pkgs.nodePackages_13_x in my overlays.
There is no npm in nodePackages_13_x.
This is inconvenient, especially if i don't want to have npm in my nodePackages at all.

tutnix ~ 
% nixos-rebuild test
building Nix...
building the system configuration...
error: attribute 'npm' missing, at /home/alab/ws/nixpkgs/nixos/modules/programs/npm.nix:19:19
(use '--show-trace' to show detailed location information)
tutnix ~ 
% nix-build \<nixpkgs\> --no-out-link --no-build-output -A nodePackages.npm
error: attribute 'npm' in selection path 'nodePackages.npm' not found
tutnix ~ 
% nix-build \<nixpkgs\> --no-out-link --no-build-output -A nodePackages_10_x.npm
/nix/store/6mkiyvgphqahlzvj6w4f95qpal5jp1gd-node_npm-6.14.4
tutnix ~ 
% nix-build \<nixpkgs\> --no-out-link --no-build-output -A nodePackages_12_x.npm
error: attribute 'npm' in selection path 'nodePackages_12_x.npm' not found
tutnix ~ 
% nix-build \<nixpkgs\> --no-out-link --no-build-output -A nodePackages_13_x.npm
error: attribute 'npm' in selection path 'nodePackages_13_x.npm' not found

@calbrecht
Copy link
Member

calbrecht commented Apr 19, 2020

@doronbehar maybe you could change this to

diff --git a/nixos/modules/programs/npm.nix b/nixos/modules/programs/npm.nix
index f101a44587a..52810f526cd 100644
--- a/nixos/modules/programs/npm.nix
+++ b/nixos/modules/programs/npm.nix
@@ -14,9 +14,9 @@ in
       enable = mkEnableOption "<command>npm</command> global config";
 
       package = mkOption {
-        type = types.path;
+        type = types.path or null;
         description = "The npm package version / flavor to use";
-        default = pkgs.nodePackages.npm;
+        default = if hasAttr "npm" pkgs.nodePackages then pkgs.nodePackages.npm else null;
         example = literalExample "pkgs.nodePackages_13_x.npm";
       };

or add npm to both

pkgs/development/node-packages/node-packages-v12.json
pkgs/development/node-packages/node-packages-v13.json

and update with pkgs/development/node-packages/generate.sh
And while you are at it you could also add the missing standard gyp tools

  "node-gyp",
  "node-gyp-build",
  "node-pre-gyp",

@doronbehar
Copy link
Contributor Author

Can't you use:

    programs.npm.package = pkgs.nodejs_latest;

?

This is inconvenient, especially if i don't want to have npm in my nodePackages at all.

I'm not sure I understand what kind of ability does the current module is missing. If you feel there are missing attributes to some of the nodePackages* attribute sets, then please fix it in a different PR. This one is merged & done. I'm afraid I don't have the necessary time to do apply your suggestions my self or review them once made. Then please don't try to request my review 🙏 . Thanks.

@calbrecht
Copy link
Member

I could :) The point is i do not use programs.npm so why should i need to set this?

calbrecht added a commit to calbrecht/nixpkgs that referenced this pull request Apr 19, 2020
MR NixOS#84599 introduced an option 
that breaks rebuilding nixos in case that user has set nodePackages to a different
version than nodePackages_10_x. The package attribute npm is not part of any other
nodePackages version than version 10.

This commit makes sure nixos may be build by users who st their nodePackages to a different
version and do not want to have programs npm enabled.
@doronbehar doronbehar deleted the nodejs-python3 branch March 2, 2023 10:39
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.

None yet

3 participants