Remove code related to trimming the host configuration.

The `FragmentClassSet` passed to `BuildConfiguration#clone` is always the full set of fragments, so no trimming is ever performed.

PiperOrigin-RevId: 400847795

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

@@ -14,8 +14,6 @@
 
 package com.google.devtools.build.lib.analysis.config;
 
-import static com.google.common.collect.ImmutableSortedMap.toImmutableSortedMap;
-
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Suppliers;
 import com.google.common.collect.ArrayListMultimap;
@@ -51,7 +49,6 @@
 import java.util.Objects;
 import java.util.Set;
 import java.util.TreeMap;
-import java.util.function.Function;
 import java.util.function.Supplier;
 import net.starlark.java.annot.StarlarkAnnotations;
 import net.starlark.java.annot.StarlarkBuiltin;
@@ -124,28 +121,6 @@ public interface ActionEnvironmentProvider {
 
   private final boolean siblingRepositoryLayout;
 
-  /**
-   * Returns true if this configuration is semantically equal to the other, with the possible
-   * exception that the other has fewer fragments.
-   *
-   * <p>This is useful for trimming: as the same configuration gets "trimmed" while going down a
-   * dependency chain, it's still the same configuration but loses some of its fragments. So we need
-   * a more nuanced concept of "equality" than simple reference equality.
-   */
-  // TODO(b/121048710): make this reflect starlark options
-  public boolean equalsOrIsSupersetOf(BuildConfiguration other) {
-    return this.equals(other)
-        || (other != null
-            // TODO(gregce): add back in output root checking. This requires a better approach to
-            // configuration-safe output paths. If the parent config has a fragment the child config
-            // doesn't, it may inject $(FOO) into the output roots. So the child bindir might be
-            // "bazel-out/arm-linux-fastbuild/bin" while the parent bindir is
-            // "bazel-out/android-arm-linux-fastbuild/bin". That's pretty awkward to check here.
-            //      && outputRoots.equals(other.outputRoots)
-            && fragments.values().containsAll(other.fragments.values())
-            && buildOptions.getNativeOptions().containsAll(other.buildOptions.getNativeOptions()));
-  }
-
   /**
    * Returns {@code true} if this configuration is semantically equal to the other, including
    * checking that both have the same sets of fragments and options.
@@ -259,38 +234,6 @@ public BuildConfiguration(
     this.commandLineLimits = new CommandLineLimits(options.minParamFileSize);
   }
 
-  /**
-   * Returns a copy of this configuration only including the given fragments (which the current
-   * configuration is assumed to have).
-   */
-  public BuildConfiguration clone(
-      FragmentClassSet fragmentClasses, RuleClassProvider ruleClassProvider) {
-    ImmutableSortedMap<Class<? extends Fragment>, Fragment> fragmentsMap =
-        fragments.values().stream()
-            .filter(fragment -> fragmentClasses.contains(fragment.getClass()))
-            .collect(
-                toImmutableSortedMap(
-                    FragmentClassSet.LEXICAL_FRAGMENT_SORTER,
-                    Fragment::getClass,
-                    Function.identity()));
-    BuildOptions options =
-        buildOptions.trim(getOptionsClasses(fragmentsMap.keySet(), ruleClassProvider));
-    try {
-      return new BuildConfiguration(
-          getDirectories(),
-          fragmentsMap,
-          fragmentClasses,
-          options,
-          reservedActionMnemonics,
-          actionEnv,
-          mainRepositoryName,
-          siblingRepositoryLayout);
-    } catch (InvalidMnemonicException e) {
-      throw new IllegalStateException(
-          "Invalid mnemonic unexpected when cloning: " + this + ", " + fragmentClasses, e);
-    }
-  }
-
   /** Returns the config fragment options classes used by the given fragment types. */
   public static Set<Class<? extends FragmentOptions>> getOptionsClasses(
       Iterable<Class<? extends Fragment>> fragmentClasses, RuleClassProvider ruleClassProvider) {

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

@@ -158,8 +158,7 @@ public static StarlarkDefinedAspect loadStarlarkDefinedAspect(
       Object starlarkValue = bzlLoadValue.getModule().getGlobal(starlarkValueName);
       if (starlarkValue == null) {
         throw new ConversionException(
-            String.format(
-                "%s is not exported from %s", starlarkValueName, extensionLabel.toString()));
+            String.format("%s is not exported from %s", starlarkValueName, extensionLabel));
       }
       if (!(starlarkValue instanceof StarlarkDefinedAspect)) {
         throw new ConversionException(
@@ -296,7 +295,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     if (AliasProvider.isAlias(associatedTarget)) {
       return createAliasAspect(
           env,
-          view.getHostConfiguration(aspectConfiguration),
+          view.getHostConfiguration(),
           new TargetAndConfiguration(target, configuration),
           aspect,
           key,
@@ -427,7 +426,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
                         .build(),
                 shouldUseToolchainTransition(configuration, aspect.getDefinition()),
                 ruleClassProvider,
-                view.getHostConfiguration(originalTargetAndAspectConfiguration.getConfiguration()),
+                view.getHostConfiguration(),
                 transitivePackagesForPackageRootResolution,
                 transitiveRootCauses);
       } catch (ConfiguredValueCreationException e) {
@@ -729,7 +728,7 @@ private AspectValue createAliasAspect(
         transitivePackagesForPackageRootResolution);
   }
 
-  private AspectValue createAliasAspect(
+  private static AspectValue createAliasAspect(
       Environment env,
       Target originalTarget,
       AspectKey originalKey,
@@ -875,7 +874,7 @@ private AspectValue createAspect(
                     configConditions,
                     toolchainContext,
                     aspectConfiguration,
-                    view.getHostConfiguration(aspectConfiguration),
+                    view.getHostConfiguration(),
                     key);
       } catch (MissingDepException e) {
         Preconditions.checkState(env.valuesMissing());

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

@@ -328,7 +328,7 @@ public SkyValue compute(SkyKey key, Environment env)
                   : unloadedToolchainContexts.asToolchainContexts(),
               DependencyResolver.shouldUseToolchainTransition(configuration, ctgValue.getTarget()),
               ruleClassProvider,
-              view.getHostConfiguration(configuration),
+              view.getHostConfiguration(),
               transitivePackagesForPackageRootResolution,
               transitiveRootCauses);
       if (!transitiveRootCauses.isEmpty()) {

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

@@ -59,7 +59,6 @@
 import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsDiff;
 import com.google.devtools.build.lib.analysis.config.ConfigConditions;
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
-import com.google.devtools.build.lib.analysis.config.FragmentClassSet;
 import com.google.devtools.build.lib.bugreport.BugReport;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.BuildEventId.ConfigurationId;
@@ -146,10 +145,6 @@ public final class SkyframeBuildView {
 
   // The host configuration containing all fragments used by this build's transitive closure.
   private BuildConfiguration topLevelHostConfiguration;
-  // Fragment-limited versions of the host configuration. It's faster to create/cache these here
-  // than to store them in Skyframe.
-  private final Map<BuildConfiguration, BuildConfiguration> hostConfigurationCache =
-      Maps.newConcurrentMap();
 
   private BuildConfigurationCollection configurations;
 
@@ -351,13 +346,9 @@ public BuildConfigurationCollection getBuildConfigurationCollection() {
   /**
    * Sets the host configuration consisting of all fragments that will be used by the top level
    * targets' transitive closures.
-   *
-   * <p>This is used to power {@link #getHostConfiguration} during analysis, which computes
-   * fragment-trimmed host configurations from the top-level one.
    */
   private void setTopLevelHostConfiguration(BuildConfiguration topLevelHostConfiguration) {
     if (!topLevelHostConfiguration.equals(this.topLevelHostConfiguration)) {
-      hostConfigurationCache.clear();
       this.topLevelHostConfiguration = topLevelHostConfiguration;
     }
   }
@@ -1117,7 +1108,7 @@ ConfiguredTarget createConfiguredTarget(
         artifactFactory,
         target,
         configuration,
-        getHostConfiguration(configuration),
+        topLevelHostConfiguration,
         configuredTargetKey,
         prerequisiteMap,
         configConditions,
@@ -1126,55 +1117,13 @@ ConfiguredTarget createConfiguredTarget(
   }
 
   /**
-   * Returns the host configuration trimmed to the same fragments as the input configuration. If the
-   * input is null, returns the top-level host configuration.
+   * Returns the top-level host configuration.
    *
    * <p>This may only be called after {@link #setTopLevelHostConfiguration} has set the correct host
    * configuration at the top-level.
    */
-  public BuildConfiguration getHostConfiguration(BuildConfiguration config) {
-    if (config == null) {
-      return topLevelHostConfiguration;
-    }
-    // Currently, a single build doesn't use many different BuildConfiguration instances. Thus,
-    // having a cache per BuildConfiguration is efficient. It might lead to instances of otherwise
-    // identical configurations if multiple of these configs use the same fragment classes. However,
-    // these are cheap especially if there is only a small number of configs. Revisit and turn into
-    // a cache per FragmentClassSet if configuration trimming results in a much higher number of
-    // configuration instances.
-    BuildConfiguration hostConfig = hostConfigurationCache.get(config);
-    if (hostConfig != null) {
-      return hostConfig;
-    }
-    // TODO(bazel-team): have the fragment classes be those required by the consuming target's
-    // transitive closure. This isn't the same as the input configuration's fragment classes -
-    // the latter may be a proper subset of the former.
-    //
-    // ConfigurationFactory.getConfiguration provides the reason why: if a declared required
-    // fragment is evaluated and returns null, it never gets added to the configuration. So if we
-    // use the configuration's fragments as the source of truth, that excludes required fragments
-    // that never made it in.
-    //
-    // If we're just trimming an existing configuration, this is no big deal (if the original
-    // configuration doesn't need the fragment, the trimmed one doesn't either). But this method
-    // trims a host configuration to the same scope as a target configuration. Since their options
-    // are different, the host instance may actually be able to produce the fragment. So it's
-    // wrong and potentially dangerous to unilaterally exclude it.
-    FragmentClassSet fragmentClasses = ruleClassProvider.getConfigurationFragments();
-    // TODO(bazel-team): investigate getting the trimmed config from Skyframe instead of cloning.
-    // This is the only place we instantiate BuildConfigurations outside of Skyframe, This can
-    // produce surprising effects, such as requesting a configuration that's in the Skyframe cache
-    // but still produces a unique instance because we don't check that cache. It'd be nice to
-    // guarantee that *all* instantiations happen through Skyframe. That could, for example,
-    // guarantee that config1.equals(config2) implies config1 == config2, which is nice for
-    // verifying we don't accidentally create extra configurations. But unfortunately,
-    // hostConfigurationCache was specifically created because Skyframe is too slow for this use
-    // case. So further optimization is necessary to make that viable (proto_library in particular
-    // contributes to much of the difference).
-    BuildConfiguration trimmedConfig =
-        topLevelHostConfiguration.clone(fragmentClasses, ruleClassProvider);
-    hostConfigurationCache.put(config, trimmedConfig);
-    return trimmedConfig;
+  public BuildConfiguration getHostConfiguration() {
+    return topLevelHostConfiguration;
   }
 
   /**

src/test/java/com/google/devtools/build/lib/analysis/LateBoundSplitUtil.java

@@ -14,7 +14,6 @@
 
 package com.google.devtools.build.lib.analysis;
 
-
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.Fragment;

src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java

@@ -19,7 +19,6 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.analysis.config.BuildOptions.MapBackedChecksumCache;
 import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsChecksumCache;
@@ -202,20 +201,6 @@ public void testGetTransitiveOptionDetails() throws Exception {
     assertThat(create().getTransitiveOptionDetails().getOptionValue("test_filter")).isNull();
   }
 
-  @Test
-  public void testEqualsOrIsSupersetOf() throws Exception {
-    BuildConfiguration config = create();
-    BuildConfiguration trimmedConfig =
-        config.clone(
-            FragmentClassSet.of(ImmutableSet.of(CppConfiguration.class)),
-            analysisMock.createRuleClassProvider());
-    BuildConfiguration hostConfig = createHost();
-
-    assertThat(config.equalsOrIsSupersetOf(trimmedConfig)).isTrue();
-    assertThat(config.equalsOrIsSupersetOf(hostConfig)).isFalse();
-    assertThat(trimmedConfig.equalsOrIsSupersetOf(config)).isFalse();
-  }
-
   @Test
   public void testConfigFragmentsAreShareableAcrossConfigurations() throws Exception {
     // Note we can't use any fragments that load files (e.g. CROSSTOOL) because those get

src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java

@@ -44,7 +44,7 @@ public class GenRuleConfiguredTargetTest extends BuildViewTestCase {
   private static final Pattern SETUP_COMMAND_PATTERN =
       Pattern.compile(".*/genrule-setup.sh;\\s+(?<command>.*)");
 
-  private void assertCommandEquals(String expected, String command) {
+  private static void assertCommandEquals(String expected, String command) {
     // Ensure the command after the genrule setup is correct.
     Matcher m = SETUP_COMMAND_PATTERN.matcher(command);
     if (m.matches()) {
@@ -417,8 +417,7 @@ public void testToolsAreHostConfiguration() throws Exception {
           foundSrc = true;
           break;
         case "tool":
-          assertThat(getHostConfiguration().equalsOrIsSupersetOf(getConfiguration(prereq)))
-              .isTrue();
+          assertThat(getHostConfiguration()).isEqualTo(getConfiguration(prereq));
           foundTool = true;
           break;
         case GENRULE_SETUP_PATH:
@@ -535,7 +534,7 @@ private void assertStamped(String target) throws Exception {
     assertStamped(getConfiguredTarget(target));
   }
 
-  private void assertStamped(ConfiguredTarget target) throws Exception {
+  private void assertStamped(ConfiguredTarget target) {
     Artifact out = getFilesToBuild(target).toList().get(0);
     List<String> inputs = ActionsTestUtil.baseArtifactNames(getGeneratingAction(out).getInputs());
     assertThat(inputs).containsAtLeast("build-info.txt", "build-changelist.txt");
@@ -545,7 +544,7 @@ private void assertNotStamped(String target) throws Exception {
     assertNotStamped(getConfiguredTarget(target));
   }
 
-  private void assertNotStamped(ConfiguredTarget target) throws Exception {
+  private void assertNotStamped(ConfiguredTarget target) {
     Artifact out = getFilesToBuild(target).toList().get(0);
     List<String> inputs = ActionsTestUtil.baseArtifactNames(getGeneratingAction(out).getInputs());
     assertThat(inputs).doesNotContain("build-info.txt");

src/test/java/com/google/devtools/build/lib/skyframe/ConfigurationsForTargetsTest.java

@@ -261,7 +261,7 @@ public void targetDeps() throws Exception {
     BuildConfiguration topLevelConfiguration =
         getConfiguration(Iterables.getOnlyElement(update("//a:binary").getTargetsToBuild()));
     for (ConfiguredTarget dep : deps) {
-      assertThat(topLevelConfiguration.equalsOrIsSupersetOf(getConfiguration(dep))).isTrue();
+      assertThat(topLevelConfiguration).isEqualTo(getConfiguration(dep));
     }
   }