Skip to content

Commit

Permalink
Fix BEP inlined outputs for aspect-complete events when outputs must …
Browse files Browse the repository at this point in the history 
…be uploaded.

The recently introduced option `--experimental_build_event_output_group_mode` allows users to request certain output groups to have their `File`s reported inline in the `TargetComplete` event. This allows users to more easily obtain URIs for files without walking the `NamedSetOfFiles` events, while accepting the responsibility to ensure that the requested output groups are small enough that the `TargetComplete` event does not grow enormous. (Unlike `NamedSetOfFiles`, a `TargetComplete` event cannot be split up recursively.)

The default BEP behavior is to only report a `File` with a remote URI. In some build configurations, outputs might already have a remote URI determined during execution, but this is not true in general. When an output file that appears in BEP does not have a known remote URI, the BEP subsystem is responsible for uploading the file to obtain a remote URI.

The `NamedSetOfFiles` event has working uploading logic, so files were correctly reported by aspects using the default value of `--experimental_build_event_output_group_mode`. Previously, the uploading logic was missing for `AspectCompleteEvent`, so we did not have a remote URI when inlining was requested by `--experimental_build_event_output_group_mode`. This change rectifies this oversight, ensuring a remote URI is available no matter how an aspect-produced file is reported.

PiperOrigin-RevId: 722658204
Change-Id: Ie6fc70192f687b8e5582d76ab048dfa6bd25186e
  • Loading branch information
@michaeledgar @copybara-github
michaeledgar authored and copybara-github committed Feb 3, 2025
1 parent e82df5d commit 2d1f69d
Show file tree
Hide file tree
Showing 3 changed files with 139 additions and 25 deletions.
35 changes: 35 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/AspectCompleteEvent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// limitations under the License.
package com.google.devtools.build.lib.analysis;

import static com.google.devtools.build.lib.analysis.TargetCompleteEvent.filterFilesets;
import static com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil.configurationId;

import com.google.common.base.Preconditions;
Expand All @@ -21,8 +22,11 @@
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.CompletionContext;
import com.google.devtools.build.lib.actions.CompletionContext.ArtifactReceiver;
import com.google.devtools.build.lib.actions.EventReportingArtifacts;
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.analysis.TopLevelArtifactHelper.ArtifactsInOutputGroup;
import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile.LocalFileType;
import com.google.devtools.build.lib.buildeventstream.BuildEventContext;
import com.google.devtools.build.lib.buildeventstream.BuildEventIdUtil;
import com.google.devtools.build.lib.buildeventstream.BuildEventProtocolOptions.OutputGroupFileModes;
Expand All @@ -36,6 +40,8 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.skyframe.AspectKeyCreator.AspectKey;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.SkyValue;
import java.util.Collection;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -180,6 +186,35 @@ public BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventContext convert
return GenericBuildEvent.protoChaining(this).setCompleted(builder.build()).build();
}

@Override
public ImmutableList<LocalFile> referencedLocalFiles() {
ImmutableList.Builder<LocalFile> builder = ImmutableList.builder();
for (ArtifactsInOutputGroup group : artifactOutputGroups.values()) {
if (group.areImportant()) {
completionContext.visitArtifacts(
filterFilesets(group.getArtifacts().toList()),
new ArtifactReceiver() {
@Override
public void accept(Artifact artifact) {
FileArtifactValue metadata = completionContext.getFileArtifactValue(artifact);
builder.add(
new LocalFile(
completionContext.pathResolver().toPath(artifact),
LocalFileType.forArtifact(artifact, metadata),
metadata));
}

@Override
public void acceptFilesetMapping(
Artifact fileset, PathFragment name, Path targetFile) {
throw new IllegalStateException(fileset + " should have been filtered out");
}
});
}
}
return builder.build();
}

@Override
public boolean storeForReplay() {
return true;
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ public void acceptFilesetMapping(
});
}

private static Iterable<Artifact> filterFilesets(Iterable<Artifact> artifacts) {
static Iterable<Artifact> filterFilesets(Iterable<Artifact> artifacts) {
return Iterables.filter(artifacts, artifact -> !artifact.isFileset());
}

Expand Down
127 changes: 103 additions & 24 deletions src/test/java/com/google/devtools/build/lib/buildtool/TargetCompleteEventTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,9 +185,9 @@ public void outputFile() throws Exception {
write("foo/BUILD", "genrule(name = 'foobin', outs = ['out.txt'], cmd = 'echo -n Hello > $@')");

addOptions("--experimental_build_event_output_group_mode=default=named_set_of_files_only");
File bep = buildTargetAndCaptureBEP("//foo:foobin");
File bep = buildTargetAndCaptureBep("//foo:foobin");

BuildEventStreamProtos.File outFile = findOutputFileInBEPStream(bep, "out.txt");
BuildEventStreamProtos.File outFile = findOutputFileInBepStream(bep, "out.txt");
assertThat(outFile).isNotNull();
assertThat(outFile.getUri()).startsWith("file://");
assertThat(outFile.getUri()).endsWith("/bin/foo/out.txt");
Expand Down Expand Up @@ -219,17 +219,17 @@ def _impl(ctx):
""");

addOptions("--experimental_build_event_output_group_mode=default=named_set_of_files_only");
File bep = buildTargetAndCaptureBEP("//foo:dir");
File bep = buildTargetAndCaptureBep("//foo:dir");

BuildEventStreamProtos.TargetComplete targetComplete = findTargetCompleteEventInBEPStream(bep);
BuildEventStreamProtos.TargetComplete targetComplete = findTargetCompleteEventInBepStream(bep);
assertThat(targetComplete.getDirectoryOutputList()).hasSize(1);
BuildEventStreamProtos.File dir = targetComplete.getDirectoryOutputList().get(0);
assertThat(dir.getName()).endsWith("/dir");
assertThat(dir.getUri()).isEmpty();
assertThat(dir.getContents()).isEmpty();
assertThat(dir.getSymlinkTargetPath()).isEmpty();

BuildEventStreamProtos.File outFile = findOutputFileInBEPStream(bep, "file.txt");
BuildEventStreamProtos.File outFile = findOutputFileInBepStream(bep, "file.txt");
assertThat(outFile).isNotNull();
assertThat(outFile.getUri()).startsWith("file://");
assertThat(outFile.getUri()).endsWith("/bin/foo/dir/file.txt");
Expand Down Expand Up @@ -258,9 +258,9 @@ def _impl(ctx):
""");

addOptions("--experimental_build_event_output_group_mode=default=named_set_of_files_only");
File bep = buildTargetAndCaptureBEP("//foo:sym");
File bep = buildTargetAndCaptureBep("//foo:sym");

BuildEventStreamProtos.File outFile = findOutputFileInBEPStream(bep, "sym");
BuildEventStreamProtos.File outFile = findOutputFileInBepStream(bep, "sym");
assertThat(outFile).isNotNull();
assertThat(outFile.getSymlinkTargetPath()).isEqualTo("/some/path");
assertThat(outFile.getLength()).isEqualTo(0);
Expand All @@ -272,12 +272,12 @@ public void outputFile_inlineOutputGroup() throws Exception {
write("foo/BUILD", "genrule(name = 'foobin', outs = ['out.txt'], cmd = 'echo -n Hello > $@')");

addOptions("--experimental_build_event_output_group_mode=default=inline_only");
File bep = buildTargetAndCaptureBEP("//foo:foobin");
File bep = buildTargetAndCaptureBep("//foo:foobin");

BuildEventStreamProtos.File outFileFromNestedSet = findOutputFileInBEPStream(bep, "out.txt");
BuildEventStreamProtos.File outFileFromNestedSet = findOutputFileInBepStream(bep, "out.txt");
assertThat(outFileFromNestedSet).isNull();

TargetComplete completeEvent = findTargetCompleteEventInBEPStream(bep);
TargetComplete completeEvent = findTargetCompleteEventInBepStream(bep);
assertThat(completeEvent).isNotNull();
assertThat(completeEvent.getOutputGroupCount()).isEqualTo(1);
assertThat(completeEvent.getOutputGroup(0).getInlineFilesCount()).isEqualTo(1);
Expand All @@ -295,12 +295,12 @@ public void outputFile_outputGroupFileModeOptionRepeated_lastValueTaken() throws

addOptions("--experimental_build_event_output_group_mode=default=named_set_of_files_only");
addOptions("--experimental_build_event_output_group_mode=default=inline_only");
File bep = buildTargetAndCaptureBEP("//foo:foobin");
File bep = buildTargetAndCaptureBep("//foo:foobin");

BuildEventStreamProtos.File outFileFromNestedSet = findOutputFileInBEPStream(bep, "out.txt");
BuildEventStreamProtos.File outFileFromNestedSet = findOutputFileInBepStream(bep, "out.txt");
assertThat(outFileFromNestedSet).isNull();

TargetComplete completeEvent = findTargetCompleteEventInBEPStream(bep);
TargetComplete completeEvent = findTargetCompleteEventInBepStream(bep);
assertThat(completeEvent).isNotNull();
assertThat(completeEvent.getOutputGroupCount()).isEqualTo(1);
assertThat(completeEvent.getOutputGroup(0).getInlineFilesCount()).isEqualTo(1);
Expand Down Expand Up @@ -343,17 +343,17 @@ def _impl(ctx):
addOptions("--experimental_build_event_output_group_mode=filesetgroup=named_set_of_files_only");
addOptions("--experimental_build_event_output_group_mode=bothgroup=both");
addOptions("--output_groups=+inlinegroup,+filesetgroup,+bothgroup");
File bep = buildTargetAndCaptureBEP("//foo:myrule");
File bep = buildTargetAndCaptureBep("//foo:myrule");

TargetComplete completeEvent = findTargetCompleteEventInBEPStream(bep);
TargetComplete completeEvent = findTargetCompleteEventInBepStream(bep);
assertThat(completeEvent).isNotNull();
assertThat(completeEvent.getOutputGroupCount()).isEqualTo(3);
OutputGroup inlineOutputGroup = findOutputGroupWithName(completeEvent, "inlinegroup");
OutputGroup filesetOutputGroup = findOutputGroupWithName(completeEvent, "filesetgroup");
OutputGroup bothOutputGroup = findOutputGroupWithName(completeEvent, "bothgroup");

assertThat(inlineOutputGroup.getInlineFilesCount()).isEqualTo(1);
assertThat(findOutputFileInBEPStream(bep, "myrule.inline.txt")).isNull();
assertThat(findOutputFileInBepStream(bep, "myrule.inline.txt")).isNull();
BuildEventStreamProtos.File inlineOutFile = inlineOutputGroup.getInlineFiles(0);
assertThat(inlineOutFile.getUri()).startsWith("file://");
assertThat(inlineOutFile.getUri()).endsWith("/bin/foo/myrule.inline.txt");
Expand All @@ -362,7 +362,7 @@ def _impl(ctx):

assertThat(filesetOutputGroup.getInlineFilesCount()).isEqualTo(0);
BuildEventStreamProtos.File filesetOutFile =
findOutputFileInBEPStream(bep, "myrule.fileset.txt");
findOutputFileInBepStream(bep, "myrule.fileset.txt");
assertThat(filesetOutFile.getUri()).startsWith("file://");
assertThat(filesetOutFile.getUri()).endsWith("/bin/foo/myrule.fileset.txt");
assertThat(filesetOutFile.getLength()).isEqualTo("Hola".length());
Expand All @@ -371,7 +371,7 @@ def _impl(ctx):
assertThat(bothOutputGroup.getInlineFilesCount()).isEqualTo(1);
BuildEventStreamProtos.File bothOutFileInline = bothOutputGroup.getInlineFiles(0);
BuildEventStreamProtos.File bothOutFileInFileset =
findOutputFileInBEPStream(bep, "myrule.both.txt");
findOutputFileInBepStream(bep, "myrule.both.txt");
for (var outfile : ImmutableList.of(bothOutFileInline, bothOutFileInFileset)) {
assertThat(outfile.getUri()).startsWith("file://");
assertThat(outfile.getUri()).endsWith("/bin/foo/myrule.both.txt");
Expand All @@ -380,7 +380,74 @@ def _impl(ctx):
}
}

private File buildTargetAndCaptureBEP(String target) throws Exception {
@Test
public void outputFile_multipleOutputGroups_aspect() throws Exception {
write("foo/BUILD", "genrule(name = 'myrule', outs = ['out.txt'], cmd = 'echo -n Hello > $@')");
write(
"foo/aspect.bzl",
"""
def _impl(target, ctx):
inline_out = ctx.actions.declare_file(ctx.label.name + '.inline.txt')
ctx.actions.write(output = inline_out, content = 'Hello')
fileset_out = ctx.actions.declare_file(ctx.label.name + '.fileset.txt')
ctx.actions.write(output = fileset_out, content = 'Hola')
both_out = ctx.actions.declare_file(ctx.label.name + '.both.txt')
ctx.actions.write(output = both_out, content = 'Bonjour')
output_groups = {
"inlinegroup": depset([inline_out]),
"filesetgroup": depset([fileset_out]),
"bothgroup": depset([both_out]),
}
return [
OutputGroupInfo(**output_groups),
]
multiple_groups = aspect(implementation = _impl)
""");

addOptions("--experimental_build_event_output_group_mode=inlinegroup=inline_only");
addOptions("--experimental_build_event_output_group_mode=filesetgroup=named_set_of_files_only");
addOptions("--experimental_build_event_output_group_mode=bothgroup=both");
addOptions("--output_groups=+inlinegroup,+filesetgroup,+bothgroup");
addOptions("--aspects=//foo:aspect.bzl%multiple_groups");
File bep = buildTargetAndCaptureBep("//foo:myrule");

TargetComplete completeEvent = findAspectCompleteEventInBepStream(bep);
assertThat(completeEvent).isNotNull();
assertThat(completeEvent.getOutputGroupCount()).isEqualTo(3);
OutputGroup inlineOutputGroup = findOutputGroupWithName(completeEvent, "inlinegroup");
OutputGroup filesetOutputGroup = findOutputGroupWithName(completeEvent, "filesetgroup");
OutputGroup bothOutputGroup = findOutputGroupWithName(completeEvent, "bothgroup");

assertThat(inlineOutputGroup.getInlineFilesCount()).isEqualTo(1);
assertThat(findOutputFileInBepStream(bep, "myrule.inline.txt")).isNull();
BuildEventStreamProtos.File inlineOutFile = inlineOutputGroup.getInlineFiles(0);
assertThat(inlineOutFile.getUri()).startsWith("file://");
assertThat(inlineOutFile.getUri()).endsWith("/bin/foo/myrule.inline.txt");
assertThat(inlineOutFile.getLength()).isEqualTo("Hello".length());
assertDigest("Hello", BaseEncoding.base16().lowerCase().decode(inlineOutFile.getDigest()));

assertThat(filesetOutputGroup.getInlineFilesCount()).isEqualTo(0);
BuildEventStreamProtos.File filesetOutFile =
findOutputFileInBepStream(bep, "myrule.fileset.txt");
assertThat(filesetOutFile.getUri()).startsWith("file://");
assertThat(filesetOutFile.getUri()).endsWith("/bin/foo/myrule.fileset.txt");
assertThat(filesetOutFile.getLength()).isEqualTo("Hola".length());
assertDigest("Hola", BaseEncoding.base16().lowerCase().decode(filesetOutFile.getDigest()));

assertThat(bothOutputGroup.getInlineFilesCount()).isEqualTo(1);
BuildEventStreamProtos.File bothOutFileInline = bothOutputGroup.getInlineFiles(0);
BuildEventStreamProtos.File bothOutFileInFileset =
findOutputFileInBepStream(bep, "myrule.both.txt");
for (var outfile : ImmutableList.of(bothOutFileInline, bothOutFileInFileset)) {
assertThat(outfile.getUri()).startsWith("file://");
assertThat(outfile.getUri()).endsWith("/bin/foo/myrule.both.txt");
assertThat(outfile.getLength()).isEqualTo("Bonjour".length());
assertDigest("Bonjour", BaseEncoding.base16().lowerCase().decode(outfile.getDigest()));
}
}

private File buildTargetAndCaptureBep(String target) throws Exception {
File bep = tmpFolder.newFile();
// We use WAIT_FOR_UPLOAD_COMPLETE because it's the easiest way to force the BES module to
// wait until the BEP binary file has been written.
Expand Down Expand Up @@ -413,7 +480,7 @@ private static void assertDigest(String contents, byte[] bepDigest) {
assertThat(foundHashFunction).isTrue();
}

private static ImmutableList<BuildEvent> parseBuildEventsFromBEPStream(File bep)
private static ImmutableList<BuildEvent> parseBuildEventsFromBepStream(File bep)
throws IOException {
ImmutableList.Builder<BuildEvent> buildEvents = ImmutableList.builder();
try (InputStream in = new FileInputStream(bep)) {
Expand All @@ -426,9 +493,9 @@ private static ImmutableList<BuildEvent> parseBuildEventsFromBEPStream(File bep)
}

@Nullable
private static BuildEventStreamProtos.TargetComplete findTargetCompleteEventInBEPStream(File bep)
private static BuildEventStreamProtos.TargetComplete findTargetCompleteEventInBepStream(File bep)
throws IOException {
for (BuildEvent buildEvent : parseBuildEventsFromBEPStream(bep)) {
for (BuildEvent buildEvent : parseBuildEventsFromBepStream(bep)) {
if (buildEvent.getId().getIdCase() == IdCase.TARGET_COMPLETED) {
return buildEvent.getCompleted();
}
Expand All @@ -437,9 +504,21 @@ private static BuildEventStreamProtos.TargetComplete findTargetCompleteEventInBE
}

@Nullable
private static BuildEventStreamProtos.File findOutputFileInBEPStream(File bep, String name)
private static BuildEventStreamProtos.TargetComplete findAspectCompleteEventInBepStream(File bep)
throws IOException {
for (BuildEvent buildEvent : parseBuildEventsFromBepStream(bep)) {
if (buildEvent.getId().getIdCase() == IdCase.TARGET_COMPLETED
&& !buildEvent.getId().getTargetCompleted().getAspect().isEmpty()) {
return buildEvent.getCompleted();
}
}
return null;
}

@Nullable
private static BuildEventStreamProtos.File findOutputFileInBepStream(File bep, String name)
throws IOException {
for (BuildEvent buildEvent : parseBuildEventsFromBEPStream(bep)) {
for (BuildEvent buildEvent : parseBuildEventsFromBepStream(bep)) {
if (buildEvent.getId().getIdCase() == IdCase.NAMED_SET) {
NamedSetOfFiles namedSetOfFiles = buildEvent.getNamedSetOfFiles();
for (BuildEventStreamProtos.File file : namedSetOfFiles.getFilesList()) {
Expand Down

0 comments on commit 2d1f69d

Please sign in to comment.