Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.0.0] "Cannot get node id for DirectoryArtifactValue" on tree artifact containing symlink to directory + remote cache + BES #20424

Merged
merged 1 commit into from
Dec 4, 2023
Merged

[7.0.0] "Cannot get node id for DirectoryArtifactValue" on tree artifact containing symlink to directory + remote cache + BES #20424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/com/google/devtools/build/lib/actions/ActionExecutedEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,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
Open in desktop
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
17 changes: 16 additions & 1 deletion src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,22 @@ public boolean isOutput() {
|| this == OUTPUT_SYMLINK;
}

public static LocalFileType forArtifact(Artifact artifact) {
/**
* Returns the {@link LocalFileType} implied by a {@link FileArtifactValue}, or the associated
* {@link Artifact} if metadata 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
Open in desktop
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
61 changes: 29 additions & 32 deletions src/test/java/com/google/devtools/build/lib/analysis/TargetCompleteEventTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,9 @@ 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 +96,9 @@ 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 +107,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 +117,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 +142,11 @@ 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 +155,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 +179,9 @@ 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