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'");
+ }
}
