Remove BuildConfiguration.Fragment#setupActionEnvironment().

This is accomplished by moving it to ConfiguredRuleClassProvider. This also suggests a neat way to get rid of logic in ShellConfiguration.Loader() by moving the determination of the shell executable, there, too, but not in this change.

RELNOTES: None.
PiperOrigin-RevId: 192411609

5 files changed, 84 insertions, 150 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
index 8cd2b7bef3..cad348ba0b 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
@@ -27,6 +27,7 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedSet;
+import com.google.devtools.build.lib.actions.ActionEnvironment;
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment;
@@ -237,6 +238,8 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider {
         ImmutableList.<Class<?>>builder().addAll(SkylarkModules.MODULES);
     private ImmutableList.Builder<NativeProvider> nativeProviders = ImmutableList.builder();
     private Set<String> reservedActionMnemonics = new TreeSet<>();
+    private BuildConfiguration.ActionEnvironmentProvider actionEnvironmentProvider =
+        (BuildOptions options) -> ActionEnvironment.EMPTY;
     private ImmutableBiMap.Builder<String, Class<? extends TransitiveInfoProvider>>
         registeredSkylarkProviders = ImmutableBiMap.builder();
 
@@ -371,6 +374,12 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider {
       return this;
     }
 
+    public Builder setActionEnvironmentProvider(
+        BuildConfiguration.ActionEnvironmentProvider actionEnvironmentProvider) {
+      this.actionEnvironmentProvider = actionEnvironmentProvider;
+      return this;
+    }
+
     /**
      * Sets the C++ LIPO data transition, as defined in {@link
      * com.google.devtools.build.lib.rules.cpp.transitions.DisableLipoTransition}.
@@ -471,6 +480,7 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider {
           skylarkAccessibleTopLevels.build(),
           skylarkModules.build(),
           ImmutableSet.copyOf(reservedActionMnemonics),
+          actionEnvironmentProvider,
           nativeProviders.build());
     }
 
@@ -579,6 +589,8 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider {
 
   private final ImmutableSet<String> reservedActionMnemonics;
 
+  private final BuildConfiguration.ActionEnvironmentProvider actionEnvironmentProvider;
+
   private final ImmutableList<NativeProvider> nativeProviders;
 
   private final ImmutableMap<String, Class<?>> configurationFragmentMap;
@@ -601,6 +613,7 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider {
       ImmutableMap<String, Object> skylarkAccessibleJavaClasses,
       ImmutableList<Class<?>> skylarkModules,
       ImmutableSet<String> reservedActionMnemonics,
+      BuildConfiguration.ActionEnvironmentProvider actionEnvironmentProvider,
       ImmutableList<NativeProvider> nativeProviders) {
     this.preludeLabel = preludeLabel;
     this.runfilesPrefix = runfilesPrefix;
@@ -618,6 +631,7 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider {
     this.prerequisiteValidator = prerequisiteValidator;
     this.globals = createGlobals(skylarkAccessibleJavaClasses, skylarkModules);
     this.reservedActionMnemonics = reservedActionMnemonics;
+    this.actionEnvironmentProvider = actionEnvironmentProvider;
     this.nativeProviders = nativeProviders;
     this.configurationFragmentMap = createFragmentMap(configurationFragmentFactories);
   }
@@ -832,6 +846,10 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider {
     return reservedActionMnemonics;
   }
 
+  public BuildConfiguration.ActionEnvironmentProvider getActionEnvironmentProvider() {
+    return actionEnvironmentProvider;
+  }
+
   /**
    * Returns all registered {@link NativeProvider} instances, i.e. all built-in provider types that
    * are based on {@link Provider} rather than {@link TransitiveInfoProvider}.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ShellConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/ShellConfiguration.java
index 2e4769e506..df2e5f1d5b 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ShellConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ShellConfiguration.java
@@ -21,10 +21,6 @@ import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.ConfigurationEnvironment;
 import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory;
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
-import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext;
-import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
-import com.google.devtools.build.lib.skyframe.serialization.SerializationContext;
-import com.google.devtools.build.lib.skyframe.serialization.SerializationException;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.OptionsUtils.PathFragmentConverter;
@@ -32,45 +28,11 @@ import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.common.options.Option;
 import com.google.devtools.common.options.OptionDocumentationCategory;
 import com.google.devtools.common.options.OptionEffectTag;
-import com.google.protobuf.CodedInputStream;
-import com.google.protobuf.CodedOutputStream;
-import java.io.IOException;
-import java.util.Map;
 import javax.annotation.Nullable;
 
 /** A configuration fragment that tells where the shell is. */
+@AutoCodec
 public class ShellConfiguration extends BuildConfiguration.Fragment {
-
-  /**
-   * A codec for {@link ShellConfiguration}.
-   *
-   * <p>It does not handle the Bazel version, but that's fine, because we don't want to serialize
-   * anything in Bazel.
-   *
-   * <p>We cannot use {@code AutoCodec} because the field {@link #actionEnvironment} is a lambda.
-   * That does not necessarily need to be the case, but it's there in support for
-   * {@link BuildConfiguration.Fragment#setupActionEnvironment()}, which is slated to be removed.
-   */
-  public static final class Codec implements ObjectCodec<ShellConfiguration> {
-    @Override
-    public Class<? extends ShellConfiguration> getEncodedClass() {
-      return ShellConfiguration.class;
-    }
-
-    @Override
-    public void serialize(SerializationContext context, ShellConfiguration obj,
-        CodedOutputStream codedOut) throws SerializationException, IOException {
-      context.serialize(obj.shellExecutable, codedOut);
-    }
-
-    @Override
-    public ShellConfiguration deserialize(DeserializationContext context, CodedInputStream codedIn)
-        throws SerializationException, IOException {
-      PathFragment shellExecutable = context.deserialize(codedIn);
-      return new ShellConfiguration(shellExecutable, NO_ACTION_ENV.fromOptions(null));
-    }
-  }
-
   private static final ImmutableMap<OS, PathFragment> OS_SPECIFIC_SHELL =
       ImmutableMap.<OS, PathFragment>builder()
           .put(OS.WINDOWS, PathFragment.create("c:/tools/msys64/usr/bin/bash.exe"))
@@ -78,23 +40,15 @@ public class ShellConfiguration extends BuildConfiguration.Fragment {
           .build();
 
   private final PathFragment shellExecutable;
-  private final ShellActionEnvironment actionEnvironment;
 
-  public ShellConfiguration(PathFragment shellExecutable,
-      ShellActionEnvironment actionEnvironment) {
+  public ShellConfiguration(PathFragment shellExecutable) {
     this.shellExecutable = shellExecutable;
-    this.actionEnvironment = actionEnvironment;
   }
 
   public PathFragment getShellExecutable() {
     return shellExecutable;
   }
 
-  @Override
-  public void setupActionEnvironment(Map<String, String> builder) {
-    actionEnvironment.setupActionEnvironment(this, builder);
-  }
-
   /** An option that tells Bazel where the shell is. */
   @AutoCodec(strategy = AutoCodec.Strategy.PUBLIC_FIELDS)
   public static class Options extends FragmentOptions {
@@ -123,33 +77,6 @@ public class ShellConfiguration extends BuildConfiguration.Fragment {
     }
   }
 
-  /**
-   * Encapsulates the contributions of {@link ShellConfiguration} to the action environment.
-   *
-   * <p>This is done this way because we need the shell environment to be different between Bazel
-   * and Blaze, but configuration fragments are handed out based on their classes, thus,
-   * doing this with inheritance would be difficult. The good old "has-a instead of this-a" pattern.
-   */
-  public interface ShellActionEnvironment {
-    void setupActionEnvironment(ShellConfiguration configuration, Map<String, String> builder);
-  }
-
-  /**
-   * A factory for shell action environments.
-   *
-   * <p>This is necessary because the Bazel shell action environment depends on whether we use
-   * strict action environments or not, but we cannot simply hardcode the dependency on that bit
-   * here because it doesn't exist in Blaze. Thus, during configuration creation time, we call this
-   * factory which returns an object, which, when called, updates the actual action environment.
-   */
-  public interface ShellActionEnvironmentFactory {
-    ShellActionEnvironment fromOptions(BuildOptions options);
-  }
-
-  /** A {@link ShellConfiguration} that contributes nothing to the action environment. */
-  public static final ShellActionEnvironmentFactory NO_ACTION_ENV =
-      (BuildOptions options) -> (ShellConfiguration config, Map<String, String> builder) -> {};
-
   /** the part of {@link ShellConfiguration} that determines where the shell is. */
   public interface ShellExecutableProvider {
     PathFragment getShellExecutable(BuildOptions options);
@@ -163,23 +90,18 @@ public class ShellConfiguration extends BuildConfiguration.Fragment {
   /** The loader for {@link ShellConfiguration}. */
   public static class Loader implements ConfigurationFragmentFactory {
     private final ShellExecutableProvider shellExecutableProvider;
-    private final ShellActionEnvironmentFactory actionEnvironmentFactory;
     private final ImmutableSet<Class<? extends FragmentOptions>> requiredOptions;
 
     public Loader(ShellExecutableProvider shellExecutableProvider,
-        ShellActionEnvironmentFactory actionEnvironmentFactory,
         Class<? extends FragmentOptions>... requiredOptions) {
       this.shellExecutableProvider = shellExecutableProvider;
-      this.actionEnvironmentFactory = actionEnvironmentFactory;
       this.requiredOptions = ImmutableSet.copyOf(requiredOptions);
     }
 
     @Nullable
     @Override
     public Fragment create(ConfigurationEnvironment env, BuildOptions buildOptions) {
-        return new ShellConfiguration(
-            shellExecutableProvider.getShellExecutable(buildOptions),
-            actionEnvironmentFactory.fromOptions(buildOptions));
+        return new ShellConfiguration(shellExecutableProvider.getShellExecutable(buildOptions));
     }
 
     public static PathFragment determineShellExecutable(
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index b2b77314b3..2a0faf087b 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -123,6 +123,11 @@ public class BuildConfiguration {
   private static final Interner<ImmutableSortedMap<Class<? extends Fragment>, Fragment>>
       fragmentsInterner = BlazeInterners.newWeakInterner();
 
+  /** Compute the default shell environment for actions from the command line options. */
+  public interface ActionEnvironmentProvider {
+    ActionEnvironment getActionEnvironment(BuildOptions options);
+  }
+
   /**
    * An interface for language-specific configurations.
    *
@@ -157,14 +162,6 @@ public class BuildConfiguration {
     }
 
     /**
-     * Add items to the action environment.
-     *
-     * @param builder the map to add environment variables to
-     */
-    public void setupActionEnvironment(Map<String, String> builder) {
-    }
-
-    /**
      * Returns { 'option name': 'alternative default' } entries for options where the
      * "real default" should be something besides the default specified in the {@link Option}
      * declaration.
@@ -1219,27 +1216,6 @@ public class BuildConfiguration {
   }
 
   /**
-   * Compute the shell environment, which, at configuration level, is a pair consisting of the
-   * statically set environment variables with their values and the set of environment variables to
-   * be inherited from the client environment.
-   */
-  private ActionEnvironment setupActionEnvironment() {
-    // We make a copy first to remove duplicate entries; last one wins.
-    Map<String, String> actionEnv = new HashMap<>();
-    // TODO(ulfjack): Remove all env variables from configuration fragments.
-    for (Fragment fragment : fragments.values()) {
-      fragment.setupActionEnvironment(actionEnv);
-    }
-    // Shell environment variables specified via options take precedence over the
-    // ones inherited from the fragments. In the long run, these fragments will
-    // be replaced by appropriate default rc files anyway.
-    for (Map.Entry<String, String> entry : options.actionEnvironment) {
-      actionEnv.put(entry.getKey(), entry.getValue());
-    }
-    return ActionEnvironment.split(actionEnv);
-  }
-
-  /**
    * Compute the test environment, which, at configuration level, is a pair consisting of the
    * statically set environment variables with their values and the set of environment variables to
    * be inherited from the client environment.
@@ -1265,6 +1241,7 @@ public class BuildConfiguration {
       BuildOptions buildOptions,
       BuildOptions.OptionsDiffForReconstruction buildOptionsDiff,
       ImmutableSet<String> reservedActionMnemonics,
+      ActionEnvironment actionEnvironment,
       String repositoryName) {
     this.directories = directories;
     this.fragments = makeFragmentsMap(fragmentsMap);
@@ -1314,7 +1291,7 @@ public class BuildConfiguration {
         OutputDirectory.MIDDLEMAN.getRoot(
             RepositoryName.MAIN, outputDirName, directories, mainRepositoryName);
 
-    this.actionEnv = setupActionEnvironment();
+    this.actionEnv = actionEnvironment;
 
     this.testEnv = setupTestEnvironment();
 
@@ -1367,6 +1344,7 @@ public class BuildConfiguration {
             options,
             BuildOptions.diffForReconstruction(defaultBuildOptions, options),
             reservedActionMnemonics,
+            actionEnv,
             mainRepositoryName.strippedName());
     return newConfig;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java
index 3d169694b7..389ac44ff1 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java
@@ -19,13 +19,14 @@ 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.ImmutableSet;
+import com.google.devtools.build.lib.actions.ActionEnvironment;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider.Builder;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider.RuleSet;
 import com.google.devtools.build.lib.analysis.ShellConfiguration;
-import com.google.devtools.build.lib.analysis.ShellConfiguration.ShellActionEnvironmentFactory;
 import com.google.devtools.build.lib.analysis.ShellConfiguration.ShellExecutableProvider;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
+import com.google.devtools.build.lib.analysis.config.BuildConfiguration.ActionEnvironmentProvider;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.FragmentOptions;
 import com.google.devtools.build.lib.bazel.rules.android.AndroidNdkRepositoryRule;
@@ -100,6 +101,7 @@ import com.google.devtools.common.options.OptionDocumentationCategory;
 import com.google.devtools.common.options.OptionEffectTag;
 import java.io.IOException;
 import java.util.Map;
+import java.util.TreeMap;
 import javax.annotation.Nullable;
 
 /** A rule class provider implementing the rules Bazel knows. */
@@ -130,35 +132,53 @@ public class BazelRuleClassProvider {
     }
   }
 
-  public static final ShellActionEnvironmentFactory SHELL_ACTION_ENV = (BuildOptions options) -> {
-    boolean strictActionEnv = options.get(
-        StrictActionEnvOptions.class).useStrictActionEnv;
-    return (ShellConfiguration configuration, Map<String, String> builder) -> {
-      OS os = OS.getCurrent();
-      // All entries in the builder that have a value of null inherit the value from the client
-      // environment, which is only known at execution time - we don't want to bake the client env
-      // into the configuration since any change to the configuration requires rerunning the full
-      // analysis phase.
-      if (!strictActionEnv) {
-        builder.put("LD_LIBRARY_PATH", null);
-      }
+  private static final PathFragment FALLBACK_SHELL = PathFragment.create("/bin/bash");
 
-      if (strictActionEnv) {
-        builder.put("PATH", pathOrDefault(os, null, configuration.getShellExecutable()));
-      } else if (os == OS.WINDOWS) {
-        // TODO(ulfjack): We want to add the MSYS root to the PATH, but that prevents us from
-        // inheriting PATH from the client environment. For now we use System.getenv even though
-        // that is incorrect. We should enable strict_action_env by default and then remove this
-        // code, but that change may break Windows users who are relying on the MSYS root being in
-        // the PATH.
-        builder.put("PATH", pathOrDefault(
-            os, System.getenv("PATH"), configuration.getShellExecutable()));
-      } else {
-        // The previous implementation used System.getenv (which uses the server's environment), and
-        // fell back to a hard-coded "/bin:/usr/bin" if PATH was not set.
-        builder.put("PATH", null);
-      }
-    };
+  public static final ShellExecutableProvider SHELL_EXECUTABLE = (BuildOptions options) ->
+      ShellConfiguration.Loader.determineShellExecutable(
+          OS.getCurrent(),
+          options.get(ShellConfiguration.Options.class),
+          FALLBACK_SHELL);
+
+  public static final ActionEnvironmentProvider SHELL_ACTION_ENV = (BuildOptions options) -> {
+    boolean strictActionEnv = options.get(StrictActionEnvOptions.class).useStrictActionEnv;
+    OS os = OS.getCurrent();
+    PathFragment shellExecutable = SHELL_EXECUTABLE.getShellExecutable(options);
+    TreeMap<String, String> env = new TreeMap<>();
+
+    // All entries in the builder that have a value of null inherit the value from the client
+    // environment, which is only known at execution time - we don't want to bake the client env
+    // into the configuration since any change to the configuration requires rerunning the full
+    // analysis phase.
+    if (!strictActionEnv) {
+      env.put("LD_LIBRARY_PATH", null);
+    }
+
+    if (strictActionEnv) {
+      env.put("PATH", pathOrDefault(os, null, shellExecutable));
+    } else if (os == OS.WINDOWS) {
+      // TODO(ulfjack): We want to add the MSYS root to the PATH, but that prevents us from
+      // inheriting PATH from the client environment. For now we use System.getenv even though
+      // that is incorrect. We should enable strict_action_env by default and then remove this
+      // code, but that change may break Windows users who are relying on the MSYS root being in
+      // the PATH.
+      env.put("PATH", pathOrDefault(
+          os, System.getenv("PATH"), shellExecutable));
+    } else {
+      // The previous implementation used System.getenv (which uses the server's environment), and
+      // fell back to a hard-coded "/bin:/usr/bin" if PATH was not set.
+      env.put("PATH", null);
+    }
+
+    // Shell environment variables specified via options take precedence over the
+    // ones inherited from the fragments. In the long run, these fragments will
+    // be replaced by appropriate default rc files anyway.
+    for (Map.Entry<String, String> entry :
+        options.get(BuildConfiguration.Options.class).actionEnvironment) {
+      env.put(entry.getKey(), entry.getValue());
+    }
+
+    return ActionEnvironment.split(env);
   };
 
   /** Used by the build encyclopedia generator. */
@@ -175,14 +195,6 @@ public class BazelRuleClassProvider {
     }
   }
 
-  private static final PathFragment FALLBACK_SHELL = PathFragment.create("/bin/bash");
-
-  public static final ShellExecutableProvider SHELL_EXECUTABLE = (BuildOptions options) ->
-    ShellConfiguration.Loader.determineShellExecutable(
-        OS.getCurrent(),
-        options.get(ShellConfiguration.Options.class),
-        FALLBACK_SHELL);
-
   public static final RuleSet BAZEL_SETUP =
       new RuleSet() {
         @Override
@@ -191,12 +203,12 @@ public class BazelRuleClassProvider {
               .setPrelude("//tools/build_rules:prelude_bazel")
               .setNativeLauncherLabel("//tools/launcher:launcher")
               .setRunfilesPrefix(Label.DEFAULT_REPOSITORY_DIRECTORY)
-              .setPrerequisiteValidator(new BazelPrerequisiteValidator());
+              .setPrerequisiteValidator(new BazelPrerequisiteValidator())
+              .setActionEnvironmentProvider(SHELL_ACTION_ENV);
 
           builder.addConfigurationOptions(ShellConfiguration.Options.class);
           builder.addConfigurationFragment(new ShellConfiguration.Loader(
               SHELL_EXECUTABLE,
-              SHELL_ACTION_ENV,
               ShellConfiguration.Options.class,
               StrictActionEnvOptions.class));
           builder.addUniversalConfigurationFragment(ShellConfiguration.class);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java
index 093cbb015a..9edebb8735 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java
@@ -18,6 +18,7 @@ import static com.google.devtools.build.lib.analysis.config.BuildConfiguration.F
 import com.google.common.collect.ClassToInstanceMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.MutableClassToInstanceMap;
+import com.google.devtools.build.lib.actions.ActionEnvironment;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
@@ -77,6 +78,8 @@ public class BuildConfigurationFunction implements SkyFunction {
     }
 
     BuildOptions options = defaultBuildOptions.applyDiff(key.getOptionsDiff());
+    ActionEnvironment actionEnvironment =
+      ruleClassProvider.getActionEnvironmentProvider().getActionEnvironment(options);
 
     BuildConfiguration config =
         new BuildConfiguration(
@@ -85,6 +88,7 @@ public class BuildConfigurationFunction implements SkyFunction {
             options,
             key.getOptionsDiff(),
             ruleClassProvider.getReservedActionMnemonics(),
+            actionEnvironment,
             workspaceNameValue.getName());
     return new BuildConfigurationValue(config);
   }