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.

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
39 changes: 35 additions & 4 deletions internal.nix
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 @@ -26,12 +26,12 @@ rec {
"0" <= c && c <= "9" ||
"a" <= c && c <= "z" ||
"A" <= c && c <= "Z" ||
c == "+" || c == "-" || c == "." || c == "_" || c == "?" || c == "=";
c == "+" || c == "-" || c == "." || c == "_" || c == "=";

# Description: Converts a npm package name to something that is compatible with nix
# Type: String -> String
makeValidDrvName = str:
lib.stringAsChars (c: if isValidDrvNameChar c then c else "?") str;
lib.stringAsChars (c: if isValidDrvNameChar c then c else "-") str;

# Description: Takes a string of the format "github:org/repo#revision" and returns
# an attribute set { org, repo, rev }
Expand Down Expand Up @@ -214,7 +214,9 @@ rec {
dependencies = if (content ? dependencies) then lib.mapAttrs patchDep content.dependencies else { };
devDependencies = if (content ? devDependencies) then lib.mapAttrs patchDep content.devDependencies else { };
in
content // { inherit devDependencies dependencies; };
content //
{ inherit devDependencies dependencies; } //
{ scripts = {}; }; # in function node_modules, ignore the install script for the root package

# Description: Takes a Path to a package file and returns the patched version as file in the Nix store
# Type: Fn -> Path -> Derivation
Expand Down Expand Up @@ -283,6 +285,22 @@ rec {
else
throw "sourceHashFunc: spec.type '${spec.type}' is not supported. Supported types: 'github'";

runInstallScriptsForRootPackage = ''
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

# https://docs.npmjs.com/cli/v7/using-npm/scripts#npm-install
allScripts=$(jq -r '.scripts | keys[]' package.json)
runScripts=""
for script in preinstall install postinstall prepublish preprepare prepare postprepare; do
if ( echo "$allScripts" | grep "^$script$" >/dev/null ); then
runScripts+=" $script"
fi
done
if [ ! -z "$runScripts" ]; then
echo "run install scripts for root package:"
for script in $runScripts; do echo " npm run $script"; done # make easier to copy-paste
for script in $runScripts; do npm run $script || break; done
fi
'';

node_modules =
{ src
, packageJson ? src + "/package.json"
Expand Down Expand Up @@ -331,6 +349,16 @@ rec {

${preInstallLinkCommands}

# 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
Comment on lines +356 to +364
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

@milahu milahu Mar 24, 2022

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

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

  1. 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
  2. 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 ...)
  3. 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


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

nativeBuildInputs = nativeBuildInputs ++ [
nodejs
jq
];

propagatedBuildInputs = [
Expand Down Expand Up @@ -416,6 +445,7 @@ rec {
shellHook = ''
# FIXME: we should somehow register a GC root here in case of a symlink?
${add_node_modules_to_cwd nm node_modules_mode}
${runInstallScriptsForRootPackage}
'' + shellHook;
passthru = passthru // {
node_modules = nm;
Expand Down Expand Up @@ -446,6 +476,7 @@ rec {

buildPhase = ''
runHook preBuild
${runInstallScriptsForRootPackage}
${lib.concatStringsSep "\n" buildCommands}
runHook postBuild
'';
Expand Down