From 1d3f3d3036ad599d8155415202f509f6358443ea Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 26 Jun 2026 11:17:11 +0200 Subject: [PATCH 1/2] Gate external-repo source dir package boundary checks behind a flag Besides fixing the false positives reported in #29688, this also started enforcing a genuinely new class of errors: a source directory in an external repository that crosses a package boundary into a sub-package of that same external repository. Previously the lookup always resolved against the main repository, so such crossings inside external repositories went undetected and the build succeeded. That is an incompatible change: builds that worked on 9.1.1 now fail (see the repro in #29688). Gate the new enforcement behind --incompatible_check_external_repo_source_dir_package_boundary (default off). When the flag is off, package boundary crossings by source directories in external repositories are skipped entirely rather than reverting to the old main-repo lookup, so #29688 stays fixed regardless of the flag value; only the new strictness is deferred. Source directories in the main repository are unaffected. --- .../semantics/BuildLanguageOptions.java | 18 +++++ .../google/devtools/build/lib/skyframe/BUILD | 3 + .../RecursiveFilesystemTraversalFunction.java | 10 +++ .../packages/semantics/ConsistencyTest.java | 4 ++ .../SourceDirectoryIntegrationTest.java | 68 +++++++++++++++++++ 5 files changed, 103 insertions(+) diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java index 211963d86225d1..e768f7238caa92 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java @@ -707,6 +707,19 @@ public abstract class BuildLanguageOptions extends OptionsBase { + " attributes of symbolic macros or attribute default values.") public abstract boolean getIncompatibleSimplifyUnconditionalSelectsInRuleAttrs(); + @Option( + name = "incompatible_check_external_repo_source_dir_package_boundary", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, + help = + "If true, a source directory in an external repository fails the build if it crosses a" + + " package boundary into a sub-package, matching the behavior that already applies" + + " to source directories in the main repository. If false, such package boundary" + + " crossings inside external repositories are not detected.") + public abstract boolean getIncompatibleCheckExternalRepoSourceDirPackageBoundary(); + @Option( name = "experimental_enable_starlark_set", defaultValue = "true", @@ -859,6 +872,9 @@ EXPERIMENTAL_SIBLING_REPOSITORY_LAYOUT, getExperimentalSiblingRepositoryLayout() .setBool( INCOMPATIBLE_ALWAYS_CHECK_DEPSET_ELEMENTS, getIncompatibleAlwaysCheckDepsetElements()) + .setBool( + INCOMPATIBLE_CHECK_EXTERNAL_REPO_SOURCE_DIR_PACKAGE_BOUNDARY, + getIncompatibleCheckExternalRepoSourceDirPackageBoundary()) .setBool(INCOMPATIBLE_DISALLOW_EMPTY_GLOB, getIncompatibleDisallowEmptyGlob()) .setBool( INCOMPATIBLE_PACKAGE_GROUP_HAS_PUBLIC_SYNTAX, @@ -1050,6 +1066,8 @@ public FlagConsumer setBool(String key, boolean ignored) { "-experimental_sibling_repository_layout"; public static final String INCOMPATIBLE_ALWAYS_CHECK_DEPSET_ELEMENTS = "+incompatible_always_check_depset_elements"; + public static final String INCOMPATIBLE_CHECK_EXTERNAL_REPO_SOURCE_DIR_PACKAGE_BOUNDARY = + "-incompatible_check_external_repo_source_dir_package_boundary"; // Note that INCOMPATIBLE_DISALLOW_EMPTY_GLOB differs in Google and in OSS Bazel. public static final String INCOMPATIBLE_DISALLOW_EMPTY_GLOB = "+incompatible_disallow_empty_glob"; diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index eda02b5252ad9e..aa7dd8e4bf9149 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -2292,6 +2292,7 @@ java_library( ":dirents", ":filesystem_keys", ":package_lookup_value", + ":precomputed_value", ":sky_functions", ":tree_artifact_value", "//src/main/java/com/google/devtools/build/lib/actions", @@ -2304,6 +2305,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/io:file_symlink_infinite_expansion_uniqueness_function", "//src/main/java/com/google/devtools/build/lib/io:inconsistent_filesystem_exception", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization:visible-for-serialization", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", @@ -2313,6 +2315,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/com/google/devtools/build/skyframe", "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects", + "//src/main/java/net/starlark/java/eval", "//src/main/protobuf:failure_details_java_proto", "//third_party:auto_value", "//third_party:error_prone_annotations", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java index 857dbdfced6c3d..f5dbe14d9a63f8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java @@ -39,6 +39,7 @@ import com.google.devtools.build.lib.io.FileSymlinkInfiniteExpansionUniquenessFunction; import com.google.devtools.build.lib.io.InconsistentFilesystemException; import com.google.devtools.build.lib.packages.BuildFileNotFoundException; +import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.ProfilerTask; import com.google.devtools.build.lib.profiler.SilentCloseable; @@ -72,6 +73,7 @@ import java.util.List; import java.util.Map; import javax.annotation.Nullable; +import net.starlark.java.eval.StarlarkSemantics; /** A {@link SkyFunction} to build {@link RecursiveFilesystemTraversalValue}s. */ public final class RecursiveFilesystemTraversalFunction implements SkyFunction { @@ -557,6 +559,14 @@ private PkgLookupResult checkIfPackage( // those are symlinks to other locations (e.g. in the case of a local_repository), so all other // roots are part of the main repository. if (rootPath.startsWith(externalDirectory)) { + StarlarkSemantics semantics = PrecomputedValue.STARLARK_SEMANTICS.get(env); + if (semantics == null) { + return null; + } + if (!semantics.getBool( + BuildLanguageOptions.INCOMPATIBLE_CHECK_EXTERNAL_REPO_SOURCE_DIR_PACKAGE_BOUNDARY)) { + return PkgLookupResult.directory(traversal, rootInfo); + } checkState( rootPath.relativeTo(externalDirectory).isSingleSegment(), "%s is expected to be directly under %s", diff --git a/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java index 4e32040d4133aa..ba5057e0daa9f4 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java @@ -140,6 +140,7 @@ private static BuildLanguageOptions buildRandomOptions(Random rand) throws Excep "--experimental_dormant_deps=" + rand.nextBoolean(), "--force_starlark_stack_trace=" + rand.nextBoolean(), "--incompatible_always_check_depset_elements=" + rand.nextBoolean(), + "--incompatible_check_external_repo_source_dir_package_boundary=" + rand.nextBoolean(), "--incompatible_disallow_empty_glob=" + rand.nextBoolean(), "--incompatible_do_not_split_linking_cmdline=" + rand.nextBoolean(), "--incompatible_enable_deprecated_label_apis=" + rand.nextBoolean(), @@ -185,6 +186,9 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) { .setBool(BuildLanguageOptions.EXPERIMENTAL_DORMANT_DEPS, rand.nextBoolean()) .setBool(StarlarkSemantics.FORCE_STARLARK_STACK_TRACE, rand.nextBoolean()) .setBool(BuildLanguageOptions.INCOMPATIBLE_ALWAYS_CHECK_DEPSET_ELEMENTS, rand.nextBoolean()) + .setBool( + BuildLanguageOptions.INCOMPATIBLE_CHECK_EXTERNAL_REPO_SOURCE_DIR_PACKAGE_BOUNDARY, + rand.nextBoolean()) .setBool(BuildLanguageOptions.INCOMPATIBLE_DISALLOW_EMPTY_GLOB, rand.nextBoolean()) .setBool(BuildLanguageOptions.INCOMPATIBLE_DO_NOT_SPLIT_LINKING_CMDLINE, rand.nextBoolean()) .setBool(BuildLanguageOptions.INCOMPATIBLE_ENABLE_DEPRECATED_LABEL_APIS, rand.nextBoolean()) diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java index b15b4d63e49d6f..c0651fa4a55ea8 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java @@ -299,6 +299,74 @@ public void crossingRepoPackageCollision_doesNotFail() throws Exception { buildTarget("//:consume"); } + /** + * With --incompatible_check_external_repo_source_dir_package_boundary, a source directory in an + * external repository that crosses a package boundary into a sub-package of that same repository + * fails the build, just like in the main repository. + */ + @Test + public void crossingExternalRepoPackageBoundary_withFlag_fails() throws Exception { + if (!AnalysisMock.get().isThisBazel()) { + return; + } + setUpCrossingExternalRepoPackageBoundary(); + addOptions("--incompatible_check_external_repo_source_dir_package_boundary"); + + assertThrows(BuildFailedException.class, () -> buildTarget("//:consume")); + assertContainsEvent("crosses package boundary into package rooted at data/sub"); + } + + /** + * Without --incompatible_check_external_repo_source_dir_package_boundary (the default), the same + * package boundary crossing inside an external repository is not detected and the build succeeds. + */ + @Test + public void crossingExternalRepoPackageBoundary_withoutFlag_doesNotFail() throws Exception { + if (!AnalysisMock.get().isThisBazel()) { + return; + } + setUpCrossingExternalRepoPackageBoundary(); + + buildTarget("//:consume"); + } + + private void setUpCrossingExternalRepoPackageBoundary() throws Exception { + write( + "MODULE.bazel", + """ + bazel_dep(name = "ext") + local_path_override( + module_name = "ext", + path = "ext", + ) + """); + write("ext/MODULE.bazel", "module(name = 'ext')"); + write( + "ext/BUILD", + """ + filegroup( + name = "dir", + srcs = ["data"], + visibility = ["//visibility:public"], + ) + """); + // The presence of this BUILD file makes ext's data/sub a package, so the data source directory + // crosses a package boundary. + write("ext/data/sub/BUILD"); + writeIsoLatin1(getWorkspace().getRelative("ext/data/top"), "content"); + + write( + "BUILD", + """ + genrule( + name = "consume", + srcs = ["@ext//:dir"], + outs = ["consume.out"], + cmd = "touch $@", + ) + """); + } + private static final String GENRULE_EVENT = "Executing genrule //foo:foo"; private void assertInvalidatedByBuild() throws Exception { From 15e4c9f84ec161cb7caf6031cd87b372b4689594 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Fri, 26 Jun 2026 18:05:22 +0200 Subject: [PATCH 2/2] Update src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Xùdōng Yáng --- .../build/lib/skyframe/SourceDirectoryIntegrationTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java index c0651fa4a55ea8..5d40515d4a5a8b 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java @@ -350,7 +350,7 @@ private void setUpCrossingExternalRepoPackageBoundary() throws Exception { visibility = ["//visibility:public"], ) """); - // The presence of this BUILD file makes ext's data/sub a package, so the data source directory + // The presence of this BUILD file makes ext's `data/sub` a package, so the `data` source directory // crosses a package boundary. write("ext/data/sub/BUILD"); writeIsoLatin1(getWorkspace().getRelative("ext/data/top"), "content");