skyframe: detect local repositories during package lookup by abishekgiri · Pull Request #29218 · bazelbuild/bazel

abishekgiri · 2026-04-02T23:13:07Z

Description

This PR teaches LocalRepositoryLookupFunction to recognize visible local repositories instead of always treating workspace paths as the main repository.

It now detects repository roots from command-line repository overrides and from Bzlmod local repositories, including local_path_override, local_repository, and new_local_repository. The lookup also avoids pulling in Bzlmod and repository graph state for ordinary non-repository directories by first checking for an actual repo boundary file.

The follow-up test updates seed the extra precomputed repository state that direct Skyframe evaluators now need for this lookup, refresh the shared Bzlmod root-module fixture for the current InterimModule builder requirements, add the direct deps required by the touched Skyframe test targets, and make the local-repo fixtures create a real MODULE.bazel boundary.

Validated locally with:

bazel test //src/test/java/com/google/devtools/build/lib/skyframe:CollectPackagesUnderDirectoryTest --test_output=errors
bazel test //src/test/java/com/google/devtools/build/lib/skyframe/packages:BazelPackageLoaderTest --test_output=errors
bazel test //src/test/java/com/google/devtools/build/lib/skyframe:LocalRepositoryLookupFunctionTest //src/test/java/com/google/devtools/build/lib/skyframe:PackageLookupFunctionTest --test_output=errors
bazel test //src/test/java/com/google/devtools/build/lib/skyframe/... --build_tests_only --test_output=errors

Motivation

Before this change, LocalRepositoryLookupFunction could misclassify visible local repositories as main-repo packages. That meant package lookup could miss the correct incorrect-repository-reference hint for cross-repository label violations.

The remaining presubmit fallout came from repository lookup doing too much work for directories that are not repository roots. Checking for a real boundary first keeps those non-repo paths on the normal package-loading path while still surfacing the correct repository hint for actual local repositories.

Build API Changes

No

Checklist

I have added tests for the new use cases (if any).
I have updated the documentation (if applicable).

Release Notes

RELNOTES: None

abishekgiri · 2026-04-02T23:13:47Z

Reopened this change from the neutral branch name local-repo-lookup-fix. The targeted test command is in the PR body; local execution on this machine is still blocked by the unaccepted Xcode license.

abishekgiri · 2026-04-02T23:28:02Z

Addressed the Buildkite compile break from the failing skyframe_cluster shard. The failure was in LocalRepositoryLookupFunction: RepoDefinitionFunction.REPOSITORY_OVERRIDES.get(env) needed to propagate InterruptedException, and the new Bzlmod types also needed direct deps on //src/main/java/com/google/devtools/build/lib/bazel/bzlmod:common and //src/main/java/com/google/devtools/build/lib/bazel/bzlmod:module_extension. Pushed in 6caf65e.

bharadwaj08-one · 2026-04-03T09:16:06Z

@abishekgiri Could you please take a look at the failing checks?

abishekgiri · 2026-04-06T21:24:51Z

@abishekgiri Could you please take a look at the failing checks?

I traced the failing checks to a shared Skyframe test-fixture issue, not 25 separate failures.

The common compile break was that several tests were constructing RootModuleFileValue directly from InterimModule.builder().build(), which fails because build() is not public from those packages and also trips strict-deps on RootModuleFileValue.

I pushed a fix in 39b2152216 that moves this fixture construction into BzlmodTestUtil and updates the affected Skyframe tests/BUILD deps to use that shared helper. A targeted local bazel test run now gets past analysis and no longer fails on that Java compile error first, though this machine is still blocked by the local Xcode license issue (xcrun code 69).

I’m waiting on the fresh Buildkite rerun to confirm the CI side.

abishekgiri · 2026-04-15T23:28:03Z

@bharadwaj08-one Updated the PR description to Bazel's template and refreshed it to include the follow-up test wiring fix from 176f16734b.

I also re-ran the focused Skyframe targets locally after that patch:
bazel test //src/test/java/com/google/devtools/build/lib/skyframe:FileFunctionTest //src/test/java/com/google/devtools/build/lib/skyframe:ContainingPackageLookupFunctionTest //src/test/java/com/google/devtools/build/lib/skyframe:PackageLookupFunctionTest //src/test/java/com/google/devtools/build/lib/skyframe:LocalRepositoryLookupFunctionTest //src/test/java/com/google/devtools/build/lib/skyframe:RecursiveFilesystemTraversalFunctionTest //src/test/java/com/google/devtools/build/lib/skyframe:GlobFunctionTest --test_output=errors

That focused set now passes locally.

abishekgiri · 2026-04-15T23:52:56Z

I tracked the remaining presubmit failures to repository lookup getting pulled into non-repository paths during package-loading tests. I pushed 5d36c41931, which makes LocalRepositoryLookupFunction check for an actual repo boundary before consulting Bzlmod or repository state, treats symlink errors during that probe as non-repo paths, and updates the local-repo regression fixtures to create a real MODULE.bazel.

Local verification now includes:

bazel test //src/test/java/com/google/devtools/build/lib/skyframe:CollectPackagesUnderDirectoryTest --test_output=errors
bazel test //src/test/java/com/google/devtools/build/lib/skyframe/packages:BazelPackageLoaderTest --test_output=errors
bazel test //src/test/java/com/google/devtools/build/lib/skyframe:LocalRepositoryLookupFunctionTest //src/test/java/com/google/devtools/build/lib/skyframe:PackageLookupFunctionTest --test_output=errors
bazel test //src/test/java/com/google/devtools/build/lib/skyframe/... --build_tests_only --test_output=errors

That broader Skyframe sweep completed successfully on this machine (135 targets).

meteorcloudy · 2026-04-16T09:42:04Z

@abishekgiri Thanks, does this change fix #22208?

abishekgiri · 2026-04-16T19:07:29Z

@meteorcloudy Yes, for the concrete #22208 reproducer, this appears to fix it.

I verified it against the original mbland/bzlmod-local-module-bug example repo at commit 31f1f966f4f2f1ef1c27cfe8ddf77a102fccdfa0.

With the repo’s pinned Bazel 8.0.0-pre.20240415.1, bazel build --nobuild --enable_bzlmod=true //... still fails with the same cannot load '//:top_level_target.bzl' error reported in #22208.

With the Bazel binary built from this PR (5d36c41931), the same command succeeds. I also checked --enable_bzlmod=false, and that still succeeds as well.

So for the bug as reported in #22208, this change fixes the reproducer.

meteorcloudy · 2026-04-17T10:14:05Z

@abishekgiri Thanks for checking!

I also checked --enable_bzlmod=false, and that still succeeds as well.

With Bazel@HEAD, --enable_bzlmod is already a no-op, bzlmod is always enabled. So this is expected.

meteorcloudy · 2026-04-17T10:15:04Z

FYI @katre @fmeum

katre

I have a lot of comments here, but these are mostly just nits and style fixes to make it easier to submit. This is overall a very high-quality change in a very tricky bit of Bazel's code, very well done.

My biggest concern here is the expansion in the number of Skyframe edges due to the extra lookups: my initial read of this (please confirm!) is that this check happens for every subdirectory, which means that in a large monorepo (cough Google's cough) this adds a lot of skytframe edges, which increases memory usage.

(Note: a lot of this is based on my memory of adding the handling for BUILD.bazel files alongside BUILD files back in 2016. Things have changed since then, but even then adding an extra skyframe edge per subdirectory made a big impact.)

However! Last time I saw Google's monorepo, they didn't use local repositories at all. @Wyverald, should we include a flag to disable these checks to use for large monorepos without local repositories?

@Wyverald, regardless of that point, you (or someone with access) absolutely need to run the memory benchmarks on this change. Do we have an open source version? I'm curious what the changes look like just for trying to build Bazel itself. If there's no open source version I may try to cobble something together myself.

katre · 2026-04-17T11:41:26Z

+    if (rootModuleFileValue == null) {
+      return null;
+    }
+    BazelDepGraphValue bazelDepGraphValue = (BazelDepGraphValue) env.getValue(BazelDepGraphValue.KEY);


Instead of doing three env.getValue calls in a row, use getValuesAndExceptions.

katre · 2026-04-17T11:44:53Z

+    for (RootedPath repoBoundaryPath : repoBoundaryPaths) {
+      FileValue fileValue;
+      try {
+        fileValue = (FileValue) env.getValueOrThrow(FileValue.key(repoBoundaryPath), IOException.class);


This is going to do the four (or two, see earlier comment) file lookups in a row, again you should instead use getValuesOrExceptions, and then check those values in the above order.

The only case where that would be a problem would be if this really needs the early-stop behavior: with this implementation, if directory/MODULE.bazel exists, then this never checks for directory/REPO.bazel, and so there's no extra Skyframe edge to that node. There's a benefit there (less memory use for that node), but the semantics are slightly different.

katre · 2026-04-17T11:45:42Z

+        ImmutableList.of(
+            rootedPath(directory, LabelConstants.MODULE_DOT_BAZEL_FILE_NAME),
+            rootedPath(directory, LabelConstants.REPO_FILE_NAME),
+            rootedPath(directory, LabelConstants.WORKSPACE_DOT_BAZEL_FILE_NAME),


Given that in bazel 9 and later, workspace is entirely disabled, why check for these? Do you intend to backport this to earlier branches? Even so, no reason to check for WORKSPACE.bazel or WORKSPACE in bazel 9.

katre · 2026-04-17T11:49:09Z

+  }
+
+  @Nullable
+  private static Boolean directoryHasRepositoryBoundary(Environment env, RootedPath directory)


I really dislike (and, more importantly, Google's Java style guide really dislikes) using an object Boolean just so this can return null to indicate a Skyframe restart.

I'd prefer either a private enum (with values PRESENT, MISSING, or RESTART?), or borrow from ToolchainResolutionFunction.ValueMissingException and use a custom, private exception from all of these to indicate a restart is needed, and then compute can catch that and return null.

katre · 2026-04-17T11:49:47Z

+    return false;
+  }
+
+  private static RootedPath rootedPath(RootedPath directory, PathFragment relativePath) {


Should this be part of RootedPath? Add a TODO comment to possibly move it.

katre · 2026-04-17T11:51:14Z

+  }
+
+  @Nullable
+  private LocalRepositoryLookupValue maybeMatchCommandLineOverrides(


All of these maybe methods are returning a null to indicate "no local repo found", correct? This is a bit confusing because skyframe also uses "null" to indicate "skyframe restart needed", so instead of null, return Optional<LocalRepositoryLookupValue> from all of these.

skyframe: detect local repositories during package lookup

16fcc41

abishekgiri mentioned this pull request Apr 2, 2026

skyframe: detect local repositories during package lookup #29215

Closed

abishekgiri marked this pull request as ready for review April 2, 2026 23:16

abishekgiri requested review from Wyverald and meteorcloudy as code owners April 2, 2026 23:16

skyframe: fix local repository lookup CI build

6caf65e

bazel-docs-pull-requestor Bot mentioned this pull request Apr 3, 2026

docs preview: bazelbuild/bazel#29218 bazel-contrib/bazel-docs#331

Closed

bharadwaj08-one added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Apr 3, 2026

skyframe: share bzlmod root module test fixtures

39b2152

skyframe: fix local repository lookup test wiring

176f167

bazel-docs-pull-requestor Bot mentioned this pull request Apr 15, 2026

docs preview: bazelbuild/bazel#29218 bazel-contrib/bazel-docs#362

Draft

skyframe: avoid repo graph work for non-repo paths

5d36c41

katre suggested changes Apr 17, 2026

View reviewed changes

Conversation

abishekgiri commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation

Build API Changes

Checklist

Release Notes

Uh oh!

abishekgiri commented Apr 2, 2026

Uh oh!

abishekgiri commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bharadwaj08-one commented Apr 3, 2026

Uh oh!

abishekgiri commented Apr 6, 2026

Uh oh!

abishekgiri commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abishekgiri commented Apr 15, 2026

Uh oh!

meteorcloudy commented Apr 16, 2026

Uh oh!

abishekgiri commented Apr 16, 2026

Uh oh!

meteorcloudy commented Apr 17, 2026

Uh oh!

meteorcloudy commented Apr 17, 2026

Uh oh!

katre left a comment

Choose a reason for hiding this comment

Uh oh!

katre Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

katre Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

katre Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

katre Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

katre Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

katre Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

abishekgiri commented Apr 2, 2026 •

edited

Loading

abishekgiri commented Apr 2, 2026 •

edited

Loading

abishekgiri commented Apr 15, 2026 •

edited

Loading