Using nodejs_binary as a tool for a genrule requires --bazel_patch_module_resolver or disabling sandbox

[jbms]

🐞 bug report

Description

Using a nodejs_binary as a tool for a genrule does not appear to work except by disabling the sandbox. Specifying --bazel_patch_module_resolver also partially fixes the problem, but does not allow esbuild to work correctly since the patching naturally does not affect esbuild.

🔬 Minimal Reproduction

https://gist.github.com/jbms/67e28412418f453c48e1e1207c9925d1

With --bazel_patch_module_resolver NOT specified:

bazelisk.py build :empty.txt

fails, with the error:

Error: Cannot find module 'postcss'

Adding --bazel_patch_module_resolver, or running with sandbox disabled:

bazelisk.py build :empty.txt --genrule_strategy=local

fixes the problem.

The same thing happens with:

bazelisk.py build :foo.js

This results in the error:

Error: Cannot find module 'esbuild'

Running outside the sandbox with:

bazelisk.py build :foo.js --genrule_strategy=local

makes it run correctly, and esbuild correctly locates the lodash module.

Unlike the postcss case, specifying --bazel_patch_module_resolver does not fully fix this case. It does indeed enable the esbuild module to be found, but naturally esbuild is then unable to locate the lodash module.

🌍 Your Environment

Operating System:

  
Debian testing
  

Output of bazel version:

  
Build label: 4.1.0rc1
Build target: bazel-out/k8-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Mon Mar 15 11:13:13 2021 (1615806793)
Build timestamp: 1615806793
Build timestamp as int: 1615806793
  

Rules_nodejs version:

  
http_archive(
    name = "build_bazel_rules_nodejs",
    sha256 = "f533eeefc8fe1ddfe93652ec50f82373d0c431f7faabd5e6323f6903195ef227",
    urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/3.3.0/rules_nodejs-3.3.0.tar.gz"],
)

  

Anything else relevant?

[jbms]

As a related issue, the launcher shell script appears to be too aggressive in resolving symlinks, such that --preserve-symlinks-main does not work since the symlink has already been resolved. That makes it difficult to locate runfiles relative to the binary location.

[alexeagle]

We have the npm_package_bin rule that understands how to adapt a nodejs_binary to run as an action. You ought to use that instead of genrule so that the module resolutions work.

[alexeagle]

I'm going to take this as a prompt to fix

https://docs.bazel.build/versions/master/be/general.html#genrule

Genrules are generic build rules that you can use if there's no specific rule for the task. If for example you want to minify JavaScript files then you can use a genrule to do so.

which we think is the wrong time to use it :)

[alexeagle]

bazelbuild/bazel#13382 updates the genrule doc.

Note that technically this isn't a regression - the esbuild rule was introduced in 3.2.0 after the flag flip for --bazel_patch_module_resolver so that case probably never worked.

In 3.4.0 we fixed esbuild module resolution
be184c2
so could you try that again?

I'd also note, it would be desirable if nodejs_binary produced a tool that could plug into genrule in the way you expect. Right now we don't have any maintainers with spare time to look into it.

[jbms]

To be clear, I'm not actually using the esbuild-specific rules that are part of this package, I'm invoking esbuild through my own scripts.

In general it seems like it would be best if rules_nodejs can provide an environment that is as close as possible to a normal non-bazel nodejs environment, so that most tools will just work without any modification.

I also noticed that the "linker" operates in a somewhat unusual way, writing symlinks directly to the runfiles tree at runtime. Is there a reason that you didn't just use ctx.runfiles? It does have the downside that you cannot symlink directories, only individual files, which is slightly annoying, but still it is advantageous in that it then reuses bazel's normal runfiles mechanism.

[alexeagle]

Yeah, we don't use runfiles because node.js doesn't know to look there. Even with --bazel_patch_module_resolver we only monkey-patch the built-in require function to know about runfiles, there are still plenty of npm packages that use a third-party implementation like http://npmjs.com/package/resolve or which simply look for the node_modules tree in the current working directory and load files directly.

[jbms]

As far as I am aware, nodejs module resolution, including separate implementations e.g. in esbuild, etc., is always done based on the directory containing the current (importing) module. (In addition NODE_PATH may be consulted.) If you create a node_modules directory of symlinks at the root of the runfiles tree, and use --preserve-symlinks and --preserve-symlinks-main, then I think the normal module resolution would just work without any special knowledge of runfiles.

[gonzojive]

FYI:

A nodejs_binary was used to change imports for protobuf code before commit 5f26d0f was applied. See change_import_style.js (5f26d0f#diff-bea936771af0977ee76a9000ec2c96acbb1bbea71c6f333587121834b8391ce0).

I still have a version of change_import_style.js around that fails with `Error: Cannot find module 'minimist'. (Edit: It turns out this was because the target I am using was not updated after --bazel_patch_module_resolver=false was mde the default in #2125.)

[github-actions]

This issue has been automatically marked as stale because it has not had any activity for 90 days. It will be closed if no further activity occurs in two weeks. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!