Skip to content

Patch shebangs in dependency binaries #266

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
thomasjm wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Patch shebangs in dependency binaries #266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
17f56d8
Patch shebangs in dependencies
thomasjm Oct 13, 2021
964975b
Add one more arg
thomasjm Oct 13, 2021
705317b
Expose preRebuild hook in buildNodeDependencies
thomasjm Oct 13, 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
2 changes: 1 addition & 1 deletion default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

let
nodeEnv = import ./nix/node-env.nix {
inherit (pkgs) stdenv lib python2 runCommand writeTextFile writeShellScript;
inherit (pkgs) stdenv lib python2 runCommand writeTextFile writeShellScript jq;
inherit pkgs nodejs;
libtool = if pkgs.stdenv.isDarwin then pkgs.darwin.cctools else null;
};
Expand Down
1 change: 1 addition & 0 deletions lib/expressions/CompositionExpression.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ CompositionExpression.prototype.toNixAST = function() {
runCommand: new nijs.NixInherit("pkgs"),
writeTextFile: new nijs.NixInherit("pkgs"),
writeShellScript: new nijs.NixInherit("pkgs"),
jq: new nijs.NixInherit("pkgs"),
pkgs: new nijs.NixInherit(),
nodejs: new nijs.NixInherit()
}
Expand Down
15 changes: 13 additions & 2 deletions nix/node-env.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# This file originates from node2nix

{lib, stdenv, nodejs, python2, pkgs, libtool, runCommand, writeTextFile, writeShellScript}:
{lib, stdenv, nodejs, python2, pkgs, libtool, runCommand, writeTextFile, writeShellScript, jq}:

let
# Workaround to cope with utillinux in Nixpkgs 20.09 and util-linux in Nixpkgs master
Expand Down Expand Up @@ -84,6 +84,16 @@ let

# Change to the package directory to install dependencies
cd "$DIR/$packageName"

Copy link
Owner

@svanderburg svanderburg Oct 20, 2021

Choose a reason for hiding this comment

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

It would be nice if this can be rewritten in JavaScript code and executed by Node.js. This is what we do in other pieces of the script as well.

Otherwise, we need to introduce yet another build-time dependency.

Copy link
Contributor Author

@thomasjm thomasjm Oct 20, 2021

Choose a reason for hiding this comment

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

Hmm. I like processing things with jq this way because it's so fast. If you consider the amount of NPM packages that need to get downloaded on a typical build, downloading a copy of jq seems like the least of one's problems :P

# Patch shebangs in any binary files
if [ -f package.json ]; then
set +e
for file in $(cat package.json | ${jq}/bin/jq '((.bin // {}) | if type=="string" then [.] else (. | values) end) | .[]' -r); do
chmod u+x "$file"
patchShebangs "$file"
done
set -e
fi
}
'';

Expand Down Expand Up @@ -483,6 +493,7 @@ let
, dontNpmInstall ? false
, bypassCache ? false
, reconstructLock ? false
, preRebuild ? ""
Copy link
Owner

@svanderburg svanderburg Oct 20, 2021

Choose a reason for hiding this comment

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

Why is this parameter removed?

Copy link
Contributor Author

@thomasjm thomasjm Oct 20, 2021

Choose a reason for hiding this comment

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

Huh? It was missing before and it looked like an oversight so I added it. I think this will resolve #226.

, dontStrip ? true
, unpackPhase ? "true"
, buildPhase ? "true"
Expand All @@ -500,7 +511,7 @@ let
++ buildInputs;

inherit dontStrip; # Stripping may fail a build for some package deployments
inherit dontNpmInstall unpackPhase buildPhase;
inherit dontNpmInstall preRebuild unpackPhase buildPhase;

includeScript = includeDependencies { inherit dependencies; };
pinpointDependenciesScript = pinpointDependenciesOfPackage args;
Expand Down