Skip to content
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

Jump to bottom

Avoid Bazel's unsound directory dependency checking #1482

Closed
wants to merge 1 commit into from
Closed

Avoid Bazel's unsound directory dependency checking #1482

wants to merge 1 commit into from

Conversation

Silic0nS0ldier
Copy link

@Silic0nS0ldier Silic0nS0ldier commented Feb 19, 2024 •
edited

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

  • Covered by existing test cases
  • 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/@aspect-test+d@2.0.0_@aspect-test+c@2.0.2/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/@aspect-test+f@1.0.0/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/@aspect-test+e@1.0.0/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/@aspect-test+g@1.0.0/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/@aspect-test+a@5.0.2/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/@aspect-test+b@5.0.2/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/@aspect-test+c@2.0.2/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
Copy link

CLAassistant commented Feb 19, 2024 •
edited

CLA assistant check
All committers have signed the CLA.

@alexeagle
Copy link
Member

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
Copy link
Author

I've posted some thoughts on a potential larger change that should avoid the likely performance regression in #1408 (comment).

@jesses-canva
Copy link

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
Copy link
Member

@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
Copy link
Author

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregmagolan gregmagolan Awaiting requested review from gregmagolan

Assignees
No one assigned
Labels
None yet
Projects
Open Source
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: pkg is a directory; dependency checking of directories is unsound
4 participants
@Silic0nS0ldier @CLAassistant @alexeagle @jesses-canva