From 8e2dfcda3debcb5a1aac507a9bf8a5ded6203bc2 Mon Sep 17 00:00:00 2001 From: "bazel.build machine account" <15028808+bazel-io@users.noreply.github.com> Date: Mon, 4 Dec 2023 13:29:20 +0100 Subject: [PATCH] [7.0.0] "Cannot get node id for DirectoryArtifactValue" on tree artifact containing symlink to directory + remote cache + BES (#20424) When a tree artifact contains a symlink to a directory, the TreeArtifactValue contains a TreeFileArtifact mapping to a DirectoryArtifactValue (see #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 #20415). This PR fixes the file type computation to take into account both the artifact and its metadata, preferring the latter when available. Fixes #20415. Closes #20419. Commit https://github.com/bazelbuild/bazel/commit/bb5ff63d18abda883f2ccf7cf47e009ef6012420 PiperOrigin-RevId: 587654070 Change-Id: Ib62bbaaed62b00c12bcabdc2bc9bee57aee0bcef Co-authored-by: Tiago Quelhas --- .../lib/actions/ActionExecutedEvent.java | 2 +- .../lib/analysis/TargetCompleteEvent.java | 5 +- .../lib/buildeventstream/BuildEvent.java | 17 +++++- .../build/lib/runtime/NamedArtifactGroup.java | 22 ++----- .../lib/analysis/TargetCompleteEventTest.java | 61 +++++++++---------- 5 files changed, 53 insertions(+), 54 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutedEvent.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutedEvent.java index a94aa698d497ac..03c93f734288a4 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutedEvent.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutedEvent.java @@ -173,7 +173,7 @@ public Collection referencedLocalFiles() { localFiles.add( new LocalFile( primaryOutput, - LocalFileType.forArtifact(outputArtifact), + LocalFileType.forArtifact(outputArtifact, primaryOutputMetadata), outputArtifact, primaryOutputMetadata)); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java b/src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java index 3adf040fc35bd0..5b3ee7871a0147 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java @@ -406,12 +406,13 @@ public ImmutableList 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 diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEvent.java b/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEvent.java index 89dcdb36304903..08c8a86516c3fe 100644 --- a/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEvent.java +++ b/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEvent.java @@ -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()) { diff --git a/src/main/java/com/google/devtools/build/lib/runtime/NamedArtifactGroup.java b/src/main/java/com/google/devtools/build/lib/runtime/NamedArtifactGroup.java index 66ba8919af86e5..ea2f29758dd558 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/NamedArtifactGroup.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/NamedArtifactGroup.java @@ -75,14 +75,14 @@ public Collection 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( @@ -96,20 +96,6 @@ public Collection 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(); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/TargetCompleteEventTest.java b/src/test/java/com/google/devtools/build/lib/analysis/TargetCompleteEventTest.java index 53defdf74df67f..f9d1e9e73bf98d 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/TargetCompleteEventTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/TargetCompleteEventTest.java @@ -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 @@ -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 @@ -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( @@ -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( @@ -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 @@ -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( @@ -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. */