Avoid Bazel's unsound directory dependency checking

[Silic0nS0ldier]

trafficstars

Fixes #1408.

This PR modifies npm_import (and by extension npm_translate_lock) to mask the package source directory with a generated target and TreeArtifact (ctx.declare_directory) output of the same name. This allows package files to be collected with glob(["package/**"]), such that no source directory dependency exists (which Bazel reports as unsound).

Any rule that depends on the package source directory directly (there should be none) will now be depending on the :package target instead (this should extend to every file inside the directory as well, I think).

Repo rules are the exception, they cannot consume the :package target and so will continue to see the source directory. There are no output differences, so this is fine.

Type of change

Bug fix (Bazel 7+).

Currently mitigated with;

• --noincompatible_disallow_unsound_directory_outputs, if builds are failing (observed in other rules).
• startup --host_jvm_args=-DBAZEL_TRACK_SOURCE_DIRECTORIES=1, to hide warnings.

Test plan

• [x] Covered by existing test cases
• [x] Manual testing; please provide instructions so we can reproduce:
  1. Clone rules_js repo (checking out main/this PR as appropriate for before/after)
  2. Remove mitigations. (startup --host_jvm_args=-DBAZEL_TRACK_SOURCE_DIRECTORIES=1 from .aspect/bazelrc/correctness.bazelrc)
  3. cd e2e/pnpm_workspace
  4. bazel build //app/a:main
     • Baseline: Warnings visible. e.g.

       (17:52:22) WARNING: BUILD.bazel:3:22: input 'package' to //:.aspect_rules_js/node_modules/@[email protected]_@[email protected]/pkg is a directory; dependency checking of directories is unsound
       (17:52:22) WARNING: BUILD.bazel:3:22: input 'package' to //:.aspect_rules_js/node_modules/@[email protected]/pkg is a directory; dependency checking of directories is unsound
       (17:52:22) WARNING: BUILD.bazel:3:22: input 'package' to //:.aspect_rules_js/node_modules/@[email protected]/pkg is a directory; dependency checking of directories is unsound
       (17:52:22) WARNING: BUILD.bazel:3:22: input 'package' to //:.aspect_rules_js/node_modules/@[email protected]/pkg is a directory; dependency checking of directories is unsound
       (17:52:22) WARNING: BUILD.bazel:3:22: input 'package' to //:.aspect_rules_js/node_modules/@[email protected]/pkg is a directory; dependency checking of directories is unsound
       (17:52:23) WARNING: BUILD.bazel:3:22: input 'package' to //:.aspect_rules_js/node_modules/@[email protected]/pkg is a directory; dependency checking of directories is unsound
       (17:52:23) WARNING: BUILD.bazel:3:22: input 'package' to //:.aspect_rules_js/node_modules/@[email protected]/lc is a directory; dependency checking of directories is unsound
       

     • PR: No warnings.
• [ ] Benchmark to ensure no performance regression (merge requirement, regressions here will be significant)

[CLAassistant]

All committers have signed the CLA.

[alexeagle]

Hey @Silic0nS0ldier thanks for sending this.

This definitely causes a performance regression, since there must be an extra copy made of all node_modules files, and there can be easily O(10k) of those. We could measure the performance regression by repeating some of the benchmarks from https://blog.aspect.dev/rulesjs-npm-benchmarks to understand the severity - right now we don't have a repeatable recipe to make performance decisions about PRs.

One possibility to unblock #1408 would be to make this change hidden behind a flag. But Bazel has hundreds of flags and someone would have to find that bug to discover it, so that's not really great.

[Silic0nS0ldier]

I've posted some thoughts on a potential larger change that should avoid the likely performance regression in https://github.com/aspect-build/rules_js/issues/1408#issuecomment-1953720626.

[jesses-canva]

IMO in the absence of another fix this is a good change to have behind an option (eg an environment variable to pass with --repo_env). The many "dependency checking of directories is unsound" warnings are spooky and it's unclear if incorrect builds could possibly result. It would be good to at least have the option of fixing it at the expense of performance.

[alexeagle]

@jesses-canva I'm okay with adding an option controlled by --repo_env as you suggest, at least for engineers who manage to find this issue, they could discover that it exists. On an email thread Tiago and Greg have been making some progress in understanding the root cause here.

[Silic0nS0ldier]

Superseded by #1538 which fixes this issue more efficiently (were possible extraction is performed in an action, eliminating the need to run CopyDirectory in the process).