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

Patch the shebang of files specifed with bin field #108

Conversation

ilkecan
Copy link
Contributor

@ilkecan ilkecan commented Sep 14, 2021

Sometimes files specified with the bin field of package.json are missing
their executable bits when they are extracted[1]. npm must be setting their
executable bits at some stage of npm install life cycle because by the
time the command is complete, they are executable. But at "preinstall"
stage, which is when we patch the shebangs, those files are not
executable thus skipped by the patchShebangs function.

To fix this, jq is used to get the files specified with the "bin" field of
"package.json and the ones missing their executable bit are chmod'ed and patched.

[1]: E.g., package/bin.js extracted from https://registry.npmjs.org/prebuild-install/-/prebuild-install-6.0.1.tgz has its executable bit correctly set while the one from doesn't https://registry.npmjs.org/prebuild-install/-/prebuild-install-6.1.4.tgz

Sometimes files specified with the bin field of package.json are missing
their executable bits when they are extracted. npm must be setting their
executable bits at some stage of `npm install` life cycle because by the
time the command is complete, they are executable. But at "preinstall"
stage, which is when we patch the shebangs, those files are not
executable thus skipped by the `patchShebangs` function.

We are now using `jq` to get the files specified with the "bin" field of
"package.json and patches the ones missing their executable bit.
@ilkecan
Copy link
Contributor Author

ilkecan commented Sep 14, 2021

While trying to package https://github.com/eez-open/studio, using this commit patches 7 extra files, where at least one of those are used during the install. Without this commit, unpatched shebangs produces an error similar to
sh: /build/node_modules/better-sqlite3/node_modules/.bin/prebuild-install: /usr/bin/env: bad interpreter: No such file or directory.

@ilkecan
Copy link
Contributor Author

ilkecan commented Sep 14, 2021

Also somewhat related, currently this project 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

@ilkecan
Copy link
Contributor Author

ilkecan commented Sep 14, 2021

Wow, I didn't see #107

@milahu
Copy link
Contributor

milahu commented Sep 15, 2021

hook scripts are removed from npm v7

good to know!

we can run the lifecycle scripts manually

npmlock2nix/internal.nix

Lines 288 to 302 in de6d2c6

runInstallScriptsForRootPackage = ''
# https://docs.npmjs.com/cli/v7/using-npm/scripts#npm-install
allScripts=$(jq -r 'select(.scripts != null) | .scripts | keys[]' package.json)
runScripts=""
for script in preinstall install postinstall prepublish preprepare prepare postprepare; do
if ( echo "$allScripts" | grep "^$script$" >/dev/null ); then
runScripts+=" $script"
fi
done
if [ ! -z "$runScripts" ]; then
echo "run install scripts for root package:"
for script in $runScripts; do echo " npm run $script"; done # make easier to copy-paste
for script in $runScripts; do npm run $script || break; done
fi
'';

while were at it, we could fully replace npm install with bash scripts : P
(and use the deep folder structure with symlinks, like pnpm)

(and of course, merge all the npm2nix/yarn2nix projects into one)

@milahu
Copy link
Contributor

milahu commented Sep 15, 2021

while were at it, we could fully replace npm install with bash scripts : P

much easier in javascript

https://github.com/milahu/npm-install-mini
forked from the lockfile parser https://github.com/npm/logical-tree

i use the same folder structure like pnpm (deep node_modules with symlinks),
but my "store" is in node_modules/.pnpm, not in $HOME/.pnpm-store

$ diff -rq node_modules.pnpm node_modules | grep ^Only

Only in node_modules.pnpm: .modules.yaml
Only in node_modules.pnpm/.pnpm: lock.yaml
Only in node_modules.pnpm/.pnpm: node_modules

works at least with cowsay ^^

$ npx cowsay "mooh!" -r
 _______
< mooh! >
 -------
 \
  \
 _   /|
 \'o.O'
 =(___)=
    U

@ilkecan
Copy link
Contributor Author

ilkecan commented Sep 15, 2021

@milahu I think GitHub stops sending notifications when a PR is closed. I read your comments by chance because this tab was left open. (I must have missed the notification email from GitHub) I opened an issue regarding the removal of hook scripts just now, to make it more visible. It was quite unlikely the maintainers would read these messages.

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

Successfully merging this pull request may close these issues.

2 participants