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.

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

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

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 2 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 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}

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

  1. does not exist
  2. is a string
  3. is an object

all of these are handled

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

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