-
Notifications
You must be signed in to change notification settings - Fork 100
Patch shebangs in dependency binaries #266
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
base: master
Are you sure you want to change the base?
Conversation
| , dontNpmInstall ? false | ||
| , bypassCache ? false | ||
| , reconstructLock ? false | ||
| , preRebuild ? "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this parameter removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh? It was missing before and it looked like an oversight so I added it. I think this will resolve #226.
| # Change to the package directory to install dependencies | ||
| cd "$DIR/$packageName" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if this can be rewritten in JavaScript code and executed by Node.js. This is what we do in other pieces of the script as well.
Otherwise, we need to introduce yet another build-time dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I like processing things with jq this way because it's so fast. If you consider the amount of NPM packages that need to get downloaded on a typical build, downloading a copy of jq seems like the least of one's problems :P
|
I've implemented my own implementation that does not rely on I'm still validating whether this does not introduce new problems. |
|
I was able to package my npm-based piece of software only with this patch. Would be great to have this merged! |
|
Just wanted to check @svanderburg, is this fixed now? I'll close the PR if so. |
I needed to do this so that a call to
node-gyp-buildwould work in one of my dependencies.Sometimes a dependency comes with an executable script, which ultimately gets symlinked into
node_modules/.bin. The existingpatchShebangscall at the beginning ofprepareAndInvokeNPMdoesn't always catch these, because they don't always have the executable bit set. As a result, you get a failure in the Nix build because/usr/bin/env nodedoesn't exist.So, this PR uses
jqto traverse thebinfield ofpackage.json, turning these files executable and patching them as necessary.This PR also exposes the
preRebuildhook forbuildNodeDependencies, and by extension for theshellattribute. I think this will resolve #226.