Fix a bug that outputs from aspects are not downloaded.
Previously, we changed how to handle toplevel outputs with https://github.com/bazelbuild/bazel/commit/3ef2e53ab65eab006b0d01c41ebc2265754b2007. It introduced a bug that outputs from output groups registered by toplevel aspects (combination of `--aspects` and `--output_groups`) were not downloaded. This CL fixes that.
PiperOrigin-RevId: 532460057
Change-Id: I8bd2ad872c0038c3006779547994ef060ea113d7
diff --git a/src/main/java/com/google/devtools/build/lib/remote/BUILD b/src/main/java/com/google/devtools/build/lib/remote/BUILD
index cd77fd8..c3136da 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/remote/BUILD
@@ -191,10 +191,12 @@
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
+ "//src/main/java/com/google/devtools/build/lib/analysis:provider_collection",
"//src/main/java/com/google/devtools/build/lib/analysis:top_level_artifact_context",
"//src/main/java/com/google/devtools/build/lib/clock",
"//src/main/java/com/google/devtools/build/lib/packages",
"//third_party:guava",
+ "//third_party:jsr305",
],
)
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java
index 8418237..8a8e324 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java
@@ -25,6 +25,7 @@
import com.google.devtools.build.lib.analysis.AnalysisResult;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.FilesToRunProvider;
+import com.google.devtools.build.lib.analysis.ProviderCollection;
import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
@@ -32,6 +33,7 @@
import java.util.Set;
import java.util.function.Supplier;
import java.util.regex.Pattern;
+import javax.annotation.Nullable;
/** A {@link RemoteArtifactChecker} that checks the TTL of remote metadata. */
public class RemoteOutputChecker implements RemoteArtifactChecker {
@@ -79,36 +81,40 @@
public void afterTopLevelTargetAnalysis(
ConfiguredTarget configuredTarget,
Supplier<TopLevelArtifactContext> topLevelArtifactContextSupplier) {
- addTopLevelTarget(configuredTarget, topLevelArtifactContextSupplier);
+ addTopLevelTarget(configuredTarget, configuredTarget, topLevelArtifactContextSupplier);
}
public void afterAnalysis(AnalysisResult analysisResult) {
for (var target : analysisResult.getTargetsToBuild()) {
- addTopLevelTarget(target, analysisResult::getTopLevelContext);
+ addTopLevelTarget(target, target, analysisResult::getTopLevelContext);
+ }
+ for (var aspect : analysisResult.getAspectsMap().values()) {
+ addTopLevelTarget(aspect, /* configuredTarget= */ null, analysisResult::getTopLevelContext);
}
var targetsToTest = analysisResult.getTargetsToTest();
if (targetsToTest != null) {
for (var target : targetsToTest) {
- addTopLevelTarget(target, analysisResult::getTopLevelContext);
+ addTopLevelTarget(target, target, analysisResult::getTopLevelContext);
}
}
}
private void addTopLevelTarget(
- ConfiguredTarget toplevelTarget,
+ ProviderCollection target,
+ @Nullable ConfiguredTarget configuredTarget,
Supplier<TopLevelArtifactContext> topLevelArtifactContextSupplier) {
- if (shouldDownloadToplevelOutputs(toplevelTarget)) {
+ if (shouldDownloadToplevelOutputs(configuredTarget)) {
var topLevelArtifactContext = topLevelArtifactContextSupplier.get();
var artifactsToBuild =
- TopLevelArtifactHelper.getAllArtifactsToBuild(toplevelTarget, topLevelArtifactContext)
+ TopLevelArtifactHelper.getAllArtifactsToBuild(target, topLevelArtifactContext)
.getImportantArtifacts();
toplevelArtifactsToDownload.addAll(artifactsToBuild.toList());
- addRunfiles(toplevelTarget);
+ addRunfiles(target);
}
}
- private void addRunfiles(ConfiguredTarget buildTarget) {
+ private void addRunfiles(ProviderCollection buildTarget) {
var runfilesProvider = buildTarget.getProvider(FilesToRunProvider.class);
if (runfilesProvider == null) {
return;
@@ -125,7 +131,11 @@
}
}
- private boolean shouldDownloadToplevelOutputs(ConfiguredTarget configuredTarget) {
+ public void addInputToDownload(ActionInput file) {
+ inputsToDownload.add(file);
+ }
+
+ private boolean shouldDownloadToplevelOutputs(@Nullable ConfiguredTarget configuredTarget) {
switch (commandMode) {
case RUN:
// Always download outputs of toplevel targets in RUN mode
diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java
index 83bb5b4..ae63822 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java
@@ -21,6 +21,7 @@
import static org.junit.Assert.assertThrows;
import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.eventbus.Subscribe;
import com.google.devtools.build.lib.actions.ActionExecutedEvent;
@@ -517,6 +518,93 @@
}
@Test
+ public void downloadToplevel_outputsFromAspect_notAggregated() throws Exception {
+ setDownloadToplevel();
+ writeCopyAspectRule(/* aggregate= */ false);
+ write(
+ "BUILD",
+ "genrule(",
+ " name = 'foo',",
+ " srcs = ['foo.in'],",
+ " outs = ['foo.out'],",
+ " cmd = 'cat $(SRCS) > $@',",
+ ")",
+ "genrule(",
+ " name = 'foobar',",
+ " srcs = [':foo'],",
+ " outs = ['foobar.out'],",
+ " cmd = 'cat $(location :foo) > $@ && echo bar >> $@',",
+ ")");
+ write("foo.in", "foo");
+
+ addOptions("--aspects=rules.bzl%copy_aspect", "--output_groups=+copy");
+ buildTarget("//:foobar");
+ waitDownloads();
+
+ assertValidOutputFile("foobar.out", "foo" + lineSeparator() + "bar\n");
+ assertOutputDoesNotExist("foo.in.copy");
+ assertValidOutputFile("foo.out.copy", "foo" + lineSeparator());
+ }
+
+ @Test
+ public void downloadToplevel_outputsFromAspect_aggregated() throws Exception {
+ setDownloadToplevel();
+ writeCopyAspectRule(/* aggregate= */ true);
+ write(
+ "BUILD",
+ "genrule(",
+ " name = 'foo',",
+ " srcs = ['foo.in'],",
+ " outs = ['foo.out'],",
+ " cmd = 'cat $(SRCS) > $@',",
+ ")",
+ "genrule(",
+ " name = 'foobar',",
+ " srcs = [':foo'],",
+ " outs = ['foobar.out'],",
+ " cmd = 'cat $(location :foo) > $@ && echo bar >> $@',",
+ ")");
+ write("foo.in", "foo");
+
+ addOptions("--aspects=rules.bzl%copy_aspect", "--output_groups=+copy");
+ buildTarget("//:foobar");
+ waitDownloads();
+
+ assertValidOutputFile("foobar.out", "foo" + lineSeparator() + "bar\n");
+ assertValidOutputFile("foo.in.copy", "foo" + lineSeparator());
+ assertValidOutputFile("foo.out.copy", "foo" + lineSeparator());
+ }
+
+ @Test
+ public void downloadToplevel_outputsFromAspect_notDownloadedIfNoOutputGroups() throws Exception {
+ setDownloadToplevel();
+ writeCopyAspectRule(/* aggregate= */ true);
+ write(
+ "BUILD",
+ "genrule(",
+ " name = 'foo',",
+ " srcs = ['foo.in'],",
+ " outs = ['foo.out'],",
+ " cmd = 'cat $(SRCS) > $@',",
+ ")",
+ "genrule(",
+ " name = 'foobar',",
+ " srcs = [':foo'],",
+ " outs = ['foobar.out'],",
+ " cmd = 'cat $(location :foo) > $@ && echo bar >> $@',",
+ ")");
+ write("foo.in", "foo");
+
+ addOptions("--aspects=rules.bzl%copy_aspect");
+ buildTarget("//:foobar");
+ waitDownloads();
+
+ assertValidOutputFile("foobar.out", "foo" + lineSeparator() + "bar\n");
+ assertOutputDoesNotExist("foo.in.copy");
+ assertOutputDoesNotExist("foo.out.copy");
+ }
+
+ @Test
public void downloadToplevel_outputsFromImportantOutputGroupAreDownloaded() throws Exception {
setDownloadToplevel();
write(
@@ -1333,6 +1421,44 @@
")");
}
+ protected void writeCopyAspectRule(boolean aggregate) throws IOException {
+ var lines = ImmutableList.<String>builder();
+ lines.add(
+ "def _copy_aspect_impl(target, ctx):",
+ " files = []",
+ " for src in ctx.rule.files.srcs:",
+ " dst = ctx.actions.declare_file(src.basename + '.copy')",
+ " ctx.actions.run_shell(",
+ " inputs = [src],",
+ " outputs = [dst],",
+ " command = '''",
+ "cp $1 $2",
+ "''',",
+ " arguments = [src.path, dst.path],",
+ " )",
+ " files.append(dst)",
+ "");
+ if (aggregate) {
+ lines.add(
+ " files = depset(",
+ " direct = files,",
+ " transitive = [src[OutputGroupInfo].copy for src in ctx.rule.attr.srcs if"
+ + " OutputGroupInfo in src],",
+ " )");
+ } else {
+ lines.add(" files = depset(files)");
+ }
+ lines.add(
+ "",
+ " return [OutputGroupInfo(copy = files)]",
+ "",
+ "copy_aspect = aspect(",
+ " implementation = _copy_aspect_impl,",
+ " attr_aspects = ['srcs'],",
+ ")");
+ write("rules.bzl", lines.build().toArray(new String[0]));
+ }
+
protected static class ActionEventCollector {
private final List<ActionExecutedEvent> actionExecutedEvents = new ArrayList<>();
private final List<CachedActionEvent> cachedActionEvents = new ArrayList<>();
