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

Hook scripts are removed from npm v7 #110

Open
ilkecan opened this issue Sep 15, 2021 · 11 comments
Open

Hook scripts are removed from npm v7 #110

ilkecan opened this issue Sep 15, 2021 · 11 comments

Comments

@ilkecan
Copy link
Contributor

ilkecan commented Sep 15, 2021

This project currently uses hook scripts (specifically "preinstall" hook) to patch the shebangs. But as far as I can tell, hook scripts are removed from npm v7[1][2]. Do you have any plans to change the way shebangs are patched in a way that could also work with npm v7?

[1] npm/cli#2692
[2] npm/cli#3486

Opened as the continuation of #108 (comment) to make the discussion more visible since that PR is closed.

@ilkecan ilkecan changed the title Hook scripts are deprecated Hook scripts are removed from npm v7 Sep 15, 2021
@andir
Copy link
Collaborator

andir commented Sep 16, 2021

Mhm, that is a bummer. I'll have to think about the alternatives for this. There was always the path of pre-populating a cache directory that NPM would then pick sources from. We could also patch sources before we pass them to npm. The problem with the latter approach is with scripts that requires custom binaries during execution. It also makes it harder to patch files that are being generated on the fly...

@milahu
Copy link
Contributor

milahu commented Sep 16, 2021

i have implemented pnpm install in just 300 lines at npm-install-mini - works for cowsay and nodegui : )
the lockfile parser library also works with yarn.lock, we just have to patch the resolved fields

how is this useful? such a small script is better hackable than a full-feature package manager
also, its easy to support all the lockfiles (npm, yarn, pnpm)

in internal.nix

          #npm install --offline --nodedir=${nodeSource nodejs}
          export NODE_preInstallLinks='${builtins.toJSON preInstallLinks}'
          node --trace-uncaught --trace-warnings ${npm-install-mini-js}

where npm-install-mini-js is the path to dist/index.js

@ilkecan
Copy link
Contributor Author

ilkecan commented Sep 26, 2021

How about something like this

SCRIPT="$(readlink -f ./node_modules/.hooks/preinstall)"
package_dirs="$(find ./node_modules -type f -name "package.json" | xargs --no-run-if-empty dirname)"
while read package_dir; do
  export npm_package_name="$(echo "$package_dir" | awk -F'/' '{ if ($(NF - 1) ~ /^@/) printf "%s/%s\n", $(NF -1), $NF; else print $NF }')"
  pushd $package_dir
  $SCRIPT
  popd
done <<< "$package_dirs"

I want to use --ignore-scripts which also seems to disable hook scripts so I needed a way to manually patch the scripts. I tried this and it seems to be doing the job. Obviously it could be polished.

@milahu
Copy link
Contributor

milahu commented Sep 26, 2021

find ./node_modules -type f -name "package.json"

not all packages have a package.json. this non-standard was introduced by yarn
so this is just 99% perfect ; )

@andir
Copy link
Collaborator

andir commented Sep 26, 2021 via email

@ilkecan
Copy link
Contributor Author

ilkecan commented Sep 26, 2021

I think we could perform

npm ci --offline --ignore-scripts
<patch scripts>

before the current

"npm install --offline --nodedir=${nodeSource nodejs}"

without any side effects, apart from the patched scripts.

To give some context, I was actually trying to package an electron project and I was unable to build the native add-ons for the electron run-time during npm install. I had to do npm rebuild <rest> after npm install but this meant the native code were building twice. To avoid that, I did something similar to above by adding a parameter named installCommands to npmlock2nix.node_modules^1 (similar to buildCommands in npmlock2nix.build) and used it like

https://github.com/ngi-nix/studio/blob/dba76693e720281a656edebbf38bc76000d0c638/nix/node_modules.nix#L40-L56

EDIT: It seems the embedded code snippets don't work across repositories, so

  installCommands = [
    "npm ci --ignore-scripts --offline"
    ''
      SCRIPT="$(readlink -f ./node_modules/.hooks/preinstall)"
      package_dirs="$(find ./node_modules -type f -name "package.json" | xargs --no-run-if-empty dirname)"
      while read package_dir; do
        export npm_package_name="$(echo "$package_dir" | awk -F'/' '{ if ($(NF - 1) ~ /^@/) printf "%s/%s\n", $(NF -1), $NF; else print $NF }')"
        pushd $package_dir
        $SCRIPT
        popd
      done <<< "$package_dirs"
    ''
    ''
      packages_with_gyp="$(find ./node_modules -name "*.gyp" | awk -F'/' '{ if ($3 ~ /^@/) printf "%s/%s\n", $3, $4; else print $3 }' | grep -v "npm" | uniq | tr "\n" " ")"
      npm rebuild $packages_with_gyp
    ''
  ];

@andir
Copy link
Collaborator

andir commented Oct 1, 2021

I just tried something like your proposal on newer npm and I was greeted with:

npm WARN old lockfile The package-lock.json file was created with an old version of npm,
npm WARN old lockfile so supplemental metadata must be fetched from the registry.
npm WARN old lockfile
npm WARN old lockfile This is a one-time fix-up, please be patient...
npm WARN old lockfile

npm ERR! syscall open
npm ERR! path /build/package-lock.json
npm ERR! errno -13
npm ERR! Error: EACCES: permission denied, open '/build/package-lock.json'
npm ERR!  [Error: EACCES: permission denied, open '/build/package-lock.json'] {
npm ERR!   errno: -13,
npm ERR!   code: 'EACCES',
npm ERR!   syscall: 'open',
npm ERR!   path: '/build/package-lock.json'
npm ERR! }
npm ERR!
npm ERR! The operation was rejected by your operating system.
npm ERR! It is likely you do not have the permissions to access this file as the current user
npm ERR!
npm ERR! If you believe this might be a permissions issue, please double-check the
npm ERR! permissions of the file and its containing directories, or try running
npm ERR! the command again as root/Administrator.

npm ERR! A complete log of this run can be found in:
npm ERR!     /build/.npm/_logs/2021-10-01T17_27_06_665Z-debug.log

This obviously doesn't work with a build sandbox....

Fuck NPM. If a lockfile suddenly doesn't have enough information anymore the implementation sucks.

@ilkecan
Copy link
Contributor Author

ilkecan commented Oct 1, 2021

I am sorry, but I am not sure how the error you are getting is related to our discussion.

npm WARN old lockfile The package-lock.json file was created with an old version of npm,
npm WARN old lockfile so supplemental metadata must be fetched from the registry.
npm WARN old lockfile
npm WARN old lockfile This is a one-time fix-up, please be patient...
npm WARN old lockfile

What I understand from this is, you are trying to package a project using npmv7 that has a lockfile created with npmv6. But I think that shouldn't be a use case this project supports. The packager should use the same npm version the upstream uses.

Can you try migrating the lockfile to v7 before using npmlock2nix on the project?

@milahu
Copy link
Contributor

milahu commented Oct 1, 2021

If a lockfile suddenly doesn't have enough information anymore the implementation sucks.

yepp. the lockfile is just a collection of name, version, url, hash
the dependency tree can be generated from all the package.json files.
the magic "supplemental metadata" is probably used to build that depTree faster

Fuck NPM.

while were at it: fuck npm's flat node_modules. every sane system uses symlinks for deduplication

ps: more experimental stuff at https://github.com/milahu/nodejs-hide-symlinks
to make npm more like pnpm + nix = all dependencies are symlinks to the /nix/store

@milahu
Copy link
Contributor

milahu commented Oct 2, 2021

the magic "supplemental metadata" is probably used to build that depTree faster

to defend NPM: this could be a rare edge-case, where npm7 actually installs more packages than npm6:
in npm7, missing peerDependencies are installed.
see peerDependencies and peerDependenciesMeta

the command we want is npm ci (clean install) which will use only the lockfile to resolve deps, or fail.

@andir
Copy link
Collaborator

andir commented Oct 2, 2021

I am sorry, but I am not sure how the error you are getting is related to our discussion.

npm WARN old lockfile The package-lock.json file was created with an old version of npm,
npm WARN old lockfile so supplemental metadata must be fetched from the registry.
npm WARN old lockfile
npm WARN old lockfile This is a one-time fix-up, please be patient...
npm WARN old lockfile

What I understand from this is, you are trying to package a project using npmv7 that has a lockfile created with npmv6. But I think that shouldn't be a use case this project supports. The packager should use the same npm version the upstream uses.

Can you try migrating the lockfile to v7 before using npmlock2nix on the project?

Yeah, so far we didn't have to care much about that. Perhaps that will be a requirement going forward.

I'll try to upgrade one of our examples to npmv7 and test your recommendation with that again...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants