Fix non-determinism in the `FailureDetail` produced for a package with multiple label crosses subpackage boundary errors.
While I'm here, also make the code a tiny bit simpler:
* Explicitly create the empty inner `ArrayList` on each loop iteration. There's no need to use `#computeIfAbsent`; we know no other loop iteration will be processing the same `Target`.
* Use a `List<Pair<Target, List<PackageLookupValue.Key>>>` instead of a `Map<Target, List<PackageLookupValue.Key>>` since we don't need lookup semantics.
PiperOrigin-RevId: 533585562
Change-Id: Iaa3c3d9302a0eef533b55d1f17c33de3ec23666b
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 ae8ba7b..30d84e8 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 @@
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<Target, List<PackageLookupValue.Key>> targetSubpackagePackageLookupKeyMap = new HashMap<>();
+ List<Pair<Target, List<PackageLookupValue.Key>>> targetsAndSubpackagePackageLookupKeys =
+ new ArrayList<>();
Set<PackageLookupValue.Key> allPackageLookupKeys = new HashSet<>();
for (Target target : pkgBuilder.getTargets()) {
Label label = target.getLabel();
@@ -813,6 +814,7 @@
if (dir.equals(pkgDir)) {
continue;
}
+ List<PackageLookupValue.Key> subpackagePackageLookupKeys = new ArrayList<>();
String labelName = label.getName();
PathFragment labelAsRelativePath = PathFragment.create(labelName).getParentDirectory();
PathFragment subpackagePath = pkgDir;
@@ -821,14 +823,13 @@
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 @@
return;
}
- for (Map.Entry<Target, List<PackageLookupValue.Key>> entry :
- targetSubpackagePackageLookupKeyMap.entrySet()) {
- List<PackageLookupValue.Key> targetPackageLookupKeys = entry.getValue();
+ for (Pair<Target, List<PackageLookupValue.Key>> targetAndSubpackagePackageLookupKeys :
+ targetsAndSubpackagePackageLookupKeys) {
+ Target target = targetAndSubpackagePackageLookupKeys.getFirst();
+ List<PackageLookupValue.Key> 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 @@
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 a654c05..a127446 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 @@
"//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 56cb0a2..6b5d620 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 @@
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");
+ }
}
