Merge branch 'release-7.1.0' into cp20387-7.1.0

src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java

@@ -345,7 +345,7 @@ public void maybeReportSubcommand(Spawn spawn) {
             /* environmentVariablesToClear= */ null,
             getExecRoot().getPathString(),
             spawn.getConfigurationChecksum(),
-            spawn.getExecutionPlatformLabelString());
+            spawn.getExecutionPlatformLabel());
     getEventHandler().handle(Event.of(EventKind.SUBCOMMAND, null, "# " + reason + "\n" + message));
   }
 

src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java

@@ -21,7 +21,10 @@
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
+import java.util.TreeSet;
 
 /** Helper utility to create ActionInput instances. */
 public final class ActionInputHelper {
@@ -122,14 +125,24 @@ public static List<ActionInput> expandArtifacts(
       ArtifactExpander artifactExpander,
       boolean keepEmptyTreeArtifacts) {
     List<ActionInput> result = new ArrayList<>();
+    Set<Artifact> emptyTreeArtifacts = new TreeSet<>();
+    Set<Artifact> treeFileArtifactParents = new HashSet<>();
     for (ActionInput input : inputs.toList()) {
       if (input instanceof Artifact) {
-        Artifact.addExpandedArtifact(
-            (Artifact) input, result, artifactExpander, keepEmptyTreeArtifacts);
+        Artifact inputArtifact = (Artifact) input;
+        Artifact.addExpandedArtifact(inputArtifact, result, artifactExpander, emptyTreeArtifacts);
+        if (inputArtifact.isChildOfDeclaredDirectory()) {
+          treeFileArtifactParents.add(inputArtifact.getParent());
+        }
       } else {
         result.add(input);
       }
     }
+
+    if (keepEmptyTreeArtifacts) {
+      emptyTreeArtifacts.removeAll(treeFileArtifactParents);
+      result.addAll(emptyTreeArtifacts);
+    }
     return result;
   }
 

src/main/java/com/google/devtools/build/lib/actions/Artifact.java

@@ -59,6 +59,7 @@
 import java.util.Comparator;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.function.UnaryOperator;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.EvalException;
@@ -1544,21 +1545,20 @@ public static String joinRootRelativePaths(String delimiter, Iterable<Artifact>
   /**
    * Adds an artifact to a collection, expanding it once if it's a middleman or tree artifact.
    *
-   * <p>A middleman artifact is never added to the collection. If {@code keepEmptyTreeArtifacts} is
-   * true, a tree artifact will be added to the collection when it expands into zero file artifacts.
-   * Otherwise, only the file artifacts the tree artifact expands into will be added.
+   * <p>The middleman or tree artifact is never added to the output collection. If a tree artifact
+   * expands into zero file artifacts, it is added to emptyTreeArtifacts.
    */
   static void addExpandedArtifact(
       Artifact artifact,
       Collection<? super Artifact> output,
       ArtifactExpander artifactExpander,
-      boolean keepEmptyTreeArtifacts) {
+      Set<Artifact> emptyTreeArtifacts) {
     if (artifact.isMiddlemanArtifact() || artifact.isTreeArtifact()) {
       List<Artifact> expandedArtifacts = new ArrayList<>();
       artifactExpander.expand(artifact, expandedArtifacts);
       output.addAll(expandedArtifacts);
-      if (keepEmptyTreeArtifacts && artifact.isTreeArtifact() && expandedArtifacts.isEmpty()) {
-        output.add(artifact);
+      if (artifact.isTreeArtifact() && expandedArtifacts.isEmpty()) {
+        emptyTreeArtifacts.add(artifact);
       }
     } else {
       output.add(artifact);

src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java

@@ -169,11 +169,19 @@ protected boolean couldBeModifiedByMetadata(FileArtifactValue lastKnown) {
   /**
    * Optional materialization path.
    *
-   * <p>If present, this artifact is a copy of another artifact. It is still tracked as a
-   * non-symlink by Bazel, but materialized in the local filesystem as a symlink to the original
-   * artifact, whose contents live at this location. This is used by {@link
-   * com.google.devtools.build.lib.remote.AbstractActionInputPrefetcher} to implement zero-cost
-   * copies of remotely stored artifacts.
+   * <p>If present, this artifact is a copy of another artifact whose contents live at this path.
+   * This can happen when it is declared as a file and not as an unresolved symlink but the action
+   * that creates it materializes it in the filesystem as a symlink to another output artifact. This
+   * information is useful in two situations:
+   *
+   * <ol>
+   *   <li>When the symlink target is a remotely stored artifact, we can avoid downloading it
+   *       multiple times when building without the bytes (see AbstractActionInputPrefetcher).
+   *   <li>When the symlink target is inaccessible from the sandboxed environment an action runs
+   *       under, we can rewrite it accordingly (see SandboxHelpers).
+   * </ol>
+   *
+   * @see com.google.devtools.build.lib.skyframe.TreeArtifactValue#getMaterializationExecPath().
    */
   public Optional<PathFragment> getMaterializationExecPath() {
     return Optional.empty();
@@ -214,6 +222,12 @@ public static FileArtifactValue createForSourceArtifact(
         xattrProvider);
   }
 
+  public static FileArtifactValue createForResolvedSymlink(
+      PathFragment realPath, FileArtifactValue metadata, @Nullable byte[] digest) {
+    return new ResolvedSymlinkFileArtifactValue(
+        realPath, digest, metadata.getContentsProxy(), metadata.getSize());
+  }
+
   public static FileArtifactValue createFromInjectedDigest(
       FileArtifactValue metadata, @Nullable byte[] digest) {
     return createForNormalFile(digest, metadata.getContentsProxy(), metadata.getSize());
@@ -439,7 +453,25 @@ public String toString() {
     }
   }
 
-  private static final class RegularFileArtifactValue extends FileArtifactValue {
+  private static final class ResolvedSymlinkFileArtifactValue extends RegularFileArtifactValue {
+    private final PathFragment realPath;
+
+    private ResolvedSymlinkFileArtifactValue(
+        PathFragment realPath,
+        @Nullable byte[] digest,
+        @Nullable FileContentsProxy proxy,
+        long size) {
+      super(digest, proxy, size);
+      this.realPath = realPath;
+    }
+
+    @Override
+    public Optional<PathFragment> getMaterializationExecPath() {
+      return Optional.of(realPath);
+    }
+  }
+
+  private static class RegularFileArtifactValue extends FileArtifactValue {
     private final byte[] digest;
     @Nullable private final FileContentsProxy proxy;
     private final long size;
@@ -462,7 +494,8 @@ public boolean equals(Object o) {
       RegularFileArtifactValue that = (RegularFileArtifactValue) o;
       return Arrays.equals(digest, that.digest)
           && Objects.equals(proxy, that.proxy)
-          && size == that.size;
+          && size == that.size
+          && Objects.equals(getMaterializationExecPath(), that.getMaterializationExecPath());
     }
 
     @Override

src/main/java/com/google/devtools/build/lib/actions/Spawn.java

@@ -21,7 +21,6 @@
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.util.DescribableExecutionUnit;
 import java.util.Collection;
-import java.util.Objects;
 import javax.annotation.Nullable;
 
 /**
@@ -171,9 +170,9 @@ default boolean isMandatoryOutput(ActionInput output) {
 
   @Override
   @Nullable
-  default String getExecutionPlatformLabelString() {
+  default Label getExecutionPlatformLabel() {
     PlatformInfo executionPlatform = getExecutionPlatform();
-    return executionPlatform == null ? null : Objects.toString(executionPlatform.label());
+    return executionPlatform != null ? executionPlatform.label() : null;
   }
 
   @Override
@@ -184,9 +183,8 @@ default String getConfigurationChecksum() {
 
   @Override
   @Nullable
-  default String getTargetLabel() {
-    Label label = getResourceOwner().getOwner().getLabel();
-    return label == null ? null : label.toString();
+  default Label getTargetLabel() {
+    return getResourceOwner().getOwner().getLabel();
   }
 
   /**

src/main/java/com/google/devtools/build/lib/bazel/BUILD

@@ -129,7 +129,6 @@ java_library(
     ],
     deps = [
         "//src/main/java/com/google/devtools/build/lib:runtime",
-        "//src/main/java/com/google/devtools/build/lib/bazel/execlog:stable_sort",
         "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/exec:execution_options",
         "//src/main/java/com/google/devtools/build/lib/exec:executor_builder",
@@ -138,10 +137,8 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/remote/options",
         "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception",
         "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
-        "//src/main/java/com/google/devtools/build/lib/util/io",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/protobuf:failure_details_java_proto",
-        "//src/main/protobuf:spawn_java_proto",
         "//third_party:guava",
         "//third_party:jsr305",
     ],

src/main/java/com/google/devtools/build/lib/bazel/SpawnLogModule.java

@@ -15,13 +15,13 @@
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
-import com.google.devtools.build.lib.bazel.execlog.StableSort;
 import com.google.devtools.build.lib.buildtool.BuildRequest;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.exec.ExecutionOptions;
 import com.google.devtools.build.lib.exec.ExecutorBuilder;
+import com.google.devtools.build.lib.exec.ExpandedSpawnLogContext;
+import com.google.devtools.build.lib.exec.ExpandedSpawnLogContext.Encoding;
 import com.google.devtools.build.lib.exec.ModuleActionContextRegistry;
-import com.google.devtools.build.lib.exec.Protos.SpawnExec;
 import com.google.devtools.build.lib.exec.SpawnLogContext;
 import com.google.devtools.build.lib.remote.options.RemoteOptions;
 import com.google.devtools.build.lib.runtime.BlazeModule;
@@ -32,41 +32,16 @@
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
 import com.google.devtools.build.lib.util.AbruptExitException;
 import com.google.devtools.build.lib.util.DetailedExitCode;
-import com.google.devtools.build.lib.util.io.AsynchronousMessageOutputStream;
-import com.google.devtools.build.lib.util.io.MessageOutputStream;
-import com.google.devtools.build.lib.util.io.MessageOutputStreamWrapper.BinaryOutputStreamWrapper;
-import com.google.devtools.build.lib.util.io.MessageOutputStreamWrapper.JsonOutputStreamWrapper;
 import com.google.devtools.build.lib.vfs.Path;
 import java.io.IOException;
-import java.io.InputStream;
 import javax.annotation.Nullable;
 
 /** Module providing on-demand spawn logging. */
 public final class SpawnLogModule extends BlazeModule {
   @Nullable private SpawnLogContext spawnLogContext;
 
-  /** Output path for the raw output stream. */
-  @Nullable private Path rawOutputPath;
-
-  /** Output stream to write directly into during execution. */
-  @Nullable private MessageOutputStream<SpawnExec> rawOutputStream;
-
-  /**
-   * Output stream to convert the raw output into after the execution is done.
-   *
-   * <p>We open the stream at the beginning of the command so that any errors (e.g., unwritable
-   * location) are surfaced before execution begins.
-   */
-  @Nullable private MessageOutputStream<SpawnExec> convertedOutputStream;
-
-  private CommandEnvironment env;
-
   private void clear() {
     spawnLogContext = null;
-    rawOutputPath = null;
-    rawOutputStream = null;
-    convertedOutputStream = null;
-    env = null;
   }
 
   private void initOutputs(CommandEnvironment env) throws IOException {
@@ -99,45 +74,30 @@ private void initOutputs(CommandEnvironment env) throws IOException {
       return;
     }
 
-    this.env = env;
-
     Path workingDirectory = env.getWorkingDirectory();
     Path outputBase = env.getOutputBase();
 
-    // Set up the raw output stream.
-    // This stream performs the writes in a separate thread to avoid blocking execution.
-    // If the unsorted binary format was requested, use the respective output path to avoid a
-    // pointless conversion at the end. Otherwise, use a temporary path.
-    if (executionOptions.executionLogBinaryFile != null && !executionOptions.executionLogSort) {
-      rawOutputPath = workingDirectory.getRelative(executionOptions.executionLogBinaryFile);
-    } else {
-      rawOutputPath = outputBase.getRelative("execution.log");
-    }
-    rawOutputStream = new AsynchronousMessageOutputStream<>(rawOutputPath);
-
-    // Set up the binary output stream, if distinct from the raw output stream.
-    if (executionOptions.executionLogBinaryFile != null && executionOptions.executionLogSort) {
-      convertedOutputStream =
-          new BinaryOutputStreamWrapper<>(
-              workingDirectory
-                  .getRelative(executionOptions.executionLogBinaryFile)
-                  .getOutputStream());
+    Path outputPath = null;
+    Encoding encoding = null;
+    if (executionOptions.executionLogBinaryFile != null) {
+      encoding = Encoding.BINARY;
+      outputPath = workingDirectory.getRelative(executionOptions.executionLogBinaryFile);
+    } else if (executionOptions.executionLogJsonFile != null) {
+      encoding = Encoding.JSON;
+      outputPath = workingDirectory.getRelative(executionOptions.executionLogJsonFile);
     }
 
-    // Set up the text output stream.
-    if (executionOptions.executionLogJsonFile != null) {
-      convertedOutputStream =
-          new JsonOutputStreamWrapper<>(
-              workingDirectory
-                  .getRelative(executionOptions.executionLogJsonFile)
-                  .getOutputStream());
-    }
+    // Use a well-known temporary path to avoid accumulation of potentially large files in /tmp
+    // due to abnormally terminated invocations (e.g., when running out of memory).
+    Path tempPath = outputBase.getRelative("execution.log");
 
     spawnLogContext =
-        new SpawnLogContext(
+        new ExpandedSpawnLogContext(
+            checkNotNull(outputPath),
+            tempPath,
+            checkNotNull(encoding),
+            /* sorted= */ executionOptions.executionLogSort,
             env.getExecRoot().asFragment(),
-            rawOutputStream,
-            env.getOptions().getOptions(ExecutionOptions.class),
             env.getOptions().getOptions(RemoteOptions.class),
             env.getRuntime().getFileSystem().getDigestFunction(),
             env.getXattrProvider());
@@ -180,44 +140,13 @@ public void afterCommand() throws AbruptExitException {
       return;
     }
 
-    checkNotNull(rawOutputPath);
-
-    boolean done = false;
     try {
       spawnLogContext.close();
-      if (convertedOutputStream != null) {
-        InputStream in = rawOutputPath.getInputStream();
-        if (spawnLogContext.shouldSort()) {
-          StableSort.stableSort(in, convertedOutputStream);
-        } else {
-          while (in.available() > 0) {
-            SpawnExec ex = SpawnExec.parseDelimitedFrom(in);
-            convertedOutputStream.write(ex);
-          }
-        }
-        convertedOutputStream.close();
-      }
-      done = true;
     } catch (IOException e) {
       String message = e.getMessage() == null ? "Error writing execution log" : e.getMessage();
       throw new AbruptExitException(
           createDetailedExitCode(message, Code.EXECUTION_LOG_WRITE_FAILURE), e);
     } finally {
-      if (convertedOutputStream != null) {
-        if (!done) {
-          env.getReporter()
-              .handle(
-                  Event.warn(
-                      "Execution log might not have been populated. Raw execution log is at "
-                          + rawOutputPath));
-        } else {
-          try {
-            rawOutputPath.delete();
-          } catch (IOException e) {
-            // Intentionally ignored.
-          }
-        }
-      }
       clear();
     }
   }