diff --git a/src/main/java/com/google/devtools/build/lib/exec/CompactSpawnLogContext.java b/src/main/java/com/google/devtools/build/lib/exec/CompactSpawnLogContext.java index ebb18a8a2dd1ef..69aa332fdd4332 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/CompactSpawnLogContext.java +++ b/src/main/java/com/google/devtools/build/lib/exec/CompactSpawnLogContext.java @@ -222,7 +222,8 @@ public void logSpawn( builder.addOutputs( ExecLogEntry.Output.newBuilder() .setFileId(logFile(output, path, inputMetadataProvider))); - } else if (output.isDirectory() && path.isDirectory()) { + } else if (!output.isSymlink() && path.isDirectory()) { + // TODO(tjgq): Tighten once --incompatible_disallow_unsound_directory_outputs is gone. builder.addOutputs( ExecLogEntry.Output.newBuilder() .setDirectoryId(logDirectory(output, path, inputMetadataProvider))); @@ -366,7 +367,7 @@ private int logNestedSet( continue; } Path path = fileSystem.getPath(execRoot.getRelative(input.getExecPath())); - if (isInputDirectory(input, inputMetadataProvider)) { + if (isInputDirectory(input, path, inputMetadataProvider)) { builder.addDirectoryIds(logDirectory(input, path, inputMetadataProvider)); } else if (input.isSymlink()) { builder.addUnresolvedSymlinkIds(logUnresolvedSymlink(input, path)); @@ -474,7 +475,7 @@ private int logRunfilesDirectory( Path path = fileSystem.getPath(execRoot.getRelative(input.getExecPath())); - if (isInputDirectory(input, inputMetadataProvider)) { + if (isInputDirectory(input, path, inputMetadataProvider)) { builder.addAllFiles(expandDirectory(path, runfilesPath, inputMetadataProvider)); continue; } diff --git a/src/main/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContext.java b/src/main/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContext.java index ec4a7fa28e0789..53c538cb6d08fd 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContext.java +++ b/src/main/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContext.java @@ -174,7 +174,7 @@ public void logSpawn( Path contentPath = fileSystem.getPath(execRoot.getRelative(input.getExecPathString())); - if (isInputDirectory(input, inputMetadataProvider)) { + if (isInputDirectory(input, contentPath, inputMetadataProvider)) { listDirectoryContents( displayPath, contentPath, builder::addInputs, inputMetadataProvider, isTool); continue; @@ -231,7 +231,8 @@ public void logSpawn( xattrProvider, digestHashFunction, /* includeHashFunctionName= */ true)); - } else if (output.isDirectory() && path.isDirectory()) { + } else if (!output.isSymlink() && path.isDirectory()) { + // TODO(tjgq): Tighten once --incompatible_disallow_unsound_directory_outputs is gone. listDirectoryContents( output.getExecPath(), path, diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java index 1ea5a0518ccfc1..413f0bc0dce424 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java @@ -13,8 +13,6 @@ // limitations under the License. package com.google.devtools.build.lib.exec; -import static com.google.common.base.Preconditions.checkNotNull; - import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -22,7 +20,6 @@ import com.google.common.hash.HashCode; import com.google.devtools.build.lib.actions.ActionContext; import com.google.devtools.build.lib.actions.ActionInput; -import com.google.devtools.build.lib.actions.Artifact.SourceArtifact; import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.InputMetadataProvider; @@ -115,22 +112,26 @@ protected Platform getPlatform(Spawn spawn, RemoteOptions remoteOptions) * *

Do not call for action outputs. */ - protected boolean isInputDirectory(ActionInput input, InputMetadataProvider inputMetadataProvider) + protected boolean isInputDirectory( + ActionInput input, Path path, InputMetadataProvider inputMetadataProvider) throws IOException { if (input.isDirectory()) { return true; } - if (input.isSymlink() || !(input instanceof SourceArtifact)) { + if (input.isSymlink()) { return false; } - // A source artifact may be a directory in spite of claiming to be a file. Avoid unnecessary I/O - // by inspecting its metadata, which should have already been collected and cached. - FileArtifactValue metadata = - checkNotNull( - inputMetadataProvider.getInputMetadata(input), - "missing metadata for %s", - input.getExecPath()); - return metadata.getType().isDirectory(); + // There are two cases in which an input's declared type may disagree with the filesystem: + // (1) a source artifact pointing to a directory; + // (2) an output artifact declared as a file but materialized as a directory, when allowed by + // --noincompatible_disallow_unsound_directory_outputs. + // Try to avoid unnecessary I/O by inspecting its metadata, which in most cases should have + // already been collected and cached. + FileArtifactValue metadata = inputMetadataProvider.getInputMetadata(input); + if (metadata != null) { + return metadata.getType().isDirectory(); + } + return path.isDirectory(); } /** diff --git a/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java b/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java index 69a057038cf144..0cc5408d98fc0d 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java @@ -166,6 +166,47 @@ public void testFileInput(@TestParameter InputsMode inputsMode) throws Exception .build()); } + @Test + public void testFileInputWithDirectoryContents( + @TestParameter InputsMode inputsMode, @TestParameter DirContents dirContents) + throws Exception { + Artifact fileInput = ActionsTestUtil.createArtifact(rootDir, "file"); + + fileInput.getPath().createDirectoryAndParents(); + if (!dirContents.isEmpty()) { + writeFile(fileInput.getPath().getChild("file"), "abc"); + } + + SpawnBuilder spawn = defaultSpawnBuilder().withInputs(fileInput); + if (inputsMode.isTool()) { + spawn.withTools(fileInput); + } + + SpawnLogContext context = createSpawnLogContext(); + + context.logSpawn( + spawn.build(), + createInputMetadataProvider(fileInput), + createInputMap(fileInput), + fs, + defaultTimeout(), + defaultSpawnResult()); + + closeAndAssertLog( + context, + defaultSpawnExecBuilder() + .addAllInputs( + dirContents.isEmpty() + ? ImmutableList.of() + : ImmutableList.of( + File.newBuilder() + .setPath("file/file") + .setDigest(getDigest("abc")) + .setIsTool(inputsMode.isTool()) + .build())) + .build()); + } + @Test public void testDirectoryInput( @TestParameter InputsMode inputsMode, @TestParameter DirContents dirContents) @@ -465,11 +506,12 @@ public void testFileOutput( } @Test - public void testFileOutputWithInvalidType(@TestParameter OutputsMode outputsMode) + public void testFileOutputWithDirectoryContents(@TestParameter OutputsMode outputsMode) throws Exception { Artifact fileOutput = ActionsTestUtil.createArtifact(outputDir, "file"); fileOutput.getPath().createDirectoryAndParents(); + writeFile(fileOutput.getPath().getChild("file"), "abc"); SpawnBuilder spawn = defaultSpawnBuilder().withOutputs(fileOutput); @@ -483,7 +525,13 @@ public void testFileOutputWithInvalidType(@TestParameter OutputsMode outputsMode defaultTimeout(), defaultSpawnResult()); - closeAndAssertLog(context, defaultSpawnExecBuilder().addListedOutputs("out/file").build()); + closeAndAssertLog( + context, + defaultSpawnExecBuilder() + .addListedOutputs("out/file") + .addActualOutputs( + File.newBuilder().setPath("out/file/file").setDigest(getDigest("abc"))) + .build()); } @Test