[Bug]: running `pnpm` from `rules_js` has non deterministic outcomes since `pnpm` relies on `npm` on the user path

[Aghassi]

What happened?

We foud that if you use bazel run -- @pnpm//:pnpm info <package> --verbose, you will see what version of node and what version of npm is being used. The version of npm used is derived from the user's PATH instead of from the sandbox. node however is safe as it's part of js_binary. The reason this is problematic is that npm is what may or may not have a hand in determining the integrity field in the lockfile. If you have npm version drift you can end up in a state where users are getting different integrity SHAs in the lockfile based on the response from the registry.

To reproduce you can simply do the following:

1. Install nvm https://github.com/nvm-sh/nvm
2. nvm install 16 && nvm use 16
3. bazel run -- @pnpm//:pnpm info react --verbose
4. nvm install 18 && nvm use 18
5. bazel run -- @pnpm//:pnpm info react --verbose

Notice that between runs the path in this output will change. In my case, you can see I have npm from node 16 installed on my path when my monorepo uses node 18

npm verb cli /private/var/tmp/_bazel_davidaghassi/30b12e6784b9304c95faa3f4506c5839/execroot/rh/bazel-out/darwin_arm64-fastbuild/bin/external/pnpm/pnpm.sh.runfiles/rh/../pnpm/pnpm_node_bin/node /Users/davidaghassi/.nvm/versions/node/v16.18.0/bin/npm
npm info using npm@8.19.2
npm info using node@v18.18.2

Thanks to @joeljeske for helping me pin point and debug this. This may be related to this issue we are seeing too pnpm/pnpm#7419 (comment)

Version

Development (host) and target OS/architectures:

Output of bazel --version:
6.4.0

Version of the Aspect rules, or other relevant rules from your
WORKSPACE or MODULE.bazel file:
rules_js: 1.39.1

Language(s) and/or frameworks involved:

How to reproduce

See above

Any other information?

No response

[jbedard]

Notes:

• See https://github.com/pnpm/pnpm/blob/442d97daad425c9bc923c12889d5b1ccee154992/pnpm/src/runNpm.ts#L6

[mislavmandaricaxilis]

@jbedard @alexeagle After rules_nodejs#3845 got merged, I tested this while using bazel_env and it looks like it's working properly - it picked up my npm from bazel_env sandbox.

If you confirm that this kind of setup works, should we document it somewhere in rules_js as a way to fix this behaviour?

[jbedard]

Is the fix simply updating to include bazel-contrib/rules_nodejs#3845 once it's released? Or is there additional manual work required somewhere?

[korina-simicevic]

Hi, all!

I recently took a look at this issue and attempted to patch it locally for our use case. I applied the following changes:

--- npm/private/pnpm_repository.bzl
+++ npm/private/pnpm_repository.bzl
@@ -41,7 +41,7 @@ def pnpm_repository(name, pnpm_version = DEFAULT_PNPM_VERSION):
             version = pnpm_version,
             extra_build_content = "\n".join([
                 """load("@aspect_rules_js//js:defs.bzl", "js_binary")""",
-                """js_binary(name = "pnpm", data = glob(["package/**"]), entry_point = "package/dist/pnpm.cjs", visibility = ["//visibility:public"])""",
+                """js_binary(name = "pnpm", data = glob(["package/**"]), entry_point = "package/dist/pnpm.cjs", include_npm = True, visibility = ["//visibility:public"])""",
             ]),
             extract_full_archive = True,
         )

I tested this, and it successfully picked up the npm path from the sandbox.
Everything else works fine, but I am unsure of what else needs testing and doing.

Could anyone take a look at this in order to confirm if this is a valid fix for the issue?

[jbedard]

I think that is the correct fix and will be easier in 3.x with #2580

[jbedard]

I'm going to close this since it is possible in 3.x now, although is opt-in still which we can discuss still.