-
-
Notifications
You must be signed in to change notification settings - Fork 45
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
fix: patchShebangs in nonexecutable bin files (#106) #107
Open
milahu
wants to merge
10
commits into
nix-community:master
Choose a base branch
from
milahu:patch-5
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
98c359b
fix: patchShebangs in nonexecutable bin files (#106)
milahu a306d45
remove cat
milahu a75e8bc
fix jq filter
milahu 48b2eaf
ignore root package, throw on error
milahu 8c9b89b
in function node_modules, ignore the install script for the root package
milahu 40e09df
remove questionmarks from derivation names for mkdir $out
milahu ff6d44a
fix: run install script for the root package
milahu 60b003c
print install commands before running them
milahu de6d2c6
fix jq filter: handle missing scripts field
milahu 99370e3
in mkShell, dont run install scripts for the root package
milahu File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@milahu At least this part of this PR relies on hook scripts, see the
node_modules
buildPhase
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.
aah yes, thanks
so this is part of the
node_modules/.hooks/prepare
scriptwhich makes this PR useless for npmv7
which brings me back to #110 (comment)
→ lets implement our own
npm install
instead of producing workarounds for a npm-without-hooks
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.
I think being able to patch sources is useful on its own, and it's a relatively easy and reasonable workaround for the hook problem. Patching sources is also very well-known by Nix people already.
Implementing an entirely separate
npm install
feels a bit over the top to me, requiring maintenance, the potential for bugs, having to synchronize versions, adding a lot of complexity. If such global hook scripts are really needed for use cases like ours, then I think it's better to talk to npm upstream instead, urging them to add a reasonable replacement. And whether that happens or not, patching of sources will work as a workaround in any case.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.
yes, of course. like
patchPhase
instdenv.mkDerivation
for example,
npm install puppeteer
will download a full chromium binary
to
node_modules/puppeteer/.local-chromium/
in nix, we can say
related: patching json files in place
a working prototype is implemented in my pnpm-install-only
the complexity is limited by
npm install
. its not rocket science:node_modules/.bin
npm run
to run lifecycle scripts. no need to reimplement that part of npmanother bonus: this custom
npm install
allows to use the nix store as a machine-level store for node packages, see my nodejs-hide-symlinksalso that project is a working prototype. its just not popular, because of ... reasons. more human reasons than technical reasons