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

fix: nodejs: link direct bins #227

Merged
merged 5 commits into from
Aug 11, 2022
Merged

fix: nodejs: link direct bins #227

merged 5 commits into from
Aug 11, 2022

Conversation

tinybeachthor
Copy link
Contributor

@tinybeachthor tinybeachthor commented Aug 5, 2022 •
edited
Loading

Fixes #221

At the end of the src/subsystems/nodejs/builders/granular/install-deps.py,
read package.json,
and link bins of direct dependencies to node_modules/.bin.

Needs to pass the src (+ run unpack phase) to devShell so package.json is available.

@tinybeachthor tinybeachthor marked this pull request as ready for review August 5, 2022 20:57
@tinybeachthor tinybeachthor force-pushed the fix-nodejs-link-direct-bins branch from ab629a3 to 5adc175 Compare August 6, 2022 16:26
@tinybeachthor tinybeachthor force-pushed the fix-nodejs-link-direct-bins branch from 4fb8366 to d3beb8b Compare August 6, 2022 16:29
tinybeachthor
tinybeachthor commented Aug 8, 2022
View reviewed changes
src/subsystems/nodejs/builders/granular/default.nix
Comment on lines -171 to -199
# Derivation building the ./node_modules directory in isolation.
# This is used for the devShell of the current package.
# We do not want to build the full package for the devShell.
nodeModulesDir = pkgs.runCommandLocal "node_modules-${pname}" {} ''
# symlink direct dependencies to ./node_modules
mkdir $out
${l.concatStringsSep "\n" (
l.forEach nodeDeps
(pkg: ''
for dir in $(ls ${pkg}/lib/node_modules/); do
if [[ $dir == @* ]]; then
mkdir -p $out/$dir
ln -s ${pkg}/lib/node_modules/$dir/* $out/$dir/
else
ln -s ${pkg}/lib/node_modules/$dir $out/
fi
done
'')
)}

# symlink transitive executables to ./node_modules/.bin
mkdir $out/.bin
for dep in ${l.toString nodeDeps}; do
for binDir in $(ls -d $dep/lib/node_modules/.bin 2>/dev/null ||:); do
ln -sf $binDir/* $out/.bin/
done
done
'';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unused, removing

src/subsystems/nodejs/builders/granular/devShell.nix
Comment on lines -42 to -43
src = "";
dontUnpack = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to read package.json to see what the direct dependencies are, to give priority to linking their bins.

src/subsystems/nodejs/builders/granular/install-deps.py
@@ -6,7 +6,6 @@
import sys


out = os.environ.get('out')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unused, removing

src/subsystems/nodejs/builders/granular/install-deps.py
Comment on lines -49 to +45
os.symlink(origin, f"{root}/{module}/{submodule}")
if not os.path.exists(f"{root}/{module}/{submodule}"):
os.symlink(origin, f"{root}/{module}/{submodule}")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

could crash the build if the symlink already exists

src/subsystems/nodejs/builders/granular/install-deps.py
@@ -26,15 +25,11 @@ def get_package_json(path):
return package_json_cache[path]

def install_direct_dependencies():
add_to_bin_path = []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unused, removing

src/subsystems/nodejs/builders/granular/install-deps.py
Comment on lines +114 to +115
if force and os.path.exists(source):
os.remove(source)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

using force=True, overwrite symlinks with direct dep bins

src/subsystems/nodejs/builders/granular/install-deps.py
Comment on lines +183 to +198
def symlink_direct_bins():
deps = []
package_json_file = get_package_json(f"{os.path.abspath('.')}")

if package_json_file:
if 'devDependencies' in package_json_file and package_json_file['devDependencies']:
for dep,_ in package_json_file['devDependencies'].items():
deps.append(dep)
if 'dependencies' in package_json_file and package_json_file['dependencies']:
for dep,_ in package_json_file['dependencies'].items():
deps.append(dep)

for name in deps:
package_location = f"{root}/{name}"
package_json = get_package_json(package_location)
symlink_bin(f"{bin_dir}", package_location, package_json, force=True)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Read package.json, get deps, link their bins, overwriting existing

src/subsystems/nodejs/builders/granular/install-deps.py
Comment on lines -190 to -192
# dump bin paths
with open(f"{os.environ.get('TMP')}/ADD_BIN_PATH", 'w') as f:
f.write(':'.join(add_to_bin_path))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unused, removing

@wmertens
Copy link
Contributor

wmertens commented Aug 8, 2022

You might be interested in #195 - there the node_modules are always trees of symlinks and generated separately from the packages (makeModules function), and the .bin directories are correctly populated (I think).

individual node modules have bin/ so that they work as nix packages and makeWrapper etc work as expected, and the combined node_modules then symlinks those together into node_modules/.bin/

@DavHau
Copy link
Member

DavHau commented Aug 9, 2022

I could reproduce the problem using your package.json from #221.
But even after switching to this PR, the problem is still not solved.

I used this flake.nix:

{
  inputs.dream2nix.url = "github:tinybeachthor/dream2nix/fix-nodejs-link-direct-bins";
  outputs = inp: let
    pkgs = inp.dream2nix.inputs.nixpkgs.legacyPackages.x86_64-linux;
    package_json = builtins.toFile "package.json" ''
      {
        "devDependencies": {
          "svgo": "^2.2.2",
          "cssnano": "^4.1.10"
        }
      }
    '';
  in
    inp.dream2nix.lib.makeFlakeOutputs {
      systemsFromFile = ./nix_systems;
      config.projectRoot = ./.;
      source = pkgs.runCommand "src" {} ''
        mkdir $out
        cp ${package_json} $out/package.json
      '';
    };
}

after running .#resolveImpure, nix develop still produces node_modules like this:

$ ls -lah node_modules/.bin/
total 0
drwxr-xr-x   2 grmpf users  200 Aug  9 15:10 .
drwxr-xr-x 162 grmpf users 3.2K Aug  9 15:10 ..
lrwxrwxrwx   1 grmpf users   85 Aug  9 15:10 browserslist -> ../gv0wz6r2k2i2cgmrmdip0az1al52x5sj-cssnano-4.1.11/lib/node_modules/.bin/browserslist
lrwxrwxrwx   1 grmpf users   90 Aug  9 15:10 browserslist-lint -> ../gv0wz6r2k2i2cgmrmdip0az1al52x5sj-cssnano-4.1.11/lib/node_modules/.bin/browserslist-lint
lrwxrwxrwx   1 grmpf users   79 Aug  9 15:10 cssesc -> ../gv0wz6r2k2i2cgmrmdip0az1al52x5sj-cssnano-4.1.11/lib/node_modules/.bin/cssesc
lrwxrwxrwx   1 grmpf users   80 Aug  9 15:10 esparse -> ../gv0wz6r2k2i2cgmrmdip0az1al52x5sj-cssnano-4.1.11/lib/node_modules/.bin/esparse
lrwxrwxrwx   1 grmpf users   83 Aug  9 15:10 esvalidate -> ../gv0wz6r2k2i2cgmrmdip0az1al52x5sj-cssnano-4.1.11/lib/node_modules/.bin/esvalidate
lrwxrwxrwx   1 grmpf users   80 Aug  9 15:10 js-yaml -> ../gv0wz6r2k2i2cgmrmdip0az1al52x5sj-cssnano-4.1.11/lib/node_modules/.bin/js-yaml
lrwxrwxrwx   1 grmpf users   79 Aug  9 15:10 mkdirp -> ../gv0wz6r2k2i2cgmrmdip0az1al52x5sj-cssnano-4.1.11/lib/node_modules/.bin/mkdirp
lrwxrwxrwx   1 grmpf users   77 Aug  9 15:10 svgo -> ../gv0wz6r2k2i2cgmrmdip0az1al52x5sj-cssnano-4.1.11/lib/node_modules/.bin/svgo

Thesvgo bin still links to cssnano. Also the symlink seems to be broken which is probably another unrelated bug.

@tinybeachthor
Copy link
Contributor Author

@DavHau
Adding a name field to the package_json makes this build as intended.
Name is a required field for npm packages.

{
  inputs.dream2nix.url = "github:tinybeachthor/dream2nix/fix-nodejs-link-direct-bins";
  outputs = inp: let
    pkgs = inp.dream2nix.inputs.nixpkgs.legacyPackages.x86_64-linux;
    package_json = builtins.toFile "package.json" ''
      {
        "name": "anything",
        "devDependencies": {
          "svgo": "^2.2.2",
          "cssnano": "^4.1.10"
        }
      }
    '';
  in
    inp.dream2nix.lib.makeFlakeOutputs {
      systemsFromFile = ./nix_systems;
      config.projectRoot = ./.;
      source = pkgs.runCommand "src" {} ''
        mkdir $out
        cp ${package_json} $out/package.json
      '';
    };
}
> ls -la node_modules/.bin
total 32
drwxr-xr-x 1 martin users  138 Aug  9 11:22 .
drwxr-xr-x 1 martin users 4590 Aug  9 11:22 ..
lrwxrwxrwx 1 martin users   22 Aug  9 11:22 browserslist -> ../browserslist/cli.js
lrwxrwxrwx 1 martin users   32 Aug  9 11:22 browserslist-lint -> ../update-browserslist-db/cli.js
lrwxrwxrwx 1 martin users   20 Aug  9 11:22 cssesc -> ../cssesc/bin/cssesc
lrwxrwxrwx 1 martin users   25 Aug  9 11:22 esparse -> ../esprima/bin/esparse.js
lrwxrwxrwx 1 martin users   28 Aug  9 11:22 esvalidate -> ../esprima/bin/esvalidate.js
lrwxrwxrwx 1 martin users   25 Aug  9 11:22 js-yaml -> ../js-yaml/bin/js-yaml.js
lrwxrwxrwx 1 martin users   20 Aug  9 11:22 mkdirp -> ../mkdirp/bin/cmd.js
lrwxrwxrwx 1 martin users   16 Aug  9 11:22 svgo -> ../svgo/bin/svgo

I am not sure why adding a name to package.json makes a difference,
but I would expect the behavior to be consistent regardless of the name field.

@tinybeachthor tinybeachthor mentioned this pull request Aug 9, 2022
Closed
@wmertens
Copy link
Contributor

wmertens commented Aug 9, 2022

The name thing is because the discoverer gets a null name. I have a fix for it in my branch

@DavHau DavHau merged commit 4a92c17 into nix-community:main Aug 11, 2022
@DavHau DavHau added the summer-of-nix Issues/PRs opened by SoN participants label Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
summer-of-nix Issues/PRs opened by SoN participants
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Node: transitive dependency bin taking priority over direct dependency
3 participants
@tinybeachthor @wmertens @DavHau