Merge branch 'release-7.2.0' into leba-cherrypick

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

@@ -1652,6 +1652,7 @@ java_library(
         ":config/build_options",
         ":config/compilation_mode",
         ":config/core_options",
+        ":config/execution_info_modifier",
         ":config/feature_set",
         ":config/fragment",
         ":config/fragment_class_set",

src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java

@@ -362,7 +362,9 @@ public ArtifactRoot getOutputDirectory(RepositoryName repositoryName) {
     return outputDirectories.getOutputDirectory(repositoryName);
   }
 
-  /** @deprecated Use {@link #getBinDirectory} instead. */
+  /**
+   * @deprecated Use {@link #getBinDirectory} instead.
+   */
   @Override
   @Deprecated
   public ArtifactRoot getBinDir() {
@@ -897,7 +899,8 @@ public boolean remotableSourceManifestActions() {
    */
   public ImmutableMap<String, String> modifiedExecutionInfo(
       ImmutableMap<String, String> executionInfo, String mnemonic) {
-    if (!options.executionInfoModifier.matches(mnemonic)) {
+    if (!ExecutionInfoModifier.matches(
+        options.executionInfoModifier, options.additiveModifyExecutionInfo, mnemonic)) {
       return executionInfo;
     }
     Map<String, String> mutableCopy = new HashMap<>(executionInfo);
@@ -907,7 +910,11 @@ public ImmutableMap<String, String> modifiedExecutionInfo(
 
   /** Applies {@code executionInfoModifiers} to the given {@code executionInfo}. */
   public void modifyExecutionInfo(Map<String, String> executionInfo, String mnemonic) {
-    options.executionInfoModifier.apply(mnemonic, executionInfo);
+    ExecutionInfoModifier.apply(
+        options.executionInfoModifier,
+        options.additiveModifyExecutionInfo,
+        mnemonic,
+        executionInfo);
   }
 
   /** Returns the list of default features used for all packages. */

src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java

@@ -819,14 +819,15 @@ public OutputPathsConverter() {
 
   @Option(
       name = "modify_execution_info",
+      allowMultiple = true,
       converter = ExecutionInfoModifier.Converter.class,
+      defaultValue = "null",
       documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
       effectTags = {
         OptionEffectTag.EXECUTION,
         OptionEffectTag.AFFECTS_OUTPUTS,
         OptionEffectTag.LOADING_AND_ANALYSIS,
       },
-      defaultValue = "",
       help =
           "Add or remove keys from an action's execution info based on action mnemonic.  "
               + "Applies only to actions which support execution info. Many common actions "
@@ -841,7 +842,22 @@ public OutputPathsConverter() {
               + "all Genrule actions.\n"
               + "  '(?!Genrule).*=-requires-x' removes 'requires-x' from the execution info for "
               + "all non-Genrule actions.\n")
-  public ExecutionInfoModifier executionInfoModifier;
+  public List<ExecutionInfoModifier> executionInfoModifier;
+
+  @Option(
+      name = "incompatible_modify_execution_info_additive",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
+      effectTags = {
+        OptionEffectTag.EXECUTION,
+        OptionEffectTag.AFFECTS_OUTPUTS,
+        OptionEffectTag.LOADING_AND_ANALYSIS,
+      },
+      metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+      help =
+          "When enabled, passing multiple --modify_execution_info flags is additive."
+              + " When disabled, only the last flag is taken into account.")
+  public boolean additiveModifyExecutionInfo;
 
   @Option(
       name = "include_config_fragments_provider",
@@ -969,6 +985,7 @@ public FragmentOptions getExec() {
     exec.experimentalWritableOutputs = experimentalWritableOutputs;
     exec.strictConflictChecks = strictConflictChecks;
     exec.disallowUnsoundDirectoryOutputs = disallowUnsoundDirectoryOutputs;
+    exec.additiveModifyExecutionInfo = additiveModifyExecutionInfo;
 
     // === Output path calculation
     exec.outputDirectoryNamingScheme = outputDirectoryNamingScheme;

src/main/java/com/google/devtools/build/lib/analysis/config/ExecutionInfoModifier.java

@@ -21,6 +21,7 @@
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.common.options.Converters.RegexPatternConverter;
 import com.google.devtools.common.options.OptionsParsingException;
+import java.util.List;
 import java.util.Map;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -38,6 +39,7 @@ public abstract class ExecutionInfoModifier {
   private static final ExecutionInfoModifier EMPTY = create("", ImmutableList.of());
 
   abstract String option();
+
   abstract ImmutableList<Expression> expressions();
 
   @AutoValue
@@ -77,7 +79,7 @@ public ExecutionInfoModifier convert(String input) throws OptionsParsingExceptio
                 // Convert to get a useful exception if it's not a valid pattern, but use the regex
                 // (see comment in Expression)
                 new RegexPatternConverter()
-                    .convert(specMatcher.group("pattern"), /*conversionContext=*/ null)
+                    .convert(specMatcher.group("pattern"), /* conversionContext= */ null)
                     .regexPattern()
                     .pattern(),
                 specMatcher.group("sign").equals("-"),
@@ -99,10 +101,41 @@ private static ExecutionInfoModifier create(String input, ImmutableList<Expressi
   /**
    * Determines whether the given {@code mnemonic} (e.g. "CppCompile") matches any of the patterns.
    */
-  public boolean matches(String mnemonic) {
+  boolean matches(String mnemonic) {
     return expressions().stream().anyMatch(expr -> expr.pattern().matcher(mnemonic).matches());
   }
 
+  /** Checks whether the {@code executionInfoList} matches the {@code mnemonic}. */
+  public static boolean matches(
+      List<ExecutionInfoModifier> executionInfoList, boolean isAdditive, String mnemonic) {
+    if (executionInfoList.isEmpty()) {
+      return false;
+    }
+
+    if (isAdditive) {
+      return executionInfoList.stream().anyMatch(eim -> eim.matches(mnemonic));
+    } else {
+      return executionInfoList.getLast().matches(mnemonic);
+    }
+  }
+
+  /** Applies {@code executionInfoList} to the given {@code executionInfo}. */
+  public static void apply(
+      List<ExecutionInfoModifier> executionInfoList,
+      boolean isAdditive,
+      String mnemonic,
+      Map<String, String> executionInfo) {
+    if (executionInfoList.isEmpty()) {
+      return;
+    }
+
+    if (isAdditive) {
+      executionInfoList.forEach(eim -> eim.apply(mnemonic, executionInfo));
+    } else {
+      executionInfoList.getLast().apply(mnemonic, executionInfo);
+    }
+  }
+
   /** Modifies the given map of {@code executionInfo} to add or remove the keys for this option. */
   void apply(String mnemonic, Map<String, String> executionInfo) {
     for (Expression expr : expressions()) {

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

@@ -42,7 +42,7 @@
 
 /** Module providing on-demand spawn logging. */
 public final class SpawnLogModule extends BlazeModule {
-  private static final String EXEC_LOG_FILENAME = "execution_log.binpb.zstd";
+  private static final String EXEC_LOG_FILENAME = "execution_log.binpb.zst";
 
   @Nullable private SpawnLogContext spawnLogContext;
   @Nullable private Path outputPath;

src/main/java/com/google/devtools/build/lib/shell/Command.java

@@ -176,10 +176,10 @@ public Command(
    * </ul>
    *
    * @param commandLineElements elements of raw command line to execute
-   * @param environmentVariables environment variables to replace JVM's environment variables; may
-   *     be null
-   * @param workingDirectory working directory for execution; if null, the VM's current working
-   *     directory is used
+   * @param environmentVariables environment variables for the child process, or null to inherit
+   *     them from the parent
+   * @param workingDirectory working directory for the child process, or null to inherit it from the
+   *     parent
    * @param timeout timeout; a value less than or equal to 0 is treated as no timeout
    * @throws IllegalArgumentException if commandLine is null or empty
    */

src/main/java/com/google/devtools/build/lib/shell/SubprocessBuilder.java

@@ -125,7 +125,7 @@ public ImmutableMap<String, String> getEnv() {
 
   /**
    * Sets the environment passed to the child process. If null, inherit the environment of the
-   * server.
+   * parent. The default is to inherit.
    */
   @CanIgnoreReturnValue
   public SubprocessBuilder setEnv(@Nullable Map<String, String> env) {

src/main/java/com/google/devtools/build/lib/shell/WindowsSubprocessFactory.java

@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.shell;
 
+import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.shell.SubprocessBuilder.StreamAction;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.windows.WindowsProcesses;
@@ -120,37 +121,34 @@ private static String getRedirectPath(StreamAction action, File file) {
   }
 
   /** Converts an environment map to the format expected in lpEnvironment by CreateProcess(). */
-  private static byte[] convertEnvToNative(Map<String, String> envMap) throws IOException {
-    Map<String, String> realEnv = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
-    Map<String, String> systemEnv = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
+  private static byte[] convertEnvToNative(Map<String, String> envMap) {
+    Map<String, String> fullEnv = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
+
     if (envMap != null) {
-      realEnv.putAll(envMap);
-    }
-    // It is fine to use System.getenv to get default SYSTEMROOT and SYSTEMDRIVE, because they are
-    // very special system environment variables and Bazel's client and server are running on the
-    // same machine, so it should be the same in client environment.
-    systemEnv.putAll(System.getenv());
-    // Some versions of MSVCRT.DLL and tools require SYSTEMROOT and SYSTEMDRIVE to be set. They are
-    // very common environment variables on Windows, so we add these environment variables
-    // regardless of whether the caller requested it or not.
-    String[] systemEnvironmentVars = {"SYSTEMROOT", "SYSTEMDRIVE"};
-    for (String env : systemEnvironmentVars) {
-      if (realEnv.getOrDefault(env, null) == null) {
-        String value = systemEnv.getOrDefault(env, null);
-        if (value != null) {
-          realEnv.put(env, value);
+      fullEnv.putAll(envMap);
+      // Some versions of MSVCRT.DLL and tools require SYSTEMROOT and SYSTEMDRIVE to be set. They
+      // are very common environment variables on Windows, so we add these environment variables
+      // regardless of whether the caller requested it or not.
+      for (String env : ImmutableList.of("SYSTEMROOT", "SYSTEMDRIVE")) {
+        if (fullEnv.getOrDefault(env, null) == null) {
+          String value = System.getenv(env);
+          if (value != null) {
+            fullEnv.put(env, value);
+          }
         }
       }
+    } else {
+      fullEnv.putAll(System.getenv());
     }
 
-    if (realEnv.isEmpty()) {
+    if (fullEnv.isEmpty()) {
       // Special case: CreateProcess() always expects the environment block to be terminated
       // with two zeros.
       return "\0".getBytes(StandardCharsets.UTF_16LE);
     }
 
     StringBuilder result = new StringBuilder();
-    for (Map.Entry<String, String> entry : realEnv.entrySet()) {
+    for (Map.Entry<String, String> entry : fullEnv.entrySet()) {
       if (entry.getKey().contains("=")) {
         // lpEnvironment requires no '=' in environment variable name, but on Windows,
         // System.getenv() returns environment variables like '=C:' or '=ExitCode', so it can't

src/main/java/com/google/devtools/build/lib/skyframe/AspectCompletor.java

@@ -36,6 +36,7 @@
 import com.google.devtools.build.lib.skyframe.CompletionFunction.Completor;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunction.Environment;
+import java.util.function.Supplier;
 import javax.annotation.Nullable;
 
 /** Manages completing builds for aspects. */
@@ -46,13 +47,15 @@ static SkyFunction aspectCompletionFunction(
       PathResolverFactory pathResolverFactory,
       SkyframeActionExecutor skyframeActionExecutor,
       MetadataConsumerForMetrics.FilesMetricConsumer topLevelArtifactsMetric,
-      BugReporter bugReporter) {
+      BugReporter bugReporter,
+      Supplier<Boolean> isSkymeld) {
     return new CompletionFunction<>(
         pathResolverFactory,
         new AspectCompletor(),
         skyframeActionExecutor,
         topLevelArtifactsMetric,
-        bugReporter);
+        bugReporter,
+        isSkymeld);
   }
 
   @Override