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

Author: lberki

src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java

@@ -27,6 +27,7 @@
 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 static class Builder implements RuleDefinitionEnvironment {
         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 Builder addReservedActionMnemonic(String mnemonic) {
       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 ConfiguredRuleClassProvider build() {
           skylarkAccessibleTopLevels.build(),
           skylarkModules.build(),
           ImmutableSet.copyOf(reservedActionMnemonics),
+          actionEnvironmentProvider,
           nativeProviders.build());
     }
 
@@ -579,6 +589,8 @@ public Label load(String from) {
 
   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 @@ private ConfiguredRuleClassProvider(
       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 @@ private ConfiguredRuleClassProvider(
     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 ImmutableSet<String> getReservedActionMnemonics() {
     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}.

src/main/java/com/google/devtools/build/lib/analysis/ShellConfiguration.java

@@ -21,80 +21,34 @@
 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;
 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"))
           .put(OS.FREEBSD, PathFragment.create("/usr/local/bin/bash"))
           .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 Options getHost() {
     }
   }
 
-  /**
-   * 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 static ShellExecutableProvider hardcodedShellExecutable(String shell) {
   /** 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(

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.
    *
@@ -156,14 +161,6 @@ public String getOutputDirectoryName() {
       return null;
     }
 
-    /**
-     * 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}
@@ -1218,27 +1215,6 @@ public boolean apply(@Nullable Fragment fragment) {
         });
   }
 
-  /**
-   * 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
@@ -1265,6 +1241,7 @@ public 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 BuildConfiguration(
         OutputDirectory.MIDDLEMAN.getRoot(
             RepositoryName.MAIN, outputDirName, directories, mainRepositoryName);
 
-    this.actionEnv = setupActionEnvironment();
+    this.actionEnv = actionEnvironment;
 
     this.testEnv = setupTestEnvironment();
 
@@ -1367,6 +1344,7 @@ public BuildConfiguration clone(
             options,
             BuildOptions.diffForReconstruction(defaultBuildOptions, options),
             reservedActionMnemonics,
+            actionEnv,
             mainRepositoryName.strippedName());
     return newConfig;
   }