bazelbuild · iancha1992 · Feb 1, 2024 · Jan 30, 2024
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
@@ -70,7 +70,7 @@ private interface ExecLogEntrySupplier {
   private final XattrProvider xattrProvider;
 
   // Maps a key identifying an entry into its ID.
-  // Each key is either a NestedSet.Node or the String path of a file or directory.
+  // Each key is either a NestedSet.Node or the String path of a file, directory or symlink.
   // Only entries that are likely to be referenced by future entries are stored.
   // Use a specialized map for minimal memory footprint.
   @GuardedBy("this")
@@ -143,17 +143,21 @@ public void logSpawn(
 
       for (ActionInput output : spawn.getOutputFiles()) {
         Path path = fileSystem.getPath(execRoot.getRelative(output.getExecPath()));
-        if (!path.exists()) {
+        if (!output.isDirectory() && !output.isSymlink() && path.isFile()) {
           builder.addOutputs(
-              ExecLogEntry.Output.newBuilder().setMissingPath(output.getExecPathString()));
-        } else if (path.isDirectory()) {
+              ExecLogEntry.Output.newBuilder()
+                  .setFileId(logFile(output, path, inputMetadataProvider)));
+        } else if (output.isDirectory() && path.isDirectory()) {
           builder.addOutputs(
               ExecLogEntry.Output.newBuilder()
                   .setDirectoryId(logDirectory(output, path, inputMetadataProvider)));
-        } else {
+        } else if (output.isSymlink() && path.isSymbolicLink()) {
           builder.addOutputs(
               ExecLogEntry.Output.newBuilder()
-                  .setFileId(logFile(output, path, inputMetadataProvider)));
+                  .setUnresolvedSymlinkId(logUnresolvedSymlink(output, path)));
+        } else {
+          builder.addOutputs(
+              ExecLogEntry.Output.newBuilder().setInvalidOutputPath(output.getExecPathString()));
         }
       }
 
@@ -335,12 +339,12 @@ private int logFile(ActionInput input, Path path, InputMetadataProvider inputMet
   /**
    * Logs a directory.
    *
-   * <p>This may be either a source directory, a fileset or an output directory. An output directory
-   * is not guaranteed to be a tree artifact; we must support the case where a directory is produced
-   * when a file was expected. For runfiles, {@link #logRunfilesDirectory} must be used instead.
+   * <p>This may be either a source directory, a fileset or an output directory. For runfiles,
+   * {@link #logRunfilesDirectory} must be used instead.
    *
-   * @param input the input representing the directory
-   * @param root the path to the directory
+   * @param input the input representing the directory.
+   * @param root the path to the directory, which must have already been verified to be of the
+   *     correct type.
    * @return the entry ID of the {@link ExecLogEntry.Directory} describing the directory.
    */
   private int logDirectory(
@@ -455,6 +459,26 @@ private ImmutableList<ExecLogEntry.File> expandDirectory(
     return builder.build();
   }
 
+  /**
+   * Logs an unresolved symlink.
+   *
+   * @param input the input representing the unresolved symlink.
+   * @param path the path to the unresolved symlink, which must have already been verified to be of
+   *     the correct type.
+   * @return the entry ID of the {@link ExecLogEntry.UnresolvedSymlink} describing the unresolved
+   *     symlink.
+   */
+  private int logUnresolvedSymlink(ActionInput input, Path path) throws IOException {
+    return logEntry(
+        input.getExecPathString(),
+        () ->
+            ExecLogEntry.newBuilder()
+                .setUnresolvedSymlink(
+                    ExecLogEntry.UnresolvedSymlink.newBuilder()
+                        .setPath(input.getExecPathString())
+                        .setTargetPath(path.readSymbolicLink().getPathString())));
+  }
+
   /**
    * Ensures an entry is written to the log and returns its assigned ID.
    *

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
@@ -24,6 +24,7 @@
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.actions.ExecException;
+import com.google.devtools.build.lib.actions.FileArtifactValue.UnresolvedSymlinkArtifactValue;
 import com.google.devtools.build.lib.actions.InputMetadataProvider;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnResult;
@@ -56,7 +57,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.SortedMap;
-import java.util.TreeMap;
 import java.util.function.Consumer;
 import javax.annotation.Nullable;
 
@@ -144,7 +144,6 @@ public void logSpawn(
       SpawnResult result)
       throws IOException, ExecException {
     try (SilentCloseable c = Profiler.instance().profile("logSpawn")) {
-      SortedMap<Path, ActionInput> existingOutputs = listExistingOutputs(spawn, fileSystem);
       SpawnExec.Builder builder = SpawnExec.newBuilder();
       builder.addAllCommandArgs(spawn.getArguments());
       builder.addAllEnvironmentVariables(getEnvironmentVariables(spawn));
@@ -175,6 +174,16 @@ public void logSpawn(
             continue;
           }
 
+          if (input.isSymlink()) {
+            UnresolvedSymlinkArtifactValue metadata =
+                (UnresolvedSymlinkArtifactValue) inputMetadataProvider.getInputMetadata(input);
+            builder
+                .addInputsBuilder()
+                .setPath(displayPath.getPathString())
+                .setSymlinkTargetPath(metadata.getSymlinkTarget());
+            continue;
+          }
+
           Digest digest =
               computeDigest(
                   input,
@@ -201,29 +210,33 @@ public void logSpawn(
         Collections.sort(outputPaths);
         builder.addAllListedOutputs(outputPaths);
         try {
-          for (Map.Entry<Path, ActionInput> e : existingOutputs.entrySet()) {
-            Path path = e.getKey();
-            ActionInput output = e.getValue();
-            if (path.isDirectory()) {
+          for (ActionInput output : spawn.getOutputFiles()) {
+            Path path = fileSystem.getPath(execRoot.getRelative(output.getExecPathString()));
+            if (!output.isDirectory() && !output.isSymlink() && path.isFile()) {
+              builder
+                  .addActualOutputsBuilder()
+                  .setPath(output.getExecPathString())
+                  .setDigest(
+                      computeDigest(
+                          output,
+                          path,
+                          inputMetadataProvider,
+                          xattrProvider,
+                          digestHashFunction,
+                          /* includeHashFunctionName= */ true));
+            } else if (output.isDirectory() && path.isDirectory()) {
               listDirectoryContents(
                   output.getExecPath(),
                   path,
                   builder::addActualOutputs,
                   inputMetadataProvider,
                   /* isTool= */ false);
-              continue;
+            } else if (output.isSymlink() && path.isSymbolicLink()) {
+              builder
+                  .addActualOutputsBuilder()
+                  .setPath(output.getExecPathString())
+                  .setSymlinkTargetPath(path.readSymbolicLink().getPathString());
             }
-            builder
-                .addActualOutputsBuilder()
-                .setPath(output.getExecPathString())
-                .setDigest(
-                    computeDigest(
-                        output,
-                        path,
-                        inputMetadataProvider,
-                        xattrProvider,
-                        digestHashFunction,
-                        /* includeHashFunctionName= */ true));
           }
         } catch (IOException ex) {
           logger.atWarning().withCause(ex).log("Error computing spawn output properties");
@@ -292,18 +305,6 @@ public void close() throws IOException {
     }
   }
 
-  private SortedMap<Path, ActionInput> listExistingOutputs(Spawn spawn, FileSystem fileSystem) {
-    TreeMap<Path, ActionInput> result = new TreeMap<>();
-    for (ActionInput output : spawn.getOutputFiles()) {
-      Path outputPath = fileSystem.getPath(execRoot.getRelative(output.getExecPathString()));
-      // TODO(olaola): once symlink API proposal is implemented, report symlinks here.
-      if (outputPath.exists()) {
-        result.put(outputPath, output);
-      }
-    }
-    return result;
-  }
-
   /**
    * Expands a directory into its contents.
    *
@@ -318,7 +319,6 @@ private void listDirectoryContents(
       InputMetadataProvider inputMetadataProvider,
       boolean isTool)
       throws IOException {
-    // TODO(olaola): once symlink API proposal is implemented, report symlinks here.
     List<Dirent> sortedDirent = new ArrayList<>(contentPath.readdir(Symlinks.NOFOLLOW));
     sortedDirent.sort(Comparator.comparing(Dirent::getName));
 

diff --git a/src/main/protobuf/spawn.proto b/src/main/protobuf/spawn.proto
@@ -42,8 +42,12 @@ message File {
   // Path to the file relative to the execution root.
   string path = 1;
 
+  // Symlink target path.
+  // Only set for unresolved symlinks.
+  string symlink_target_path = 4;
+
   // File digest.
-  // May be omitted for empty files.
+  // Always omitted for unresolved symlinks. May be omitted for empty files.
   Digest digest = 2;
 
   // Whether the file is a tool.
@@ -147,6 +151,8 @@ message SpawnExec {
   string mnemonic = 10;
 
   // The outputs generated by the execution.
+  // In order for one of the listed_outputs to appear here, it must have been
+  // produced and have the expected type (file, directory or symlink).
   repeated File actual_outputs = 11;
 
   // If the spawn did not hit a disk or remote cache, this will be the name of
@@ -225,13 +231,22 @@ message ExecLogEntry {
   }
 
   // An input or output directory.
+  // May be a source directory, a runfiles or fileset tree, or a tree artifact.
   message Directory {
     // The directory path.
     string path = 1;
     // The contained files, whose paths are relative to the directory.
     repeated File files = 2;
   }
 
+  // An unresolved symlink.
+  message UnresolvedSymlink {
+    // The symlink path.
+    string path = 1;
+    // The path the symlink points to.
+    string target_path = 2;
+  }
+
   // A set of spawn inputs.
   // The contents of the set are the directly referenced files and directories
   // in addition to the contents of all transitively referenced sets. Sets are
@@ -249,12 +264,15 @@ message ExecLogEntry {
   // A spawn output.
   message Output {
     oneof type {
-      // An output file.
+      // An output file, i.e., ctx.actions.declare_file.
       int32 file_id = 1;
-      // An output directory.
+      // An output directory, i.e., ctx.actions.declare_directory.
       int32 directory_id = 2;
-      // A declared output that was not produced.
-      string missing_path = 3;
+      // An output unresolved symlink, i.e., ctx.actions.declare_symlink.
+      int32 unresolved_symlink_id = 3;
+      // A declared output that is either missing or has the wrong type
+      // (e.g., a file where a directory was expected).
+      string invalid_output_path = 4;
     }
   }
 
@@ -325,7 +343,8 @@ message ExecLogEntry {
     Invocation invocation = 2;
     File file = 3;
     Directory directory = 4;
-    InputSet input_set = 5;
-    Spawn spawn = 6;
+    UnresolvedSymlink unresolved_symlink = 5;
+    InputSet input_set = 6;
+    Spawn spawn = 7;
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/exec/CompactSpawnLogContextTest.java b/src/test/java/com/google/devtools/build/lib/exec/CompactSpawnLogContextTest.java
@@ -198,8 +198,14 @@ private SpawnExec reconstructSpawnExec(
             builder.addActualOutputs(reconstructFile(dir, dirFile, hashFunctionName));
           }
           break;
-        case MISSING_PATH:
-          builder.addListedOutputs(output.getMissingPath());
+        case UNRESOLVED_SYMLINK_ID:
+          ExecLogEntry.UnresolvedSymlink symlink =
+              checkNotNull(entryMap.get(output.getUnresolvedSymlinkId())).getUnresolvedSymlink();
+          builder.addListedOutputs(symlink.getPath());
+          builder.addActualOutputs(reconstructSymlink(symlink));
+          break;
+        case INVALID_OUTPUT_PATH:
+          builder.addListedOutputs(output.getInvalidOutputPath());
           break;
         case TYPE_NOT_SET:
           throw new AssertionError("malformed log entry");
@@ -252,4 +258,11 @@ private File reconstructFile(
 
     return builder.build();
   }
+
+  private File reconstructSymlink(ExecLogEntry.UnresolvedSymlink symlink) {
+    return File.newBuilder()
+        .setPath(symlink.getPath())
+        .setSymlinkTargetPath(symlink.getTargetPath())
+        .build();
+  }
 }