diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java index ae8ba7b1c789bb..30d84e8692a289 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java @@ -805,7 +805,8 @@ private static void handleLabelsCrossingSubpackagesAndPropagateInconsistentFiles PathFragment pkgDir = pkgId.getPackageFragment(); // Contains a key for each package whose label that might have a presence of a subpackage. // Values are all potential subpackages of the label. - Map> targetSubpackagePackageLookupKeyMap = new HashMap<>(); + List>> targetsAndSubpackagePackageLookupKeys = + new ArrayList<>(); Set allPackageLookupKeys = new HashSet<>(); for (Target target : pkgBuilder.getTargets()) { Label label = target.getLabel(); @@ -813,6 +814,7 @@ private static void handleLabelsCrossingSubpackagesAndPropagateInconsistentFiles if (dir.equals(pkgDir)) { continue; } + List subpackagePackageLookupKeys = new ArrayList<>(); String labelName = label.getName(); PathFragment labelAsRelativePath = PathFragment.create(labelName).getParentDirectory(); PathFragment subpackagePath = pkgDir; @@ -821,14 +823,13 @@ private static void handleLabelsCrossingSubpackagesAndPropagateInconsistentFiles subpackagePath = subpackagePath.getRelative(segment); PackageLookupValue.Key currentPackageLookupKey = PackageLookupValue.key(PackageIdentifier.create(pkgId.getRepository(), subpackagePath)); - targetSubpackagePackageLookupKeyMap - .computeIfAbsent(target, t -> new ArrayList<>()) - .add(currentPackageLookupKey); + subpackagePackageLookupKeys.add(currentPackageLookupKey); allPackageLookupKeys.add(currentPackageLookupKey); } + targetsAndSubpackagePackageLookupKeys.add(Pair.of(target, subpackagePackageLookupKeys)); } - if (targetSubpackagePackageLookupKeyMap.isEmpty()) { + if (targetsAndSubpackagePackageLookupKeys.isEmpty()) { return; } @@ -837,9 +838,11 @@ private static void handleLabelsCrossingSubpackagesAndPropagateInconsistentFiles return; } - for (Map.Entry> entry : - targetSubpackagePackageLookupKeyMap.entrySet()) { - List targetPackageLookupKeys = entry.getValue(); + for (Pair> targetAndSubpackagePackageLookupKeys : + targetsAndSubpackagePackageLookupKeys) { + Target target = targetAndSubpackagePackageLookupKeys.getFirst(); + List targetPackageLookupKeys = + targetAndSubpackagePackageLookupKeys.getSecond(); // Iterate from the deepest potential subpackage to the shallowest in that we only want to // display the deepest subpackage in the error message for each target. for (PackageLookupValue.Key packageLookupKey : Lists.reverse(targetPackageLookupKeys)) { @@ -858,10 +861,9 @@ private static void handleLabelsCrossingSubpackagesAndPropagateInconsistentFiles throw new InternalInconsistentFilesystemException(pkgId, e); } - Target target = entry.getKey(); if (maybeAddEventAboutLabelCrossingSubpackage( pkgBuilder, pkgRoot, target, packageLookupKey.argument(), packageLookupValue)) { - pkgBuilder.getTargets().remove(entry.getKey()); + pkgBuilder.getTargets().remove(target); pkgBuilder.setContainsErrors(); break; } diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/BUILD b/src/test/java/com/google/devtools/build/lib/pkgcache/BUILD index a654c0505ad604..a127446ef7438b 100644 --- a/src/test/java/com/google/devtools/build/lib/pkgcache/BUILD +++ b/src/test/java/com/google/devtools/build/lib/pkgcache/BUILD @@ -181,6 +181,7 @@ java_test( "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/common/options", "//src/main/java/net/starlark/java/syntax", + "//src/main/protobuf:failure_details_java_proto", "//src/test/java/com/google/devtools/build/lib/analysis/util", "//src/test/java/com/google/devtools/build/lib/testutil", "//src/test/java/com/google/devtools/build/lib/testutil:JunitUtils", diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/PackageLoadingTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/PackageLoadingTest.java index 56cb0a2c992c9b..6b5d620d56b231 100644 --- a/src/test/java/com/google/devtools/build/lib/pkgcache/PackageLoadingTest.java +++ b/src/test/java/com/google/devtools/build/lib/pkgcache/PackageLoadingTest.java @@ -40,6 +40,7 @@ import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction; import com.google.devtools.build.lib.runtime.QuiescingExecutorsImpl; +import com.google.devtools.build.lib.server.FailureDetails.PackageLoading; import com.google.devtools.build.lib.skyframe.BazelSkyframeExecutorConstants; import com.google.devtools.build.lib.skyframe.PrecomputedValue; import com.google.devtools.build.lib.skyframe.SkyframeExecutor; @@ -578,4 +579,18 @@ public void testDeterminismOfInputFileLocation() throws Exception { InputFile f = (InputFile) p.getTarget("f.sh"); assertThat(f.getLocation().line()).isEqualTo(1); } + + @Test + public void testDeterminismOfFailureDetailOnMultipleLabelCrossingSubpackageBoundaryErrors() + throws Exception { + reporter.removeHandler(failFastHandler); + scratch.file("p/sub/BUILD"); + scratch.file("p/BUILD", "sh_library(name = 'sub/a')", "sh_library(name = 'sub/b')"); + Package p = getPackage("p"); + assertThat(p.getFailureDetail().getPackageLoading().getCode()) + .isEqualTo(PackageLoading.Code.LABEL_CROSSES_PACKAGE_BOUNDARY); + // We used to non-deterministically pick a target whose label crossed a subpackage boundary, but + // now we deterministically pick the first one (alphabetically by target name). + assertThat(p.getFailureDetail().getMessage()).startsWith("Label '//p:sub/a' is invalid"); + } }