Merge pull request #206477 from winterqt/build-npm-package-patch-npm

buildNpmPackage: patch npm to work around various roadblocks

9 files changed, 213 insertions, 20 deletions

diff --git a/pkgs/build-support/node/build-npm-package/hooks/default.nix b/pkgs/build-support/node/build-npm-package/hooks/default.nix
index e5c93f1f77842..c34709335ff71 100644
--- a/pkgs/build-support/node/build-npm-package/hooks/default.nix
+++ b/pkgs/build-support/node/build-npm-package/hooks/default.nix
@@ -6,6 +6,7 @@
       name = "npm-config-hook";
       substitutions = {
         nodeSrc = srcOnly nodejs;
+        nodeGyp = "${buildPackages.nodejs}/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js";
 
         # Specify `diff`, `jq`, and `prefetch-npm-deps` by abspath to ensure that the user's build
         # inputs do not cause us to find the wrong binaries.
diff --git a/pkgs/build-support/node/build-npm-package/hooks/npm-config-hook.sh b/pkgs/build-support/node/build-npm-package/hooks/npm-config-hook.sh
index c1acdc3ac3f0f..0edc0e2b36a46 100644
--- a/pkgs/build-support/node/build-npm-package/hooks/npm-config-hook.sh
+++ b/pkgs/build-support/node/build-npm-package/hooks/npm-config-hook.sh
@@ -3,10 +3,15 @@
 npmConfigHook() {
     echo "Executing npmConfigHook"
 
+    # Use npm patches in the nodejs package
+    export NIX_NODEJS_BUILDNPMPACKAGE=1
+    export prefetchNpmDeps="@prefetchNpmDeps@"
+
     echo "Configuring npm"
 
     export HOME="$TMPDIR"
     export npm_config_nodedir="@nodeSrc@"
+    export npm_config_node_gyp="@nodeGyp@"
 
     if [ -z "${npmDeps-}" ]; then
         echo
@@ -93,22 +98,7 @@ npmConfigHook() {
 
     patchShebangs node_modules
 
-    local -r lockfileVersion="$(@jq@ .lockfileVersion package-lock.json)"
-
-    if (( lockfileVersion < 2 )); then
-      # This is required because npm consults a hidden lockfile in node_modules to figure out
-      # what to create bin links for. When using an old lockfile offline, this hidden lockfile
-      # contains insufficent data, making npm silently fail to create links. The hidden lockfile
-      # is bypassed when any file in node_modules is newer than it. Thus, we create a file when
-      # using an old lockfile, so bin links work as expected without having to downgrade Node or npm.
-      touch node_modules/.meow
-    fi
-
-    npm rebuild "${npmRebuildFlags[@]}" "${npmFlags[@]}"
-
-    if (( lockfileVersion < 2 )); then
-      rm node_modules/.meow
-    fi
+    npm rebuild $npmRebuildFlags "${npmRebuildFlagsArray[@]}" $npmFlags "${npmFlagsArray[@]}"
 
     patchShebangs node_modules
 
diff --git a/pkgs/development/tools/vsce/default.nix b/pkgs/development/tools/vsce/default.nix
index 633eda1371c1e..7fc1b297922ee 100644
--- a/pkgs/development/tools/vsce/default.nix
+++ b/pkgs/development/tools/vsce/default.nix
@@ -1,8 +1,10 @@
 { lib
+, stdenv
 , buildNpmPackage
 , fetchFromGitHub
 , pkg-config
 , libsecret
+, darwin
 , python3
 , testers
 , vsce
@@ -27,7 +29,8 @@ buildNpmPackage rec {
 
   nativeBuildInputs = [ pkg-config python3 ];
 
-  buildInputs = [ libsecret ];
+  buildInputs = [ libsecret ]
+    ++ lib.optionals stdenv.isDarwin (with darwin.apple_sdk.frameworks; [ AppKit Security ]);
 
   makeCacheWritable = true;
   npmFlags = [ "--legacy-peer-deps" ];
@@ -43,4 +46,3 @@ buildNpmPackage rec {
     license = licenses.mit;
   };
 }
-
diff --git a/pkgs/development/web/nodejs/fix-npm-patch-paths.sh b/pkgs/development/web/nodejs/fix-npm-patch-paths.sh
new file mode 100755
index 0000000000000..5f57032807c78
--- /dev/null
+++ b/pkgs/development/web/nodejs/fix-npm-patch-paths.sh
@@ -0,0 +1,7 @@
+#!/usr/bin/env nix-shell
+#!nix-shell -i bash -p gnused
+
+sed -i "s| a/node_modules| a/deps/npm/node_modules|" node-npm-build-npm-package-logic.patch
+sed -i "s| b/node_modules| b/deps/npm/node_modules|" node-npm-build-npm-package-logic.patch
+sed -i "s| a/workspaces| a/deps/npm/node_modules/@npmcli|" node-npm-build-npm-package-logic.patch
+sed -i "s| b/workspaces| b/deps/npm/node_modules/@npmcli|" node-npm-build-npm-package-logic.patch
diff --git a/pkgs/development/web/nodejs/node-npm-build-npm-package-logic-node16.patch b/pkgs/development/web/nodejs/node-npm-build-npm-package-logic-node16.patch
new file mode 100644
index 0000000000000..f4d3b0e32b1c6
--- /dev/null
+++ b/pkgs/development/web/nodejs/node-npm-build-npm-package-logic-node16.patch
@@ -0,0 +1,95 @@
+This patch is based off of npm tag v8.19.4.
+
+This introduces fixes for 4 issues:
+
+1. When node-gyp is included as a dependency in a project, any scripts that run it will not use the copy included in Node. This is problematic because we patch node-gyp to work without xcbuild on Darwin, leading to these packages failing to build with a sandbox on Darwin.
+2. When a Git dependency contains install scripts, it has to be built just like any other package. Thus, we need to patch shebangs appropriately, just like in npmConfigHook.
+3. We get useless warnings that clog up logs when using a v1 lockfile, so we silence them.
+4. npm looks at a hidden lockfile to determine if files have binaries to link into `node_modules/.bin`. When using a v1 lockfile offline, this lockfile does not contain enough info, leading to binaries for packages such as Webpack not being available to scripts. We used to work around this by making npm ignore the hidden lockfile by creating a file, but now we just disable the code path entirely.
+
+To update:
+1. Run `git diff` from an npm checkout
+2. Run `fix-npm-patch-paths.sh`
+3. Include/update this frontmatter, please!
+
+diff --git a/deps/npm/node_modules/@npmcli/run-script/lib/set-path.js b/deps/npm/node_modules/@npmcli/run-script/lib/set-path.js
+index c59c270d9..98785192f 100644
+--- a/deps/npm/node_modules/@npmcli/run-script/lib/set-path.js
++++ b/deps/npm/node_modules/@npmcli/run-script/lib/set-path.js
+@@ -12,7 +12,10 @@ const setPATH = (projectPath, binPaths, env) => {
+     .reduce((set, p) => set.concat(p.filter(concatted => !set.includes(concatted))), [])
+     .join(delimiter)
+ 
+-  const pathArr = []
++  // Ensure when using buildNpmPackage hooks that Node.js'
++  // bundled copy of node-gyp is used, instead of any copy
++  // pulled in as a dependency.
++  const pathArr = process.env['NIX_NODEJS_BUILDNPMPACKAGE'] ? [nodeGypPath, PATH] : [];
+   if (binPaths) {
+     pathArr.push(...binPaths)
+   }
+@@ -26,7 +29,8 @@ const setPATH = (projectPath, binPaths, env) => {
+     pp = p
+     p = dirname(p)
+   } while (p !== pp)
+-  pathArr.push(nodeGypPath, PATH)
++  if (!process.env['NIX_NODEJS_BUILDNPMPACKAGE']) { pathArr.push(nodeGypPath, PATH) }
++
+ 
+   const pathVal = pathArr.join(delimiter)
+ 
+diff --git a/deps/npm/node_modules/pacote/lib/git.js b/deps/npm/node_modules/pacote/lib/git.js
+index c4819b4fd..7efbeef05 100644
+--- a/deps/npm/node_modules/pacote/lib/git.js
++++ b/deps/npm/node_modules/pacote/lib/git.js
+@@ -186,6 +186,24 @@ class GitFetcher extends Fetcher {
+       }
+       noPrepare.push(this.resolved)
+ 
++      if (process.env['NIX_NODEJS_BUILDNPMPACKAGE']) {
++        const spawn = require('@npmcli/promise-spawn')
++
++        const npmWithNixFlags = (args, cmd) => spawn('bash', ['-c', 'npm ' + args + ` $npm${cmd}Flags "$\{npm${cmd}FlagsArray[@]}" $npmFlags "$\{npmFlagsArray[@]}"`], { cwd: dir, env: { ...process.env, _PACOTE_NO_PREPARE_: noPrepare.join('\n') } }, { message: `\`npm ${args}\` failed` })
++        const patchShebangs = () => spawn('bash', ['-c', 'source $stdenv/setup; patchShebangs node_modules'], { cwd: dir })
++
++        // the DirFetcher will do its own preparation to run the prepare scripts
++        // All we have to do is put the deps in place so that it can succeed.
++        //
++        // We ignore this.npmConfig to maintain an environment that's as close
++        // to the rest of the build as possible.
++        return spawn('bash', ['-c', '$prefetchNpmDeps --fixup-lockfile package-lock.json'], { cwd: dir })
++        .then(() => npmWithNixFlags('ci --ignore-scripts', 'Install'))
++        .then(patchShebangs)
++        .then(() => npmWithNixFlags('rebuild', 'Rebuild'))
++        .then(patchShebangs)
++      }
++
+       // the DirFetcher will do its own preparation to run the prepare scripts
+       // All we have to do is put the deps in place so that it can succeed.
+       return npm(
+diff --git a/deps/npm/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js b/deps/npm/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js
+index e9a8720d7..b29ad0185 100644
+--- a/deps/npm/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js
++++ b/deps/npm/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js
+@@ -744,7 +744,7 @@ This is a one-time fix-up, please be patient...
+           node.package = { ...mani, _id: `${mani.name}@${mani.version}` }
+         } catch (er) {
+           const warning = `Could not fetch metadata for ${name}@${id}`
+-          log.warn(heading, warning, er)
++          if (!process.env['NIX_NODEJS_BUILDNPMPACKAGE']) { log.warn(heading, warning, er) }
+         }
+         this.finishTracker(t)
+       })
+diff --git a/deps/npm/node_modules/@npmcli/arborist/lib/arborist/load-actual.js b/deps/npm/node_modules/@npmcli/arborist/lib/arborist/load-actual.js
+index 7ab65f5b0..12f563a50 100644
+--- a/deps/npm/node_modules/@npmcli/arborist/lib/arborist/load-actual.js
++++ b/deps/npm/node_modules/@npmcli/arborist/lib/arborist/load-actual.js
+@@ -143,7 +143,7 @@ module.exports = cls => class ActualLoader extends cls {
+     this[_actualTree].assertRootOverrides()
+ 
+     // if forceActual is set, don't even try the hidden lockfile
+-    if (!forceActual) {
++    if (!forceActual && !process.env['NIX_NODEJS_BUILDNPMPACKAGE']) {
+       // Note: hidden lockfile will be rejected if it's not the latest thing
+       // in the folder, or if any of the entries in the hidden lockfile are
+       // missing.
diff --git a/pkgs/development/web/nodejs/node-npm-build-npm-package-logic.patch b/pkgs/development/web/nodejs/node-npm-build-npm-package-logic.patch
new file mode 100644
index 0000000000000..a9ac6b0589efc
--- /dev/null
+++ b/pkgs/development/web/nodejs/node-npm-build-npm-package-logic.patch
@@ -0,0 +1,95 @@
+This patch is based off of npm tag v9.1.5.
+
+This introduces fixes for 4 issues:
+
+1. When node-gyp is included as a dependency in a project, any scripts that run it will not use the copy included in Node. This is problematic because we patch node-gyp to work without xcbuild on Darwin, leading to these packages failing to build with a sandbox on Darwin.
+2. When a Git dependency contains install scripts, it has to be built just like any other package. Thus, we need to patch shebangs appropriately, just like in npmConfigHook.
+3. We get useless warnings that clog up logs when using a v1 lockfile, so we silence them.
+4. npm looks at a hidden lockfile to determine if files have binaries to link into `node_modules/.bin`. When using a v1 lockfile offline, this lockfile does not contain enough info, leading to binaries for packages such as Webpack not being available to scripts. We used to work around this by making npm ignore the hidden lockfile by creating a file, but now we just disable the code path entirely.
+
+To update:
+1. Run `git diff` from an npm checkout
+2. Run `fix-npm-patch-paths.sh`
+3. Include/update this frontmatter, please!
+
+diff --git a/deps/npm/node_modules/@npmcli/run-script/lib/set-path.js b/deps/npm/node_modules/@npmcli/run-script/lib/set-path.js
+index c59c270d9..98785192f 100644
+--- a/deps/npm/node_modules/@npmcli/run-script/lib/set-path.js
++++ b/deps/npm/node_modules/@npmcli/run-script/lib/set-path.js
+@@ -12,7 +12,10 @@ const setPATH = (projectPath, binPaths, env) => {
+     .reduce((set, p) => set.concat(p.filter(concatted => !set.includes(concatted))), [])
+     .join(delimiter)
+ 
+-  const pathArr = []
++  // Ensure when using buildNpmPackage hooks that Node.js'
++  // bundled copy of node-gyp is used, instead of any copy
++  // pulled in as a dependency.
++  const pathArr = process.env['NIX_NODEJS_BUILDNPMPACKAGE'] ? [nodeGypPath, PATH] : [];
+   if (binPaths) {
+     pathArr.push(...binPaths)
+   }
+@@ -26,7 +29,8 @@ const setPATH = (projectPath, binPaths, env) => {
+     pp = p
+     p = dirname(p)
+   } while (p !== pp)
+-  pathArr.push(nodeGypPath, PATH)
++  if (!process.env['NIX_NODEJS_BUILDNPMPACKAGE']) { pathArr.push(nodeGypPath, PATH) }
++
+ 
+   const pathVal = pathArr.join(delimiter)
+ 
+diff --git a/deps/npm/node_modules/pacote/lib/git.js b/deps/npm/node_modules/pacote/lib/git.js
+index 1fa8b1f96..a026bb50d 100644
+--- a/deps/npm/node_modules/pacote/lib/git.js
++++ b/deps/npm/node_modules/pacote/lib/git.js
+@@ -188,6 +188,24 @@ class GitFetcher extends Fetcher {
+       }
+       noPrepare.push(this.resolved)
+ 
++      if (process.env['NIX_NODEJS_BUILDNPMPACKAGE']) {
++        const spawn = require('@npmcli/promise-spawn')
++
++        const npmWithNixFlags = (args, cmd) => spawn('bash', ['-c', 'npm ' + args + ` $npm${cmd}Flags "$\{npm${cmd}FlagsArray[@]}" $npmFlags "$\{npmFlagsArray[@]}"`], { cwd: dir, env: { ...process.env, _PACOTE_NO_PREPARE_: noPrepare.join('\n') } }, { message: `\`npm ${args}\` failed` })
++        const patchShebangs = () => spawn('bash', ['-c', 'source $stdenv/setup; patchShebangs node_modules'], { cwd: dir })
++
++        // the DirFetcher will do its own preparation to run the prepare scripts
++        // All we have to do is put the deps in place so that it can succeed.
++        //
++        // We ignore this.npmConfig to maintain an environment that's as close
++        // to the rest of the build as possible.
++        return spawn('bash', ['-c', '$prefetchNpmDeps --fixup-lockfile package-lock.json'], { cwd: dir })
++        .then(() => npmWithNixFlags('ci --ignore-scripts', 'Install'))
++        .then(patchShebangs)
++        .then(() => npmWithNixFlags('rebuild', 'Rebuild'))
++        .then(patchShebangs)
++      }
++
+       // the DirFetcher will do its own preparation to run the prepare scripts
+       // All we have to do is put the deps in place so that it can succeed.
+       return npm(
+diff --git a/deps/npm/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js b/deps/npm/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js
+index 2ea66ac33..25e671318 100644
+--- a/deps/npm/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js
++++ b/deps/npm/node_modules/@npmcli/arborist/lib/arborist/build-ideal-tree.js
+@@ -740,7 +740,7 @@ This is a one-time fix-up, please be patient...
+           node.package = { ...mani, _id: `${mani.name}@${mani.version}` }
+         } catch (er) {
+           const warning = `Could not fetch metadata for ${name}@${id}`
+-          log.warn(heading, warning, er)
++          if (!process.env['NIX_NODEJS_BUILDNPMPACKAGE']) { log.warn(heading, warning, er) }
+         }
+         this.finishTracker(t)
+       })
+diff --git a/deps/npm/node_modules/@npmcli/arborist/lib/arborist/load-actual.js b/deps/npm/node_modules/@npmcli/arborist/lib/arborist/load-actual.js
+index 6c3f917c6..ec21d2cc4 100644
+--- a/deps/npm/node_modules/@npmcli/arborist/lib/arborist/load-actual.js
++++ b/deps/npm/node_modules/@npmcli/arborist/lib/arborist/load-actual.js
+@@ -147,7 +147,7 @@ module.exports = cls => class ActualLoader extends cls {
+       this[_actualTree].assertRootOverrides()
+ 
+       // if forceActual is set, don't even try the hidden lockfile
+-      if (!forceActual) {
++      if (!forceActual && !process.env['NIX_NODEJS_BUILDNPMPACKAGE']) {
+         // Note: hidden lockfile will be rejected if it's not the latest thing
+         // in the folder, or if any of the entries in the hidden lockfile are
+         // missing.
diff --git a/pkgs/development/web/nodejs/v16.nix b/pkgs/development/web/nodejs/v16.nix
index 2b8ee642f39f2..8d5c5c1b11b6d 100644
--- a/pkgs/development/web/nodejs/v16.nix
+++ b/pkgs/development/web/nodejs/v16.nix
@@ -15,5 +15,6 @@ in
     patches = [
       ./disable-darwin-v8-system-instrumentation.patch
       ./bypass-darwin-xcrun-node16.patch
+      ./node-npm-build-npm-package-logic-node16.patch
     ] ++ npmPatches;
   }
diff --git a/pkgs/development/web/nodejs/v18.nix b/pkgs/development/web/nodejs/v18.nix
index bb3a2545ea5f4..44411ea731f42 100644
--- a/pkgs/development/web/nodejs/v18.nix
+++ b/pkgs/development/web/nodejs/v18.nix
@@ -1,4 +1,4 @@
-{ callPackage, fetchpatch, openssl, python3, enableNpm ? true }:
+{ callPackage, openssl, python3, enableNpm ? true }:
 
 let
   buildNodejs = callPackage ./nodejs.nix {
@@ -15,5 +15,6 @@ buildNodejs {
     ./disable-darwin-v8-system-instrumentation.patch
     ./bypass-darwin-xcrun-node16.patch
     ./revert-arm64-pointer-auth.patch
+    ./node-npm-build-npm-package-logic.patch
   ];
 }
diff --git a/pkgs/development/web/nodejs/v20.nix b/pkgs/development/web/nodejs/v20.nix
index 90bb75bb2a93d..eca03e5d66e96 100644
--- a/pkgs/development/web/nodejs/v20.nix
+++ b/pkgs/development/web/nodejs/v20.nix
@@ -1,4 +1,4 @@
-{ callPackage, openssl, fetchpatch, python3, enableNpm ? true }:
+{ callPackage, openssl, python3, enableNpm ? true }:
 
 let
   buildNodejs = callPackage ./nodejs.nix {
@@ -15,5 +15,6 @@ buildNodejs {
     ./revert-arm64-pointer-auth.patch
     ./disable-darwin-v8-system-instrumentation-node19.patch
     ./bypass-darwin-xcrun-node16.patch
+    ./node-npm-build-npm-package-logic.patch
   ];
 }