[7.1.0] Correctly handle file inputs/outputs with directory contents …

…in the execution log. (#21427)

9fd9aa5
was a little too conservative: it didn't account for inputs/outputs
declared as a file but materialized as a directory, which are still
permitted by --noincompatible_disallow_unsound_directory_outputs.

Also fall back to the filesystem if input metadata isn't available to
make the determination. This shouldn't happen most of the time, as
metadata should already be available for most inputs, but there are some
hard-to-fix edge cases where it doesn't hold.

PiperOrigin-RevId: 608584354
Change-Id: If9a4c6df4319ef66f487068dd89b382eea1b425e

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;
             }

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,

src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java

@@ -13,16 +13,13 @@
 // 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;
 import com.google.common.collect.ImmutableSortedMap;
 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)
    *
    * <p>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();
   }
 
   /**

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