Make deterministic the error that is stored in TransitiveTraversalValue. When there are multiple errors, we don't want non-determinism.
PiperOrigin-RevId: 240208756
diff --git a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
index 3d56f5b..5184e8e 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/SkyQueryEnvironment.java
@@ -892,9 +892,9 @@
for (Map.Entry<SkyKey, SkyValue> successfulEntry : successfulEntries) {
successfulKeysBuilder.add(successfulEntry.getKey());
TransitiveTraversalValue value = (TransitiveTraversalValue) successfulEntry.getValue();
- String firstErrorMessage = value.getFirstErrorMessage();
- if (firstErrorMessage != null) {
- errorMessagesBuilder.add(firstErrorMessage);
+ String errorMessage = value.getErrorMessage();
+ if (errorMessage != null) {
+ errorMessagesBuilder.add(errorMessage);
}
}
ImmutableSet<SkyKey> successfulKeys = successfulKeysBuilder.build();
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java
index 3f72cf9..b7d8b43 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java
@@ -21,13 +21,13 @@
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.NoSuchThingException;
-import com.google.devtools.build.lib.skyframe.TransitiveTraversalFunction.FirstErrorMessageAccumulator;
import com.google.devtools.build.lib.util.GroupedList;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.build.skyframe.ValueOrException2;
import java.util.Collection;
+import java.util.Comparator;
import java.util.List;
import java.util.Map;
import javax.annotation.Nullable;
@@ -35,12 +35,13 @@
/**
* This class is like {@link TransitiveTargetFunction}, but the values it returns do not contain
* {@link NestedSet}s. It performs the side-effects of {@link TransitiveTargetFunction} (i.e.,
- * ensuring that transitive targets and their packages have been loaded). It evaluates to a
- * {@link TransitiveTraversalValue} that contains the first error message it encountered, and a
- * set of names of providers if the target is a rule.
+ * ensuring that transitive targets and their packages have been loaded). It evaluates to a {@link
+ * TransitiveTraversalValue} that contains the first error message it encountered, and a set of
+ * names of providers if the target is a rule.
*/
public class TransitiveTraversalFunction
- extends TransitiveBaseTraversalFunction<FirstErrorMessageAccumulator> {
+ extends TransitiveBaseTraversalFunction<
+ TransitiveTraversalFunction.DeterministicErrorMessageAccumulator> {
@Override
Label argumentFromKey(SkyKey key) {
@@ -53,15 +54,16 @@
}
@Override
- FirstErrorMessageAccumulator processTarget(Label label, TargetAndErrorIfAny targetAndErrorIfAny) {
+ DeterministicErrorMessageAccumulator processTarget(
+ Label label, TargetAndErrorIfAny targetAndErrorIfAny) {
NoSuchTargetException errorIfAny = targetAndErrorIfAny.getErrorLoadingTarget();
String errorMessageIfAny = errorIfAny == null ? null : errorIfAny.getMessage();
- return new FirstErrorMessageAccumulator(errorMessageIfAny);
+ return DeterministicErrorMessageAccumulator.create(errorMessageIfAny);
}
@Override
void processDeps(
- FirstErrorMessageAccumulator accumulator,
+ DeterministicErrorMessageAccumulator accumulator,
EventHandler eventHandler,
TargetAndErrorIfAny targetAndErrorIfAny,
Iterable<Map.Entry<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>>>
@@ -78,9 +80,9 @@
accumulator.maybeSet(e.getMessage());
continue;
}
- String firstErrorMessage = transitiveTraversalValue.getFirstErrorMessage();
- if (firstErrorMessage != null) {
- accumulator.maybeSet(firstErrorMessage);
+ String errorMessage = transitiveTraversalValue.getErrorMessage();
+ if (errorMessage != null) {
+ accumulator.maybeSet(errorMessage);
}
}
}
@@ -104,14 +106,14 @@
}
@Override
- SkyValue computeSkyValue(TargetAndErrorIfAny targetAndErrorIfAny,
- FirstErrorMessageAccumulator accumulator) {
+ SkyValue computeSkyValue(
+ TargetAndErrorIfAny targetAndErrorIfAny, DeterministicErrorMessageAccumulator accumulator) {
boolean targetLoadedSuccessfully = targetAndErrorIfAny.getErrorLoadingTarget() == null;
- String firstErrorMessage = accumulator.getFirstErrorMessage();
+ String errorMessage = accumulator.getErrorMessage();
return targetLoadedSuccessfully
- ? TransitiveTraversalValue.forTarget(targetAndErrorIfAny.getTarget(), firstErrorMessage)
+ ? TransitiveTraversalValue.forTarget(targetAndErrorIfAny.getTarget(), errorMessage)
: TransitiveTraversalValue.unsuccessfulTransitiveTraversal(
- firstErrorMessage, targetAndErrorIfAny.getTarget());
+ errorMessage, targetAndErrorIfAny.getTarget());
}
@Override
@@ -178,28 +180,47 @@
}
/**
- * Keeps track of the first error message encountered while traversing itself and its
- * dependencies.
+ * Keeps track of a deterministic error message encountered while traversing itself and its
+ * dependencies: either the error it was initialized with, or the shortest error it encounters,
+ * with ties broken alphabetically.
+ *
+ * <p>This preserves the behavior that the local target's error is the most important, and is
+ * cheap (constant-time) to compute the comparison between strings, unless they have the same
+ * length, which is unlikely.
*/
- static class FirstErrorMessageAccumulator {
-
- @Nullable private String firstErrorMessage;
-
- public FirstErrorMessageAccumulator(@Nullable String firstErrorMessage) {
- this.firstErrorMessage = firstErrorMessage;
- }
-
- /** Remembers {@param errorMessage} if it is the first error message. */
- void maybeSet(String errorMessage) {
- Preconditions.checkNotNull(errorMessage);
- if (firstErrorMessage == null) {
- firstErrorMessage = errorMessage;
- }
- }
-
+ interface DeterministicErrorMessageAccumulator {
@Nullable
- String getFirstErrorMessage() {
- return firstErrorMessage;
+ String getErrorMessage();
+
+ default void maybeSet(String errorMessage) {}
+
+ static DeterministicErrorMessageAccumulator create(@Nullable String errorMessage) {
+ if (errorMessage != null) {
+ return () -> errorMessage;
+ }
+ return new UpdateableErrorMessageAccumulator();
+ }
+
+ class UpdateableErrorMessageAccumulator implements DeterministicErrorMessageAccumulator {
+ private static final Comparator<String> LENGTH_THEN_ALPHABETICAL =
+ Comparator.nullsLast(
+ Comparator.comparingInt(String::length).thenComparing(Comparator.naturalOrder()));
+
+ @Nullable private String errorMessage;
+
+ @Override
+ public void maybeSet(String errorMessage) {
+ Preconditions.checkNotNull(errorMessage);
+ if (LENGTH_THEN_ALPHABETICAL.compare(this.errorMessage, errorMessage) > 0) {
+ this.errorMessage = errorMessage;
+ }
+ }
+
+ @Nullable
+ @Override
+ public String getErrorMessage() {
+ return errorMessage;
+ }
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java
index 655cadd..be9c828 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java
@@ -62,20 +62,18 @@
}
static TransitiveTraversalValue unsuccessfulTransitiveTraversal(
- String firstErrorMessage, Target target) {
+ String errorMessage, Target target) {
return new TransitiveTraversalValueWithError(
- Preconditions.checkNotNull(firstErrorMessage), target.getTargetKind());
+ Preconditions.checkNotNull(errorMessage), target.getTargetKind());
}
- static TransitiveTraversalValue forTarget(Target target, @Nullable String firstErrorMessage) {
- if (firstErrorMessage == null) {
+ static TransitiveTraversalValue forTarget(Target target, @Nullable String errorMessage) {
+ if (errorMessage == null) {
if (target instanceof Rule && ((Rule) target).getRuleClassObject().isSkylark()) {
Rule rule = (Rule) target;
// Do not intern values for skylark rules.
return TransitiveTraversalValue.create(
- rule.getRuleClassObject().getAdvertisedProviders(),
- rule.getTargetKind(),
- firstErrorMessage);
+ rule.getRuleClassObject().getAdvertisedProviders(), rule.getTargetKind(), errorMessage);
} else {
TransitiveTraversalValue value = VALUES_BY_TARGET_KIND.get(target.getTargetKind());
if (value != null) {
@@ -95,18 +93,18 @@
return value;
}
} else {
- return new TransitiveTraversalValueWithError(firstErrorMessage, target.getTargetKind());
+ return new TransitiveTraversalValueWithError(errorMessage, target.getTargetKind());
}
}
@AutoCodec.Instantiator
public static TransitiveTraversalValue create(
- AdvertisedProviderSet providers, String kind, @Nullable String firstErrorMessage) {
+ AdvertisedProviderSet providers, String kind, @Nullable String errorMessage) {
TransitiveTraversalValue value =
- firstErrorMessage == null
+ errorMessage == null
? new TransitiveTraversalValueWithoutError(providers, kind)
- : new TransitiveTraversalValueWithError(firstErrorMessage, kind);
- if (firstErrorMessage == null) {
+ : new TransitiveTraversalValueWithError(errorMessage, kind);
+ if (errorMessage == null) {
TransitiveTraversalValue oldValue = VALUE_INTERNER.getCanonical(value);
return oldValue == null ? value : oldValue;
}
@@ -128,11 +126,11 @@
}
/**
- * Returns the first error message, if any, from loading the target and its transitive
+ * Returns a deterministic error message, if any, from loading the target and its transitive
* dependencies.
*/
@Nullable
- public abstract String getFirstErrorMessage();
+ public abstract String getErrorMessage();
@Override
public boolean equals(Object o) {
@@ -143,14 +141,14 @@
return false;
}
TransitiveTraversalValue that = (TransitiveTraversalValue) o;
- return Objects.equals(this.getFirstErrorMessage(), that.getFirstErrorMessage())
+ return Objects.equals(this.getErrorMessage(), that.getErrorMessage())
&& Objects.equals(this.getKind(), that.getKind())
&& this.getProviders().equals(that.getProviders());
}
@Override
public int hashCode() {
- return Objects.hash(getFirstErrorMessage(), getKind(), getProviders());
+ return Objects.hash(getErrorMessage(), getKind(), getProviders());
}
@ThreadSafe
@@ -181,7 +179,7 @@
@Override
@Nullable
- public String getFirstErrorMessage() {
+ public String getErrorMessage() {
return null;
}
@@ -196,12 +194,11 @@
/** A transitive target reference with error. */
public static final class TransitiveTraversalValueWithError extends TransitiveTraversalValue {
- private final String firstErrorMessage;
+ private final String errorMessage;
- private TransitiveTraversalValueWithError(String firstErrorMessage, String kind) {
+ private TransitiveTraversalValueWithError(String errorMessage, String kind) {
super(kind);
- this.firstErrorMessage =
- StringCanonicalizer.intern(Preconditions.checkNotNull(firstErrorMessage));
+ this.errorMessage = StringCanonicalizer.intern(Preconditions.checkNotNull(errorMessage));
}
@Override
@@ -216,14 +213,14 @@
@Override
@Nullable
- public String getFirstErrorMessage() {
- return firstErrorMessage;
+ public String getErrorMessage() {
+ return errorMessage;
}
@Override
public String toString() {
return MoreObjects.toStringHelper(this)
- .add("error", firstErrorMessage)
+ .add("error", errorMessage)
.add("kind", getKind())
.toString();
}
diff --git a/src/main/java/com/google/devtools/build/skyframe/ValueOrException2.java b/src/main/java/com/google/devtools/build/skyframe/ValueOrException2.java
index 7c8bdc3..4351c56 100644
--- a/src/main/java/com/google/devtools/build/skyframe/ValueOrException2.java
+++ b/src/main/java/com/google/devtools/build/skyframe/ValueOrException2.java
@@ -13,6 +13,7 @@
// limitations under the License.
package com.google.devtools.build.skyframe;
+import com.google.common.annotations.VisibleForTesting;
import javax.annotation.Nullable;
/** Wrapper for a value or the typed exception thrown when trying to compute it. */
@@ -23,7 +24,8 @@
@Nullable
public abstract SkyValue get() throws E1, E2;
- static <E1 extends Exception, E2 extends Exception>
+ @VisibleForTesting
+ public static <E1 extends Exception, E2 extends Exception>
ValueOrException2<E1, E2> fromUntypedException(
ValueOrUntypedException voe, Class<E1> exceptionClass1, Class<E2> exceptionClass2) {
SkyValue value = voe.getValue();
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunctionTest.java
index 0fa3e05..9ca9b5d 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunctionTest.java
@@ -31,6 +31,8 @@
import com.google.devtools.build.lib.vfs.RootedPath;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyKey;
+import com.google.devtools.build.skyframe.ValueOrException2;
+import com.google.devtools.build.skyframe.ValueOrUntypedException;
import java.util.concurrent.atomic.AtomicBoolean;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -98,6 +100,101 @@
assertThat(wasOptimizationUsed.get()).isTrue();
}
+ @Test
+ public void multipleErrorsForTransitiveTraversalFunction() throws Exception {
+ Label label = Label.parseAbsolute("//foo:foo", ImmutableMap.of());
+ Package pkg =
+ scratchPackage(
+ "workspace",
+ label.getPackageIdentifier(),
+ "sh_library(name = '" + label.getName() + "', deps = [':bar', ':baz'])");
+ TargetAndErrorIfAnyImpl targetAndErrorIfAny =
+ new TargetAndErrorIfAnyImpl(
+ /*packageLoadedSuccessfully=*/ true,
+ /*errorLoadingTarget=*/ null,
+ pkg.getTarget(label.getName()));
+ TransitiveTraversalFunction function =
+ new TransitiveTraversalFunction() {
+ @Override
+ LoadTargetResults loadTarget(Environment env, Label label) {
+ return targetAndErrorIfAny;
+ }
+ };
+ SkyKey dep1 = function.getKey(Label.parseAbsolute("//foo:bar", ImmutableMap.of()));
+ SkyKey dep2 = function.getKey(Label.parseAbsolute("//foo:baz", ImmutableMap.of()));
+ ImmutableMap<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>>
+ returnedDeps =
+ ImmutableMap.of(dep1, makeException("bad bar"), dep2, makeException("bad baz"));
+ SkyFunction.Environment mockEnv = Mockito.mock(SkyFunction.Environment.class);
+ // Try two evaluations, with the environment reversing the order of the map it returns.
+ when(mockEnv.getValuesOrThrow(
+ Mockito.any(),
+ Mockito.eq(NoSuchPackageException.class),
+ Mockito.eq(NoSuchTargetException.class)))
+ .thenReturn(returnedDeps);
+ when(mockEnv.valuesMissing()).thenReturn(false);
+
+ assertThat(
+ ((TransitiveTraversalValue) function.compute(function.getKey(label), mockEnv))
+ .getErrorMessage())
+ .isEqualTo("bad bar");
+ ImmutableMap<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>>
+ reversedDeps =
+ ImmutableMap.of(dep2, makeException("bad baz"), dep1, makeException("bad bar"));
+ when(mockEnv.getValuesOrThrow(
+ Mockito.any(),
+ Mockito.eq(NoSuchPackageException.class),
+ Mockito.eq(NoSuchTargetException.class)))
+ .thenReturn(reversedDeps);
+ assertThat(
+ ((TransitiveTraversalValue) function.compute(function.getKey(label), mockEnv))
+ .getErrorMessage())
+ .isEqualTo("bad bar");
+ }
+
+ @Test
+ public void selfErrorWins() throws Exception {
+ Label label = Label.parseAbsolute("//foo:foo", ImmutableMap.of());
+ Package pkg =
+ scratchPackage(
+ "workspace",
+ label.getPackageIdentifier(),
+ "sh_library(name = '" + label.getName() + "', deps = [':bar', ':baz'])");
+ TargetAndErrorIfAnyImpl targetAndErrorIfAny =
+ new TargetAndErrorIfAnyImpl(
+ /*packageLoadedSuccessfully=*/ true,
+ /*errorLoadingTarget=*/ new NoSuchTargetException("self error is long and last"),
+ pkg.getTarget(label.getName()));
+ TransitiveTraversalFunction function =
+ new TransitiveTraversalFunction() {
+ @Override
+ LoadTargetResults loadTarget(Environment env, Label label) {
+ return targetAndErrorIfAny;
+ }
+ };
+ SkyKey dep = function.getKey(Label.parseAbsolute("//foo:bar", ImmutableMap.of()));
+ SkyFunction.Environment mockEnv = Mockito.mock(SkyFunction.Environment.class);
+ when(mockEnv.getValuesOrThrow(
+ Mockito.any(),
+ Mockito.eq(NoSuchPackageException.class),
+ Mockito.eq(NoSuchTargetException.class)))
+ .thenReturn(ImmutableMap.of(dep, makeException("bad bar")));
+ when(mockEnv.valuesMissing()).thenReturn(false);
+
+ assertThat(
+ ((TransitiveTraversalValue) function.compute(function.getKey(label), mockEnv))
+ .getErrorMessage())
+ .isEqualTo("self error is long and last");
+ }
+
+ private static ValueOrException2<NoSuchPackageException, NoSuchTargetException> makeException(
+ String errorMessage) {
+ ValueOrUntypedException exn =
+ ValueOrUntypedException.ofExn(new NoSuchTargetException(errorMessage));
+ return ValueOrException2.fromUntypedException(
+ exn, NoSuchPackageException.class, NoSuchTargetException.class);
+ }
+
private Package scratchPackage(String workspaceName, PackageIdentifier packageId, String... lines)
throws Exception {
Path buildFile = scratch.file("" + packageId.getSourceRoot() + "/BUILD", lines);
