Fix BEP error reporting for CTs w/ deps cycles
Before this change, errors for configured targets were not correctly
reported, in particular if they depended on a dependency cycle, but also
if they depended on a configured target with an error in a different
configuration.
The reason is that ConfiguredTargetKey contains a
BuildConfiguration.Key, which does not contain enough information to
construct a BuildConfiguration EventId (for BEP). We are passing the
set of top-level ConfiguredTargetKeys to SkyframeBuildView, but not the
corresponding configurations, even though the BuildView has access to
them (that's how it's constructing the ConfiguredTargetKeys in the first
place).
The ideal fix would be to handle dependency cycle errors in Skyframe and
perform error bubbling (or something like that) to allow the top level
configuration function to catch and report the error. However, this is
not supported by Skyframe at this time.
Instead, we pass in a lookup map that allows recovering the
BuildConfiguration instances corresponding to the top-level configured
targets. In order to save effort in the common case where there is no
analysis error, we only pass in a memoized supplier.
Alternatives considered:
- call Skyframe to recover the BuildConfiguration from the key
- pass in the TargetAndConfiguration instances rather than a lookup map
I don't have any strong reasons to prefer the present approach over
either of the others.
PiperOrigin-RevId: 229568727
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisFailureEvent.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisFailureEvent.java
index cdc26ef..05860cb 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisFailureEvent.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisFailureEvent.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.analysis;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.buildeventstream.BuildEvent;
@@ -52,6 +53,11 @@
return failedTarget;
}
+ @VisibleForTesting
+ BuildEventId getConfigurationId() {
+ return configuration;
+ }
+
/**
* Returns the label of a single root cause. Use {@link #getRootCauses} to report all root causes.
*/
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index 60df775..08c09d4 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -18,6 +18,8 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
+import com.google.common.base.Supplier;
+import com.google.common.base.Suppliers;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
@@ -63,6 +65,7 @@
import com.google.devtools.build.lib.skyframe.AspectValue;
import com.google.devtools.build.lib.skyframe.AspectValue.AspectKey;
import com.google.devtools.build.lib.skyframe.AspectValue.AspectValueKey;
+import com.google.devtools.build.lib.skyframe.BuildConfigurationValue;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
import com.google.devtools.build.lib.skyframe.CoverageReportValue;
import com.google.devtools.build.lib.skyframe.PrepareAnalysisPhaseValue;
@@ -79,9 +82,11 @@
import com.google.devtools.build.skyframe.WalkableGraph;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.List;
+import java.util.Map;
import java.util.Set;
import java.util.logging.Logger;
import java.util.stream.Collectors;
@@ -225,7 +230,7 @@
try (SilentCloseable c = Profiler.instance().profile("Prepare analysis phase")) {
prepareAnalysisPhaseValue = skyframeExecutor.prepareAnalysisPhase(
eventHandler, targetOptions, multiCpu, loadingResult.getTargetLabels());
-
+
// Determine the configurations
configurations =
prepareAnalysisPhaseValue.getConfigurations(eventHandler, skyframeExecutor);
@@ -368,9 +373,26 @@
getArtifactFactory().noteAnalysisStarting();
SkyframeAnalysisResult skyframeAnalysisResult;
try {
+ Supplier<Map<BuildConfigurationValue.Key, BuildConfiguration>> configurationLookupSupplier =
+ () -> {
+ Map<BuildConfigurationValue.Key, BuildConfiguration> result = new HashMap<>();
+ for (TargetAndConfiguration node : topLevelTargetsWithConfigs) {
+ if (node.getConfiguration() != null) {
+ result.put(
+ BuildConfigurationValue.key(node.getConfiguration()), node.getConfiguration());
+ }
+ }
+ return result;
+ };
skyframeAnalysisResult =
skyframeBuildView.configureTargets(
- eventHandler, topLevelCtKeys, aspectKeys, eventBus, keepGoing, loadingPhaseThreads);
+ eventHandler,
+ topLevelCtKeys,
+ aspectKeys,
+ Suppliers.memoize(configurationLookupSupplier),
+ eventBus,
+ keepGoing,
+ loadingPhaseThreads);
setArtifactRoots(skyframeAnalysisResult.getPackageRoots());
} finally {
skyframeBuildView.clearInvalidatedConfiguredTargets();
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
index 6016ddf..2430786 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -52,7 +52,6 @@
import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsDiff;
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
import com.google.devtools.build.lib.analysis.config.FragmentClassSet;
-import com.google.devtools.build.lib.buildeventstream.BuildEventId;
import com.google.devtools.build.lib.buildtool.BuildRequestOptions;
import com.google.devtools.build.lib.causes.Cause;
import com.google.devtools.build.lib.causes.LabelCause;
@@ -352,6 +351,7 @@
ExtendedEventHandler eventHandler,
List<ConfiguredTargetKey> values,
List<AspectValueKey> aspectKeys,
+ Supplier<Map<BuildConfigurationValue.Key, BuildConfiguration>> configurationLookupSupplier,
EventBus eventBus,
boolean keepGoing,
int numThreads)
@@ -440,7 +440,13 @@
}
Pair<Boolean, ViewCreationFailedException> errors =
- processErrors(result, skyframeExecutor, eventHandler, keepGoing, eventBus);
+ processErrors(
+ result,
+ configurationLookupSupplier,
+ skyframeExecutor,
+ eventHandler,
+ keepGoing,
+ eventBus);
Collection<Exception> reportedExceptions = Sets.newHashSet();
for (Map.Entry<ActionAnalysisMetadata, ConflictException> bad : badActions.entrySet()) {
ConflictException ex = bad.getValue();
@@ -511,6 +517,7 @@
*/
static Pair<Boolean, ViewCreationFailedException> processErrors(
EvaluationResult<? extends SkyValue> result,
+ Supplier<Map<BuildConfigurationValue.Key, BuildConfiguration>> configurationLookupSupplier,
SkyframeExecutor skyframeExecutor,
ExtendedEventHandler eventHandler,
boolean keepGoing,
@@ -557,7 +564,6 @@
ConfiguredTargetKey label = (ConfiguredTargetKey) errorKey.argument();
Label topLevelLabel = label.getLabel();
- BuildEventId configuration = null;
Iterable<Cause> rootCauses;
if (cause instanceof ConfiguredValueCreationException) {
ConfiguredValueCreationException ctCause = (ConfiguredValueCreationException) cause;
@@ -580,7 +586,6 @@
}
}
rootCauses = ctCause.getRootCauses();
- configuration = ctCause.getConfiguration();
} else if (!Iterables.isEmpty(errorInfo.getCycleInfo())) {
Label analysisRootCause = maybeGetConfiguredTargetCycleCulprit(
topLevelLabel, errorInfo.getCycleInfo());
@@ -611,11 +616,12 @@
noKeepGoingException = new ViewCreationFailedException(errorMsg);
}
}
- ConfiguredTargetKey configuredTargetKey =
- ConfiguredTargetKey.of(
- topLevelLabel, label.getConfigurationKey(), label.isHostConfiguration());
if (!inTest) {
- eventBus.post(new AnalysisFailureEvent(configuredTargetKey, configuration, rootCauses));
+ BuildConfiguration configuration =
+ configurationLookupSupplier.get().get(label.getConfigurationKey());
+ eventBus.post(
+ new AnalysisFailureEvent(
+ label, configuration == null ? null : configuration.getEventId(), rootCauses));
}
}
return Pair.of(hasLoadingError, noKeepGoingException);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 4e31909..4f93086 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -1647,11 +1647,21 @@
aliasPackageValues = evaluateSkyKeys(eventHandler, aliasPackagesToFetch);
keysToProcess = aliasKeysToRedo;
}
+ Supplier<Map<BuildConfigurationValue.Key, BuildConfiguration>> configurationLookupSupplier =
+ () ->
+ configs.values().stream()
+ .collect(
+ Collectors.toMap(
+ BuildConfigurationValue::key, java.util.function.Function.identity()));
// We ignore the return value here because tests effectively run with --keep_going, and the
// loading-phase-error bit is only needed if we're constructing a SkyframeAnalysisResult.
SkyframeBuildView.processErrors(
- result, this, eventHandler, /*keepGoing=*/ true, /*eventBus=*/ null);
-
+ result,
+ configurationLookupSupplier,
+ this,
+ eventHandler,
+ /*keepGoing=*/ true,
+ /*eventBus=*/ null);
return cts.build();
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
index 128628d..2d17f19 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
@@ -39,6 +39,7 @@
import com.google.devtools.build.lib.analysis.util.BuildViewTestBase;
import com.google.devtools.build.lib.analysis.util.ExpectedTrimmedConfigurationErrors;
import com.google.devtools.build.lib.analysis.util.MockRule;
+import com.google.devtools.build.lib.buildeventstream.NullConfiguration;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.OutputFilter.RegexOutputFilter;
import com.google.devtools.build.lib.packages.BuildType;
@@ -224,6 +225,22 @@
}
@Test
+ public void testAnalysisReportsDependencyCycle() throws Exception {
+ scratch.file("foo/BUILD", "sh_library(name='foo',deps=['//bar'])");
+ scratch.file("bar/BUILD", "sh_library(name='bar',deps=[':bar'])");
+
+ reporter.removeHandler(failFastHandler);
+ EventBus eventBus = new EventBus();
+ AnalysisFailureRecorder recorder = new AnalysisFailureRecorder();
+ eventBus.register(recorder);
+ AnalysisResult result = update(eventBus, defaultFlags().with(Flag.KEEP_GOING), "//foo");
+ assertThat(result.hasError()).isTrue();
+ assertThat(recorder.events).hasSize(1);
+ AnalysisFailureEvent event = recorder.events.get(0);
+ assertThat(event.getConfigurationId()).isNotEqualTo(NullConfiguration.INSTANCE.getEventId());
+ }
+
+ @Test
public void testReportsLoadingRootCauses() throws Exception {
// This test checks that two simultaneous errors are both reported:
// - missing outs attribute,
@@ -1404,5 +1421,9 @@
@Test
public void testErrorBelowCycle() {
}
+
+ @Override
+ @Test
+ public void testAnalysisReportsDependencyCycle() {}
}
}
