Skip to content

Commit

Permalink
Fix a bug that outputs from aspects are not downloaded.
Browse files Browse the repository at this point in the history 
Previously, we changed how to handle toplevel outputs with 3ef2e53. 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
  • Loading branch information
@coeuvre @fweikert
coeuvre authored and fweikert committed May 19, 2023
1 parent b384a98 commit c7792e8
Show file tree
Hide file tree
Showing 3 changed files with 147 additions and 9 deletions.
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/remote/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,10 +191,12 @@ java_library(
"//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",
],
)

Expand Down
28 changes: 19 additions & 9 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteOutputChecker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,15 @@
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;
import com.google.devtools.build.lib.clock.Clock;
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 {
Expand Down Expand Up @@ -79,36 +81,40 @@ public RemoteOutputChecker(
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;
Expand All @@ -125,7 +131,11 @@ private void addRunfiles(ConfiguredTarget buildTarget) {
}
}

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
Expand Down
126 changes: 126 additions & 0 deletions ...st/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -516,6 +517,93 @@ public void dynamicExecution_stderrIsReported() throws Exception {
assertOutputContains(outErr.errAsLatin1(), "my-error-message");
}

@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();
Expand Down Expand Up @@ -1333,6 +1421,44 @@ protected void writeOutputDirRule() throws IOException {
")");
}

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<>();
Expand Down

0 comments on commit c7792e8

Please sign in to comment.