Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

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

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
98c359b
fix: patchShebangs in nonexecutable bin files (#106)
milahu Sep 14, 2021
a306d45
remove cat
milahu Sep 14, 2021
a75e8bc
fix jq filter
milahu Sep 14, 2021
48b2eaf
ignore root package, throw on error
milahu Sep 14, 2021
8c9b89b
in function node_modules, ignore the install script for the root package
milahu Sep 14, 2021
40e09df
remove questionmarks from derivation names for mkdir $out
milahu Sep 14, 2021
ff6d44a
fix: run install script for the root package
milahu Sep 14, 2021
60b003c
print install commands before running them
milahu Sep 14, 2021
de6d2c6
fix jq filter: handle missing scripts field
milahu Sep 15, 2021
99370e3
in mkShell, dont run install scripts for the root package
milahu Sep 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion internal.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{ nodejs, stdenv, mkShell, lib, fetchurl, writeText, writeTextFile, runCommand, fetchFromGitHub }:
{ nodejs, stdenv, mkShell, lib, fetchurl, writeText, writeTextFile, runCommand, fetchFromGitHub, jq }:
rec {
default_nodejs = nodejs;

Expand Down Expand Up @@ -331,6 +331,9 @@ rec {

${preInstallLinkCommands}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the package.json guranteed to be there?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

chmod +x "$binTarget" # patchShebangs will only patch executable files
Copy link
Contributor Author

@milahu milahu Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@milahu milahu Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?).

Copy link
Contributor Author

@milahu milahu Sep 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

done
if grep -I -q -r '/bin/' .; then
source $TMP/preinstall-env
patchShebangs .
Expand All @@ -348,6 +351,7 @@ rec {

nativeBuildInputs = nativeBuildInputs ++ [
nodejs
jq
];

propagatedBuildInputs = [
Expand Down