Fix a failure to detect action conflicts if the action conflict came about via a changed aspect that didn't change any configured targets.
Realized that that failure also indicated we weren't properly accounting for aspects in our metrics. Made our metrics count them, and added in some additional fields that allow people to track the old thing if they care, or (more likely) to ease the transition from old to new: I saw 50% increases for some metrics when aspect data was added in.
Also realized that we were storing a set of configured target keys during analysis for no good reason, and deleted that to save memory. The only casualty is the targets_loaded field, which was bogus anyway. unknown commit added it without anyone seeming to realize that it was not counting "loaded" targets, but rather "analyzed labels", which is not a particularly useful metric.
RELNOTES[INC]: In the build event stream, BuildMetrics.TargetMetrics.targets_loaded is no longer populated. Its value was always mostly meaningless. BuildMetrics.TargetMetrics.targets_configured and BuildMetrics.ActionSummary.actions_created now include configured aspect data.
PiperOrigin-RevId: 357959578
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
index f1a4d12..d2a97a5 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
@@ -920,4 +920,77 @@
setRulesAndAspectsAvailableInTests(ImmutableList.of(), ImmutableList.of());
getConfiguredTarget("//a:r");
}
+
+ @Test
+ public void topLevelConflictDetected() throws Exception {
+ String bzlFileTemplate =
+ String.join(
+ "\n",
+ "def _aspect1_impl(target, ctx):",
+ " outfile = ctx.actions.declare_file('aspect.out')",
+ " ctx.actions.run_shell(",
+ " outputs = [outfile],",
+ " progress_message = 'Action for aspect 1',",
+ " command = 'echo \"1\" > ' + outfile.path,",
+ " )",
+ " return [OutputGroupInfo(files = [outfile])]",
+ "def _aspect2_impl(target, ctx):",
+ " outfile = ctx.actions.declare_file('aspect.out')",
+ " ctx.actions.run_shell(",
+ " outputs = [outfile],",
+ " progress_message = 'Action for aspect 2',",
+ " command = 'echo \"%s\" > ' + outfile.path,",
+ " )",
+ " return [OutputGroupInfo(files = [outfile])]",
+ "aspect1 = aspect(implementation = _aspect1_impl)",
+ "aspect2 = aspect(implementation = _aspect2_impl)");
+ scratch.file("foo/aspect.bzl", String.format(bzlFileTemplate, "2"));
+ scratch.file("foo/BUILD", "sh_library(name = 'foo', srcs = ['foo.sh'])");
+ // Expect errors.
+ reporter.removeHandler(failFastHandler);
+ ViewCreationFailedException exception =
+ assertThrows(
+ ViewCreationFailedException.class,
+ () ->
+ update(
+ new EventBus(),
+ defaultFlags(),
+ ImmutableList.of("//foo:aspect.bzl%aspect1", "//foo:aspect.bzl%aspect2"),
+ "//foo:foo"));
+ assertThat(exception)
+ .hasMessageThat()
+ .contains(
+ "ConflictException: for foo/aspect.out, previous action: action 'Action for aspect 1',"
+ + " attempted action: action 'Action for aspect 2'");
+
+ // Fix bzl file so actions are shared: analysis should succeed now.
+ scratch.overwriteFile("foo/aspect.bzl", String.format(bzlFileTemplate, "1"));
+ reporter.addHandler(failFastHandler);
+ AnalysisResult result =
+ update(
+ new EventBus(),
+ defaultFlags(),
+ ImmutableList.of("//foo:aspect.bzl%aspect1", "//foo:aspect.bzl%aspect2"),
+ "//foo:foo");
+ assertThat(result.getAspectsMap()).hasSize(2);
+
+ // Break bzl file again: we should notice.
+ scratch.overwriteFile("foo/aspect.bzl", String.format(bzlFileTemplate, "2"));
+ // Expect errors.
+ reporter.removeHandler(failFastHandler);
+ exception =
+ assertThrows(
+ ViewCreationFailedException.class,
+ () ->
+ update(
+ new EventBus(),
+ defaultFlags(),
+ ImmutableList.of("//foo:aspect.bzl%aspect1", "//foo:aspect.bzl%aspect2"),
+ "//foo:foo"));
+ assertThat(exception)
+ .hasMessageThat()
+ .contains(
+ "ConflictException: for foo/aspect.out, previous action: action 'Action for aspect 1',"
+ + " attempted action: action 'Action for aspect 2'");
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/BUILD
index 88c941b..555f696 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/analysis/BUILD
@@ -38,6 +38,7 @@
deps = [
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:action_lookup_key",
+ "//src/main/java/com/google/devtools/build/lib/actions:artifact_owner",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
"//src/main/java/com/google/devtools/build/lib/actions:execution_requirements",
@@ -69,7 +70,6 @@
"//src/main/java/com/google/devtools/build/lib/analysis:config/execution_transition_factory",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
- "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_provider",
"//src/main/java/com/google/devtools/build/lib/analysis:config/host_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/per_label_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/run_under",
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/test/TrimTestConfigurationTest.java b/src/test/java/com/google/devtools/build/lib/analysis/test/TrimTestConfigurationTest.java
index 95d72e7..f52a4ee 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/test/TrimTestConfigurationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/test/TrimTestConfigurationTest.java
@@ -22,7 +22,9 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableMultiset;
+import com.google.devtools.build.lib.actions.ActionLookupKey;
import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.ArtifactOwner;
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
import com.google.devtools.build.lib.analysis.BaseRuleClasses;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
@@ -45,7 +47,6 @@
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
-import com.google.devtools.build.skyframe.SkyKey;
import java.util.Arrays;
import java.util.LinkedHashSet;
import java.util.Map;
@@ -138,11 +139,11 @@
}
private void assertNumberOfConfigurationsOfTargets(
- Set<SkyKey> keys, Map<String, Integer> targetsWithCounts) {
+ Set<ActionLookupKey> keys, Map<String, Integer> targetsWithCounts) {
ImmutableMultiset<Label> actualSet =
keys.stream()
.filter(key -> key instanceof ConfiguredTargetKey)
- .map(key -> ((ConfiguredTargetKey) key).getLabel())
+ .map(ArtifactOwner::getLabel)
.collect(toImmutableMultiset());
ImmutableMap<Label, Integer> expected =
targetsWithCounts
@@ -201,7 +202,8 @@
"//test:starlark_dep",
"//test:native_shared_dep",
"//test:starlark_shared_dep");
- LinkedHashSet<SkyKey> visitedTargets = new LinkedHashSet<>(getSkyframeEvaluatedTargetKeys());
+ LinkedHashSet<ActionLookupKey> visitedTargets =
+ new LinkedHashSet<>(getSkyframeEvaluatedTargetKeys());
// asserting that the top-level targets are the same as the ones in the diamond starting at
// //test:suite
assertNumberOfConfigurationsOfTargets(
@@ -335,7 +337,8 @@
"//test:starlark_dep",
"//test:native_shared_dep",
"//test:starlark_shared_dep");
- LinkedHashSet<SkyKey> visitedTargets = new LinkedHashSet<>(getSkyframeEvaluatedTargetKeys());
+ LinkedHashSet<ActionLookupKey> visitedTargets =
+ new LinkedHashSet<>(getSkyframeEvaluatedTargetKeys());
// asserting that the top-level targets are the same as the ones in the diamond starting at
// //test:suite
assertNumberOfConfigurationsOfTargets(
@@ -640,7 +643,8 @@
"//test:starlark_dep",
"//test:native_shared_dep",
"//test:starlark_shared_dep");
- LinkedHashSet<SkyKey> visitedTargets = new LinkedHashSet<>(getSkyframeEvaluatedTargetKeys());
+ LinkedHashSet<ActionLookupKey> visitedTargets =
+ new LinkedHashSet<>(getSkyframeEvaluatedTargetKeys());
assertNumberOfConfigurationsOfTargets(
visitedTargets,
new ImmutableMap.Builder<String, Integer>()
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
index ae61bb3..c0d04a8 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
@@ -32,6 +32,7 @@
import com.google.devtools.build.lib.actions.ActionLookupKey;
import com.google.devtools.build.lib.actions.ActionLookupValue;
import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.ArtifactOwner;
import com.google.devtools.build.lib.analysis.AnalysisOptions;
import com.google.devtools.build.lib.analysis.AnalysisResult;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
@@ -80,7 +81,6 @@
import com.google.devtools.build.lib.vfs.ModifiedFileSet;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
-import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.common.options.Options;
import com.google.devtools.common.options.OptionsParser;
import java.util.Arrays;
@@ -568,8 +568,8 @@
.build());
}
- protected Set<SkyKey> getSkyframeEvaluatedTargetKeys() {
- return buildView.getSkyframeEvaluatedTargetKeysForTesting();
+ protected Set<ActionLookupKey> getSkyframeEvaluatedTargetKeys() {
+ return buildView.getSkyframeEvaluatedActionLookupKeyCountForTesting();
}
protected void assertNumberOfAnalyzedConfigurationsOfTargets(
@@ -577,7 +577,7 @@
ImmutableMultiset<Label> actualSet =
getSkyframeEvaluatedTargetKeys().stream()
.filter(key -> key instanceof ConfiguredTargetKey)
- .map(key -> ((ConfiguredTargetKey) key).getLabel())
+ .map(ArtifactOwner::getLabel)
.collect(toImmutableMultiset());
ImmutableMap<Label, Integer> expected =
targetsWithCounts.entrySet().stream()
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
index 8d10729..4f51619 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
@@ -14,6 +14,9 @@
package com.google.devtools.build.lib.analysis.util;
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
+import static com.google.common.collect.Streams.stream;
+
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.collect.Collections2;
@@ -21,7 +24,11 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.MapDifference;
+import com.google.common.collect.Maps;
+import com.google.common.collect.Sets;
import com.google.common.eventbus.EventBus;
+import com.google.devtools.build.lib.actions.ActionLookupKey;
import com.google.devtools.build.lib.actions.ArtifactFactory;
import com.google.devtools.build.lib.actions.PackageRoots;
import com.google.devtools.build.lib.analysis.AnalysisEnvironment;
@@ -95,6 +102,7 @@
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.ValueOrException;
+import com.google.devtools.build.skyframe.Version;
import java.util.Collection;
import java.util.HashMap;
import java.util.LinkedHashMap;
@@ -119,6 +127,8 @@
private final ConfiguredRuleClassProvider ruleClassProvider;
+ private ImmutableMap<ActionLookupKey, Version> currentActionLookupKeys = ImmutableMap.of();
+
public BuildViewForTesting(
BlazeDirectories directories,
ConfiguredRuleClassProvider ruleClassProvider,
@@ -136,8 +146,28 @@
}
@VisibleForTesting
- public Set<SkyKey> getSkyframeEvaluatedTargetKeysForTesting() {
- return skyframeBuildView.getEvaluatedTargetKeys();
+ public Set<ActionLookupKey> getSkyframeEvaluatedActionLookupKeyCountForTesting() {
+ Set<ActionLookupKey> actionLookupKeys = populateActionLookupKeyMapAndGetDiff();
+ Preconditions.checkState(
+ actionLookupKeys.size() == skyframeBuildView.getEvaluatedCounts().total(),
+ "Number of newly evaluated action lookup values %s does not agree with number that changed"
+ + " in graph: %s",
+ actionLookupKeys);
+ return actionLookupKeys;
+ }
+
+ private Set<ActionLookupKey> populateActionLookupKeyMapAndGetDiff() {
+ ImmutableMap<ActionLookupKey, Version> newMap =
+ stream(skyframeExecutor.getEvaluatorForTesting().getGraphEntries())
+ .filter(e -> e.getKey() instanceof ActionLookupKey)
+ .collect(
+ toImmutableMap(
+ e -> ((ActionLookupKey) e.getKey()), e -> e.getValue().getVersion()));
+ MapDifference<ActionLookupKey, Version> difference =
+ Maps.difference(newMap, currentActionLookupKeys);
+ currentActionLookupKeys = newMap;
+ return Sets.union(
+ difference.entriesDiffering().keySet(), difference.entriesOnlyOnLeft().keySet());
}
/**
@@ -162,6 +192,7 @@
ExtendedEventHandler eventHandler,
EventBus eventBus)
throws ViewCreationFailedException, InterruptedException, InvalidConfigurationException {
+ populateActionLookupKeyMapAndGetDiff();
return buildView.update(
loadingResult,
targetOptions,
