fix: patchShebangs in nonexecutable bin files (#106) #107

milahu · 2021-09-14T13:31:59Z

close #106

andir · 2021-09-14T13:58:21Z

internal.nix

@@ -331,6 +331,9 @@ rec {

              ${preInstallLinkCommands}

+              cat package.json | jq -r '. | select(.bin != null) | .bin | values[]' | while read binTarget; do


Is the package.json guranteed to be there?

We could also pass the package.json as 2nd argument to cat and avoid the cat invocation.

Is the package.json guranteed to be there?

this is called for every npm package, these are required to have a package.json

We could also pass the package.json as 2nd argument to jq

its prettier this way ; ) but yes, lets save some nanoseconds and call only jq

Is the package.json guranteed to be there?

package.json *should* always be there
and if its missing, we should throw a fatal error

currently, the script continues and throws later

i managed to hack together a broken node_modules with missing submodules ...
in that case i got

> [email protected] preinstall /build/node_modules > /build/node_modules/.hooks/preinstall jq: error: Could not open file package.json: No such file or directory patching script interpreter paths in .

but package.json should be there

$ tar tf /nix/store/gll14kwxsz9ggfhngafafh64rv4a02fr-semver-5.7.1.tgz package/LICENSE package/bin/semver package/range.bnf package/semver.js package/package.json package/CHANGELOG.md package/README.md

I also encountered a similar problem, where jq was complaining there was no "package.json" even though it was fine accesing it with cat and ls when I was working on #108. I think using cat <file> | jq ... was mitigating the problem but then I stopped encountering it after fixing a some other part of the bash code. I still have no idea why that happened/happens.

package.json should always be there
and if its missing, we should throw a fatal error

no. there are packages without a package.json,
for example type definitions (*.d.ts) or header files (*.h)

pnpm/pnpm#3728

ilkecan · 2021-09-14T15:29:52Z

internal.nix

@@ -331,6 +331,9 @@ rec {

              ${preInstallLinkCommands}

+              jq -r '. | select(.bin != null) | .bin | values[]' package.json | while read binTarget; do


bin field could also have string as the value if there is a single executable1

{ "name": "my-program", "version": "1.2.5", "bin": "./path/to/program" }

Currently, this jq expression doesn't seem to handle that case. I used something like 'select(has("bin")) | .bin | if type == "string" then . else .[] end' to handle that case. But this is the first time I wrote jq expression, so it might be far from the optimal.

I am not really into maintaining jq expression.. I've done it once or twice but generally that isn't something that i want to maintain beyond a certain level of complexity.

How much effort would it be to rewrite this in a short nodejs script (without dependencies)? The conversaion so far makes me fear about any additional special cases that we will have to add in the future.

jq is a standard tool for json manipulation, node feels like overkill for this

The conversaion so far makes me fear about any additional special cases that we will have to add in the future.

heh, not really ... there are three cases

the bin field ...

does not exist

is a string

is an object

all of these are handled

https://docs.npmjs.com/cli/v7/configuring-npm/package-json#bin

milahu · 2021-09-14T16:28:06Z

internal.nix

@@ -331,6 +331,9 @@ rec {

              ${preInstallLinkCommands}

+              jq -r 'select(.bin != null) | .bin | if type == "string" then . else values[] end' package.json | while read binTarget; do
+                chmod +x "$binTarget" # patchShebangs will only patch executable files


maybe this should throw if binTarget is not found

in my case:

npmlock2nix.shell { src = /tmp/package; }

the root package.json (/tmp/package/package.json) has a bin,
but npmlock2nix will copy only the *.json files to /build, so binTarget is not found

or rather, npmlock2nix should ignore bin's for the root package (/tmp/package)

npmlock2nix should ignore bin's for the root package (/tmp/package)

even more, npmlock2nix.node_modules should ignore install scripts for the root package,
since for the root package, only the *.json files are available,
and install scripts may require other files

Is that so? I am not entirely sure that is true. Right now we do an npm install in the node_modules build and that does what we need (most of the time?).

consider this package.json

{ "name": "root-package", "version": "1", "scripts": { "postinstall": "node some/script.js" }, "dependencies": { "foo": "1.2.3" } }

npmlock2nix has only two files for the root-package: package.json and package-lock.json

these are symlinked to /build in npmlock2nix.node_modules

npmlock2nix/internal.nix

Lines 365 to 368 in 33eb330

postPatch = ''

ln -sf ${patchedLockfile (sourceHashFunc githubSourceHashMap) packageLockJson} package-lock.json

ln -sf ${patchedPackagefile (sourceHashFunc githubSourceHashMap) packageJson} package.json

'';

so some/script.js is NOT available in /build, and npm install would fail

the job of npmlock2nix.node_modules is only to populate node_modules

internal.nix

milahu · 2021-09-14T20:27:51Z

internal.nix

@@ -283,6 +285,22 @@ rec {
    else
      throw "sourceHashFunc: spec.type '${spec.type}' is not supported. Supported types: 'github'";

+  runInstallScriptsForRootPackage = ''


we could move this to a bash function, similar to buildPhase in stdenv.mkDerivation

avoid unnecessary or unwanted rebuilds

infinisil · 2022-03-18T20:05:52Z

With NPM 7, hook scripts, which this PR relies on, are not supported anymore, see also #110. However with #151 it will become possible to patch the sources of arbitrary packages, which should cover mostly the same use cases as hook scripts. @milahu Could you try out #151 and see if that can be used for your use cases?

milahu · 2022-03-19T09:04:37Z

Could you try out #151 and see if that can be used for your use cases?

naah, thats too general
my life is too short to add postPatch = "patchShebangs bin"; to every broken package

chmod +x and patchShebangs should run automatically on all the bin entries of a package

milahu · 2022-03-19T09:29:58Z

With NPM 7, hook scripts, which this PR relies on, are not supported anymore

i guess you're confusing ...

lifecycle scripts in package.json: preinstall install postinstall prepublish preprepare prepare postprepare
hook script files in node_modules/.hooks, for example node_modules/.hooks/prepare
- these are no longer supported in npmv7

infinisil · 2022-03-19T12:01:19Z

internal.nix

+              # patchShebangs will only patch executable files
+              if [[ "$(pwd)" != "/build" ]]; then # ignore the root package. bin entries only make sense for dependencies
+                jq -r 'select(.bin != null) | .bin | if type == "string" then . else values[] end' package.json | while read binTarget; do
+                  if [[ ! -x "$binTarget" ]]; then
+                    echo "make binary executable: $(pwd)/$binTarget"
+                    chmod +x "$binTarget" || exit 1 # on error, throw ELIFECYCLE
+                  fi
+                done
+              fi


@milahu At least this part of this PR relies on hook scripts, see the node_modules buildPhase

aah yes, thanks
so this is part of the node_modules/.hooks/prepare script
which 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

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.

I think being able to patch sources is useful on its own

yes, of course. like patchPhase in stdenv.mkDerivation

for example, npm install puppeteer
will download a full chromium binary
to node_modules/puppeteer/.local-chromium/

in nix, we can say

{ pname = "puppeteer"; patchPhase = '' <package.json jq '.scripts.install = ""' | sponge package.json ''; postInstall = '' d=$out/node_modules/puppeteer/.local-chromium/linux-970485 mkdir -p $d ln -s ${puppeteer-chromium} $d/chrome-linux '';

related: patching json files in place

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.

a working prototype is implemented in my pnpm-install-only

the complexity is limited by

focussing only on npm install. its not rocket science:

verify tar.gz files

unpack tar.gz files

run lifecycle scripts: must be "bottom up", so that child-dependencies are installed

populate node_modules/.bin

using an external library (snyk-nodejs-lockfile-parser) to parse lockfiles. bonus: this supports different lockfile formats: npm-shrinkwrap.json package-lock.json yarn.lock. pnpm-lock.yaml is supported only via workaround, but it "should" be easy to implement. (seems everybody is a frontend dev these days ...)

using npm run to run lifecycle scripts. no need to reimplement that part of npm

another bonus: this custom npm install allows to use the nix store as a machine-level store for node packages, see my nodejs-hide-symlinks

also that project is a working prototype. its just not popular, because of ... reasons. more human reasons than technical reasons

fix: patchShebangs in nonexecutable bin files (nix-community#106)

98c359b

andir reviewed Sep 14, 2021

View reviewed changes

remove cat

a306d45

ilkecan mentioned this pull request Sep 14, 2021

Patch the shebang of files specifed with bin field #108

Closed

ilkecan suggested changes Sep 14, 2021

View reviewed changes

fix jq filter

a75e8bc

ilkecan approved these changes Sep 14, 2021

View reviewed changes

milahu commented Sep 14, 2021

View reviewed changes

milahu added 3 commits September 14, 2021 18:52

ignore root package, throw on error

48b2eaf

in function node_modules, ignore the install script for the root package

8c9b89b

remove questionmarks from derivation names for mkdir $out

40e09df

milahu commented Sep 14, 2021

View reviewed changes

internal.nix Outdated Show resolved Hide resolved

milahu added 2 commits September 14, 2021 20:37

fix: run install script for the root package

ff6d44a

print install commands before running them

60b003c

milahu commented Sep 14, 2021

View reviewed changes

milahu added 2 commits September 15, 2021 07:54

fix jq filter: handle missing scripts field

de6d2c6

in mkShell, dont run install scripts for the root package

99370e3

avoid unnecessary or unwanted rebuilds

infinisil reviewed Mar 19, 2022

View reviewed changes

infinisil mentioned this pull request Apr 8, 2022

Support nodejs >= 15, npm >= 7, lockfile version >= 2 #153

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: patchShebangs in nonexecutable bin files (#106) #107

fix: patchShebangs in nonexecutable bin files (#106) #107

milahu commented Sep 14, 2021 •

edited

Loading

andir Sep 14, 2021

andir Sep 14, 2021

milahu Sep 14, 2021

milahu Sep 14, 2021

ilkecan Sep 14, 2021

milahu Sep 15, 2021

ilkecan Sep 14, 2021

milahu Sep 14, 2021

andir Sep 14, 2021

milahu Sep 14, 2021

milahu Sep 14, 2021 •

edited

Loading

milahu Sep 14, 2021 •

edited

Loading

andir Sep 14, 2021

milahu Sep 14, 2021 •

edited

Loading

milahu Sep 14, 2021

infinisil commented Mar 18, 2022

milahu commented Mar 19, 2022

milahu commented Mar 19, 2022

infinisil Mar 19, 2022

milahu Mar 19, 2022

infinisil Mar 22, 2022

milahu Mar 24, 2022 •

edited

Loading

		@@ -331,6 +331,9 @@ rec {

		${preInstallLinkCommands}

		cat package.json \| jq -r '. \| select(.bin != null) \| .bin \| values[]' \| while read binTarget; do

		@@ -331,6 +331,9 @@ rec {

		${preInstallLinkCommands}

		jq -r '. \| select(.bin != null) \| .bin \| values[]' package.json \| while read binTarget; do

	postPatch = ''
	ln -sf ${patchedLockfile (sourceHashFunc githubSourceHashMap) packageLockJson} package-lock.json
	ln -sf ${patchedPackagefile (sourceHashFunc githubSourceHashMap) packageJson} package.json
	'';

fix: patchShebangs in nonexecutable bin files (#106) #107

Are you sure you want to change the base?

fix: patchShebangs in nonexecutable bin files (#106) #107

Conversation

milahu commented Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milahu Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

milahu Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milahu Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil commented Mar 18, 2022

milahu commented Mar 19, 2022

milahu commented Mar 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milahu Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

milahu commented Sep 14, 2021 •

edited

Loading

milahu Sep 14, 2021 •

edited

Loading

milahu Sep 14, 2021 •

edited

Loading

milahu Sep 14, 2021 •

edited

Loading

milahu Mar 24, 2022 •

edited

Loading