Skip to content

Commit

Permalink
Fix LocalFileType computation when the artifact and its metadata disa…
Browse files Browse the repository at this point in the history 
…gree.

When a tree artifact contains a symlink to a directory, the TreeArtifactValue
contains a TreeFileArtifact mapping to a DirectoryArtifactValue (see bazelbuild#20418).

Because the file type to be uploaded to the BES is determined solely by the
Artifact type, this causes the uploader to attempt to upload a directory as if
it were a file. This fails silently when building with the bytes; when building
without the bytes, it crashes when attempting to digest the (in-memory)
directory, which has no associated inode (see bazelbuild#20415).

This PR fixes the file type computation to take into account both the artifact
and its metadata, preferring the latter when available.

Fixes bazelbuild#20415.
  • Loading branch information
@tjgq
tjgq committed Dec 3, 2023
1 parent 9aca477 commit 370768c
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 55 deletions.
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/actions/ActionExecutedEvent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ public Collection<LocalFile> referencedLocalFiles() {
localFiles.add(
new LocalFile(
primaryOutput,
LocalFileType.forArtifact(outputArtifact),
LocalFileType.forArtifact(outputArtifact, primaryOutputMetadata),
outputArtifact,
primaryOutputMetadata));
}
Expand Down
5 changes: 3 additions & 2 deletions 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 @@ -406,12 +406,13 @@ public ImmutableList<LocalFile> referencedLocalFiles() {
new ArtifactReceiver() {
@Override
public void accept(Artifact artifact) {
FileArtifactValue metadata = completionContext.getFileArtifactValue(artifact);
builder.add(
new LocalFile(
completionContext.pathResolver().toPath(artifact),
LocalFileType.forArtifact(artifact),
LocalFileType.forArtifact(artifact, metadata),
artifact,
completionContext.getFileArtifactValue(artifact)));
metadata));
}

@Override
Expand Down
23 changes: 21 additions & 2 deletions src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEvent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.google.common.util.concurrent.ListenableFuture;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile.LocalFileType;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.vfs.Path;
import java.util.Collection;
Expand Down Expand Up @@ -58,15 +59,33 @@ public enum LocalFileType {
LOG,
PERFORMANCE_LOG;

/** Returns whether the LocalFile is a declared action output. */
/**
* Returns whether the LocalFile is a declared action output.
*/
public boolean isOutput() {
return this == OUTPUT
|| this == OUTPUT_FILE
|| this == OUTPUT_DIRECTORY
|| this == OUTPUT_SYMLINK;
}

public static LocalFileType forArtifact(Artifact artifact) {
/**
* Returns the {@link LocalFileType} inferred from a
* {@link FileArtifactValue, or from the associated {@link Artifact} if the former is not
* available.
*/
public static LocalFileType forArtifact(Artifact artifact,
@Nullable FileArtifactValue metadata) {
if (metadata != null) {
switch (metadata.getType()) {
case DIRECTORY:
return LocalFileType.OUTPUT_DIRECTORY;
case SYMLINK:
return LocalFileType.OUTPUT_SYMLINK;
default:
return LocalFileType.OUTPUT_FILE;
}
}
if (artifact.isDirectory()) {
return LocalFileType.OUTPUT_DIRECTORY;
} else if (artifact.isSymlink()) {
Expand Down
22 changes: 4 additions & 18 deletions src/main/java/com/google/devtools/build/lib/runtime/NamedArtifactGroup.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,14 @@ public Collection<LocalFile> referencedLocalFiles() {
for (Object elem : set.getLeaves()) {
ExpandedArtifact expandedArtifact = (ExpandedArtifact) elem;
if (expandedArtifact.relPath == null) {
FileArtifactValue fileMetadata =
FileArtifactValue metadata =
completionContext.getFileArtifactValue(expandedArtifact.artifact);
artifacts.add(
new LocalFile(
completionContext.pathResolver().toPath(expandedArtifact.artifact),
getOutputType(fileMetadata),
fileMetadata == null ? null : expandedArtifact.artifact,
fileMetadata));
LocalFileType.forArtifact(expandedArtifact.artifact, metadata),
metadata == null ? null : expandedArtifact.artifact,
metadata));
} else {
// TODO(b/199940216): Can fileset metadata be properly handled here?
artifacts.add(
Expand All @@ -96,20 +96,6 @@ public Collection<LocalFile> referencedLocalFiles() {
return artifacts.build();
}

private static LocalFileType getOutputType(@Nullable FileArtifactValue fileMetadata) {
if (fileMetadata == null) {
return LocalFileType.OUTPUT;
}
switch (fileMetadata.getType()) {
case DIRECTORY:
return LocalFileType.OUTPUT_DIRECTORY;
case SYMLINK:
return LocalFileType.OUTPUT_SYMLINK;
default:
return LocalFileType.OUTPUT_FILE;
}
}

@Override
public BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventContext converters) {
PathConverter pathConverter = converters.pathConverter();
Expand Down
56 changes: 24 additions & 32 deletions src/test/java/com/google/devtools/build/lib/analysis/TargetCompleteEventTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,8 @@ public void testReferencedSourceFile() throws Exception {
artifactsToBuild.getAllArtifactsByOutputGroup(),
/* announceTargetSummary= */ false);

assertThat(event.referencedLocalFiles()).hasSize(1);
LocalFile localFile = event.referencedLocalFiles().get(0);
assertThat(localFile.path).isEqualTo(artifact.getPath());
assertThat(localFile.type).isEqualTo(LocalFileType.OUTPUT_FILE);
assertThat(event.referencedLocalFiles()).containsExactly(
new LocalFile(artifact.getPath(), LocalFileType.OUTPUT_FILE, artifact, metadata));
}

@Test
Expand All @@ -97,11 +95,8 @@ public void testReferencedSourceDirectory() throws Exception {
artifactsToBuild.getAllArtifactsByOutputGroup(),
/* announceTargetSummary= */ false);

assertThat(event.referencedLocalFiles()).hasSize(1);
LocalFile localFile = event.referencedLocalFiles().get(0);
assertThat(localFile.path).isEqualTo(artifact.getPath());
// TODO(tjgq): This should be reported as a directory.
assertThat(localFile.type).isEqualTo(LocalFileType.OUTPUT_FILE);
assertThat(event.referencedLocalFiles()).containsExactly(
new LocalFile(artifact.getPath(), LocalFileType.OUTPUT_DIRECTORY, artifact, metadata));
}

@Test
Expand All @@ -110,10 +105,7 @@ public void testReferencedTreeArtifact() throws Exception {
"defs.bzl",
"def _impl(ctx):",
" d = ctx.actions.declare_directory(ctx.label.name)",
" ctx.actions.run_shell(",
" outputs = [d],",
" command = 'touch %s/file.txt' % d.path,",
" )",
" ctx.actions.run_shell(outputs = [d], command = 'does not matter')",
" return DefaultInfo(files = depset([d]))",
"dir = rule(_impl)");
scratch.file(
Expand All @@ -123,16 +115,23 @@ public void testReferencedTreeArtifact() throws Exception {
"filegroup(name = 'files', srcs = ['dir'])");
ConfiguredTargetAndData ctAndData = getCtAndData("//:files");
ArtifactsToBuild artifactsToBuild = getArtifactsToBuild(ctAndData);
SpecialArtifact artifact =
SpecialArtifact tree =
(SpecialArtifact) Iterables.getOnlyElement(artifactsToBuild.getAllArtifacts().toList());
TreeFileArtifact fileChild = TreeFileArtifact.createTreeOutput(tree,
PathFragment.create("dir/file.txt"));
FileArtifactValue fileMetadata = FileArtifactValue.createForNormalFile(new byte[]{1, 2, 3},
null, 10);
// A TreeFileArtifact can be a directory, when materialized by a symlink.
// See https://github.com/bazelbuild/bazel/issues/20418.
TreeFileArtifact dirChild = TreeFileArtifact.createTreeOutput(tree, PathFragment.create("sym"));
FileArtifactValue dirMetadata = FileArtifactValue.createForDirectoryWithMtime(123456789);
TreeArtifactValue metadata =
TreeArtifactValue.newBuilder(artifact)
.putChild(
TreeFileArtifact.createTreeOutput(artifact, PathFragment.create("file")),
FileArtifactValue.createForNormalFile(new byte[] {1, 2, 3}, null, 10))
TreeArtifactValue.newBuilder(tree)
.putChild(fileChild, fileMetadata)
.putChild(dirChild, dirMetadata)
.build();
CompletionContext completionContext =
getCompletionContext(ImmutableMap.of(), ImmutableMap.of(artifact, metadata));
getCompletionContext(ImmutableMap.of(), ImmutableMap.of(tree, metadata));

TargetCompleteEvent event =
TargetCompleteEvent.successfulBuild(
Expand All @@ -141,11 +140,9 @@ public void testReferencedTreeArtifact() throws Exception {
artifactsToBuild.getAllArtifactsByOutputGroup(),
/* announceTargetSummary= */ false);

assertThat(event.referencedLocalFiles()).hasSize(1);
LocalFile localFile = event.referencedLocalFiles().get(0);
assertThat(localFile.path)
.isEqualTo(Iterables.getOnlyElement(metadata.getChildren()).getPath());
assertThat(localFile.type).isEqualTo(LocalFileType.OUTPUT_FILE);
assertThat(event.referencedLocalFiles()).containsExactly(
new LocalFile(fileChild.getPath(), LocalFileType.OUTPUT_FILE, fileChild, fileMetadata),
new LocalFile(dirChild.getPath(), LocalFileType.OUTPUT_DIRECTORY, dirChild, dirMetadata));
}

@Test
Expand All @@ -154,10 +151,7 @@ public void testReferencedUnresolvedSymlink() throws Exception {
"defs.bzl",
"def _impl(ctx):",
" s = ctx.actions.declare_symlink(ctx.label.name)",
" ctx.actions.symlink(",
" output = s,",
" target_path = '/some/path',",
" )",
" ctx.actions.symlink(output = s, target_path = 'does not matter')",
" return DefaultInfo(files = depset([s]))",
"sym = rule(_impl)");
scratch.file(
Expand All @@ -181,10 +175,8 @@ public void testReferencedUnresolvedSymlink() throws Exception {
artifactsToBuild.getAllArtifactsByOutputGroup(),
/* announceTargetSummary= */ false);

assertThat(event.referencedLocalFiles()).hasSize(1);
LocalFile localFile = event.referencedLocalFiles().get(0);
assertThat(localFile.path).isEqualTo(artifact.getPath());
assertThat(localFile.type).isEqualTo(LocalFileType.OUTPUT_SYMLINK);
assertThat(event.referencedLocalFiles()).containsExactly(
new LocalFile(artifact.getPath(), LocalFileType.OUTPUT_SYMLINK, artifact, metadata));
}

/** Regression test for b/165671166. */
Expand Down

0 comments on commit 370768c

Please sign in to comment.