Add value equals and hashCode to BuildConfigurationValue

Note that they only incorporate the values that are not wholly dependent on other values and not server global. To minimize chance for error here, a factory method was added to BuildConfiguration that clearly takes a set of independent, varying values (which should be part of equality) and then a constrained set of server global values (which should not be part of equality).

This is so re-computation of BuildConfigurationValue by BuildConfigurationFunction that results in an equal BuildConfigurationValue avoids full Skyframe invalidation of all ConfiguredTargetFunction computations.

PiperOrigin-RevId: 415119618

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

@@ -1524,6 +1524,8 @@ java_library(
         ":config/core_options",
         ":config/fragment",
         ":config/fragment_class_set",
+        ":config/fragment_factory",
+        ":config/fragment_registry",
         ":config/invalid_configuration_exception",
         ":config/run_under",
         ":platform_options",

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

@@ -64,14 +64,14 @@
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.lang.reflect.Constructor;
-import java.lang.reflect.InvocationTargetException;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
 import java.util.TreeSet;
+import java.util.function.Function;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipInputStream;
 import javax.annotation.Nullable;
@@ -86,7 +86,8 @@
  * configuration options is guaranteed not to change over the life time of the Blaze server.
  */
 // This class has no subclasses except those created by the evil that is mockery.
-public /*final*/ class ConfiguredRuleClassProvider implements RuleClassProvider {
+public /*final*/ class ConfiguredRuleClassProvider
+    implements RuleClassProvider, BuildConfigurationValue.GlobalStateProvider {
 
   /**
    * A coherent set of options, fragments, aspects and rules; each of these may declare a dependency
@@ -158,7 +159,7 @@ public static final class Builder implements RuleDefinitionEnvironment {
     private final ImmutableList.Builder<SymlinkDefinition> symlinkDefinitions =
         ImmutableList.builder();
     private final Set<String> reservedActionMnemonics = new TreeSet<>();
-    private BuildConfigurationValue.ActionEnvironmentProvider actionEnvironmentProvider =
+    private Function<BuildOptions, ActionEnvironment> actionEnvironmentProvider =
         (BuildOptions options) -> ActionEnvironment.EMPTY;
     private ConstraintSemantics<RuleContext> constraintSemantics =
         new RuleContextConstraintSemantics();
@@ -327,7 +328,7 @@ public Builder addReservedActionMnemonic(String mnemonic) {
     }
 
     public Builder setActionEnvironmentProvider(
-        BuildConfigurationValue.ActionEnvironmentProvider actionEnvironmentProvider) {
+        Function<BuildOptions, ActionEnvironment> actionEnvironmentProvider) {
       this.actionEnvironmentProvider = actionEnvironmentProvider;
       return this;
     }
@@ -422,10 +423,7 @@ private static RuleConfiguredTargetFactory createFactory(
       try {
         Constructor<? extends RuleConfiguredTargetFactory> ctor = factoryClass.getConstructor();
         return ctor.newInstance();
-      } catch (NoSuchMethodException
-          | IllegalAccessException
-          | InstantiationException
-          | InvocationTargetException e) {
+      } catch (ReflectiveOperationException e) {
         throw new IllegalStateException(e);
       }
     }
@@ -440,7 +438,8 @@ private void commitRuleDefinition(Class<? extends RuleDefinition> definitionClas
       RuleDefinition.Metadata metadata = instance.getMetadata();
       checkArgument(
           ruleClassMap.get(metadata.name()) == null,
-          "The rule " + metadata.name() + " was committed already, use another name");
+          "The rule %s was committed already, use another name",
+          metadata.name());
 
       List<Class<? extends RuleDefinition>> ancestors = metadata.ancestors();
 
@@ -649,7 +648,7 @@ public Builder setNetworkAllowlistForTests(Label allowlist) {
 
   private final ImmutableSet<String> reservedActionMnemonics;
 
-  private final BuildConfigurationValue.ActionEnvironmentProvider actionEnvironmentProvider;
+  private final Function<BuildOptions, ActionEnvironment> actionEnvironmentProvider;
 
   private final ImmutableMap<String, Class<?>> configurationFragmentMap;
 
@@ -682,7 +681,7 @@ private ConfiguredRuleClassProvider(
       ImmutableList<Bootstrap> starlarkBootstraps,
       ImmutableList<SymlinkDefinition> symlinkDefinitions,
       ImmutableSet<String> reservedActionMnemonics,
-      BuildConfigurationValue.ActionEnvironmentProvider actionEnvironmentProvider,
+      Function<BuildOptions, ActionEnvironment> actionEnvironmentProvider,
       ConstraintSemantics<RuleContext> constraintSemantics,
       ThirdPartyLicenseExistencePolicy thirdPartyLicenseExistencePolicy,
       @Nullable Label networkAllowlistForTests) {
@@ -761,6 +760,7 @@ public NativeAspectClass getNativeAspectClass(String key) {
     return nativeAspectClassMap.get(key);
   }
 
+  @Override
   public FragmentRegistry getFragmentRegistry() {
     return fragmentRegistry;
   }
@@ -910,11 +910,13 @@ public ThirdPartyLicenseExistencePolicy getThirdPartyLicenseExistencePolicy() {
   }
 
   /** Returns a reserved set of action mnemonics. These cannot be used from a Starlark action. */
+  @Override
   public ImmutableSet<String> getReservedActionMnemonics() {
     return reservedActionMnemonics;
   }
 
-  public BuildConfigurationValue.ActionEnvironmentProvider getActionEnvironmentProvider() {
-    return actionEnvironmentProvider;
+  @Override
+  public ActionEnvironment getActionEnvironment(BuildOptions buildOptions) {
+    return actionEnvironmentProvider.apply(buildOptions);
   }
 }

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

@@ -47,6 +47,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.TreeMap;
 import java.util.function.Supplier;
@@ -86,9 +87,14 @@ public class BuildConfigurationValue implements BuildConfigurationApi, SkyValue
   private static final Interner<ImmutableSortedMap<String, String>> executionInfoInterner =
       BlazeInterners.newWeakInterner();
 
-  /** Compute the default shell environment for actions from the command line options. */
-  public interface ActionEnvironmentProvider {
+  /** Global state necessary to build a BuildConfiguration. */
+  public interface GlobalStateProvider {
+    /** Computes the default shell environment for actions from the command line options. */
     ActionEnvironment getActionEnvironment(BuildOptions options);
+
+    FragmentRegistry getFragmentRegistry();
+
+    ImmutableSet<String> getReservedActionMnemonics();
   }
 
   private final OutputDirectories outputDirectories;
@@ -153,14 +159,55 @@ private ActionEnvironment setupTestEnvironment() {
     return ActionEnvironment.split(testEnv);
   }
 
-  public BuildConfigurationValue(
+  // Only BuildConfigurationFunction (and tests for mocking purposes) should instantiate this.
+  public static BuildConfigurationValue create(
+      BuildOptions buildOptions,
+      RepositoryName mainRepositoryName,
+      boolean siblingRepositoryLayout,
+      // Arguments below this are server-global.
       BlazeDirectories directories,
-      ImmutableMap<Class<? extends Fragment>, Fragment> fragments,
+      GlobalStateProvider globalProvider,
+      FragmentFactory fragmentFactory)
+      throws InvalidConfigurationException {
+
+    FragmentClassSet fragmentClasses = globalProvider.getFragmentRegistry().getAllFragments();
+    ImmutableSortedMap<Class<? extends Fragment>, Fragment> fragments =
+        getConfigurationFragments(buildOptions, fragmentClasses, fragmentFactory);
+
+    return new BuildConfigurationValue(
+        buildOptions,
+        mainRepositoryName,
+        siblingRepositoryLayout,
+        directories,
+        fragments,
+        globalProvider.getReservedActionMnemonics(),
+        globalProvider.getActionEnvironment(buildOptions));
+  }
+
+  private static ImmutableSortedMap<Class<? extends Fragment>, Fragment> getConfigurationFragments(
+      BuildOptions buildOptions, FragmentClassSet fragmentClasses, FragmentFactory fragmentFactory)
+      throws InvalidConfigurationException {
+    ImmutableSortedMap.Builder<Class<? extends Fragment>, Fragment> fragments =
+        ImmutableSortedMap.orderedBy(FragmentClassSet.LEXICAL_FRAGMENT_SORTER);
+    for (Class<? extends Fragment> fragmentClass : fragmentClasses) {
+      Fragment fragment = fragmentFactory.createFragment(buildOptions, fragmentClass);
+      if (fragment != null) {
+        fragments.put(fragmentClass, fragment);
+      }
+    }
+    return fragments.build();
+  }
+
+  // Package-visible for serialization purposes.
+  BuildConfigurationValue(
       BuildOptions buildOptions,
-      ImmutableSet<String> reservedActionMnemonics,
-      ActionEnvironment actionEnvironment,
       RepositoryName mainRepositoryName,
-      boolean siblingRepositoryLayout)
+      boolean siblingRepositoryLayout,
+      // Arguments below this are either server-global and constant or completely dependent values.
+      BlazeDirectories directories,
+      ImmutableMap<Class<? extends Fragment>, Fragment> fragments,
+      ImmutableSet<String> reservedActionMnemonics,
+      ActionEnvironment actionEnvironment)
       throws InvalidMnemonicException {
     this.fragments =
         fragmentsInterner.intern(
@@ -219,6 +266,26 @@ public BuildConfigurationValue(
     this.commandLineLimits = new CommandLineLimits(options.minParamFileSize);
   }
 
+  @Override
+  public boolean equals(Object other) {
+    if (this == other) {
+      return true;
+    }
+    if (!(other instanceof BuildConfigurationValue)) {
+      return false;
+    }
+    // Only considering arguments that are non-dependent and non-server-global.
+    BuildConfigurationValue otherVal = (BuildConfigurationValue) other;
+    return this.buildOptions.equals(otherVal.buildOptions)
+        && this.mainRepositoryName.equals(otherVal.mainRepositoryName)
+        && this.siblingRepositoryLayout == otherVal.siblingRepositoryLayout;
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(buildOptions, mainRepositoryName, siblingRepositoryLayout);
+  }
+
   private ImmutableMap<String, Class<? extends Fragment>> buildIndexOfStarlarkVisibleFragments() {
     ImmutableMap.Builder<String, Class<? extends Fragment>> builder = ImmutableMap.builder();
 

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

@@ -17,8 +17,8 @@
 import com.github.benmanes.caffeine.cache.LoadingCache;
 import com.google.auto.value.AutoValue;
 import com.google.common.base.Throwables;
+import com.google.common.collect.ImmutableSet;
 import java.lang.reflect.InvocationTargetException;
-import java.util.Set;
 import java.util.concurrent.CompletionException;
 import javax.annotation.Nullable;
 
@@ -67,7 +67,8 @@ public Fragment createFragment(BuildOptions buildOptions, Class<? extends Fragme
   private static BuildOptions trimToRequiredOptions(
       BuildOptions original, Class<? extends Fragment> fragment) {
     BuildOptions.Builder trimmed = BuildOptions.builder();
-    Set<Class<? extends FragmentOptions>> requiredOptions = Fragment.requiredOptions(fragment);
+    ImmutableSet<Class<? extends FragmentOptions>> requiredOptions =
+        Fragment.requiredOptions(fragment);
     for (FragmentOptions options : original.getNativeOptions()) {
       // CoreOptions is implicitly required by all fragments.
       if (options instanceof CoreOptions || requiredOptions.contains(options.getClass())) {

src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java

@@ -26,7 +26,6 @@
 import com.google.devtools.build.lib.analysis.PlatformConfiguration;
 import com.google.devtools.build.lib.analysis.ShellConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
-import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.ActionEnvironmentProvider;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.CoreOptions;
 import com.google.devtools.build.lib.analysis.config.Fragment;
@@ -183,7 +182,7 @@ public static class StrictActionEnvConfiguration extends Fragment {
     public StrictActionEnvConfiguration(BuildOptions buildOptions) {}
   }
 
-  public static final ActionEnvironmentProvider SHELL_ACTION_ENV =
+  public static final Function<BuildOptions, ActionEnvironment> SHELL_ACTION_ENV =
       (BuildOptions options) -> {
         boolean strictActionEnv = options.get(StrictActionEnvOptions.class).useStrictActionEnv;
         OS os = OS.getCurrent();

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

@@ -13,16 +13,11 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
-import com.google.common.collect.ImmutableSortedMap;
-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.BuildConfigurationValue;
-import com.google.devtools.build.lib.analysis.config.Fragment;
-import com.google.devtools.build.lib.analysis.config.FragmentClassSet;
 import com.google.devtools.build.lib.analysis.config.FragmentFactory;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
-import com.google.devtools.build.lib.analysis.config.OutputDirectories.InvalidMnemonicException;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
@@ -53,52 +48,27 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     if (workspaceNameValue == null) {
       return null;
     }
-    FragmentClassSet fragmentClasses = ruleClassProvider.getFragmentRegistry().getAllFragments();
-
-    BuildConfigurationKey key = (BuildConfigurationKey) skyKey.argument();
-    ImmutableSortedMap<Class<? extends Fragment>, Fragment> fragments;
-    try {
-      fragments = getConfigurationFragments(key, fragmentClasses);
-    } catch (InvalidConfigurationException e) {
-      throw new BuildConfigurationFunctionException(e);
-    }
 
     StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
     if (starlarkSemantics == null) {
       return null;
     }
 
-    ActionEnvironment actionEnvironment =
-        ruleClassProvider.getActionEnvironmentProvider().getActionEnvironment(key.getOptions());
-
+    BuildConfigurationKey key = (BuildConfigurationKey) skyKey.argument();
     try {
-      return new BuildConfigurationValue(
-          directories,
-          fragments,
+      return BuildConfigurationValue.create(
           key.getOptions(),
-          ruleClassProvider.getReservedActionMnemonics(),
-          actionEnvironment,
           RepositoryName.createFromValidStrippedName(workspaceNameValue.getName()),
-          starlarkSemantics.getBool(BuildLanguageOptions.EXPERIMENTAL_SIBLING_REPOSITORY_LAYOUT));
-    } catch (InvalidMnemonicException e) {
+          starlarkSemantics.getBool(BuildLanguageOptions.EXPERIMENTAL_SIBLING_REPOSITORY_LAYOUT),
+          // Arguments below this are server-global.
+          directories,
+          ruleClassProvider,
+          fragmentFactory);
+    } catch (InvalidConfigurationException e) {
       throw new BuildConfigurationFunctionException(e);
     }
   }
 
-  private ImmutableSortedMap<Class<? extends Fragment>, Fragment> getConfigurationFragments(
-      BuildConfigurationKey key, FragmentClassSet fragmentClasses)
-      throws InvalidConfigurationException {
-    ImmutableSortedMap.Builder<Class<? extends Fragment>, Fragment> fragments =
-        ImmutableSortedMap.orderedBy(FragmentClassSet.LEXICAL_FRAGMENT_SORTER);
-    for (Class<? extends Fragment> fragmentClass : fragmentClasses) {
-      Fragment fragment = fragmentFactory.createFragment(key.getOptions(), fragmentClass);
-      if (fragment != null) {
-        fragments.put(fragmentClass, fragment);
-      }
-    }
-    return fragments.build();
-  }
-
   private static final class BuildConfigurationFunctionException extends SkyFunctionException {
     BuildConfigurationFunctionException(InvalidConfigurationException e) {
       super(e, Transience.PERSISTENT);

src/test/java/com/google/devtools/build/lib/BUILD

@@ -134,8 +134,9 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment",
-        "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_class_set",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_factory",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_registry",
         "//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
         "//src/main/java/com/google/devtools/build/lib/analysis:server_directories",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration",

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

@@ -20,6 +20,7 @@
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
+import com.google.common.testing.EqualsTester;
 import com.google.devtools.build.lib.analysis.config.BuildOptions.MapBackedChecksumCache;
 import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsChecksumCache;
 import com.google.devtools.build.lib.analysis.config.OutputDirectories.InvalidMnemonicException;
@@ -382,6 +383,25 @@ public void testPlatformInOutputDir() throws Exception {
         .matches(".*/[^/]+-out/alpha-fastbuild");
   }
 
+  @Test
+  public void testConfigurationEquality() throws Exception {
+    // Note that, in practice, test_arg should not be used as a no-op argument; however,
+    // these configurations are never trimmed nor even used to build targets so not an issue.
+    new EqualsTester()
+        .addEqualityGroup(
+            createRaw(parseBuildOptions("--test_arg=1a"), "@testrepo", false),
+            createRaw(parseBuildOptions("--test_arg=1a"), "@testrepo", false))
+        // Different BuildOptions means non-equal
+        .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=1b"), "@testrepo", false))
+        // Different --experimental_sibling_repository_layout means non-equal
+        .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=2"), "@testrepo", true))
+        .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=2"), "@testrepo", false))
+        // Different repositoryName means non-equal
+        .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "@testrepo1", false))
+        .addEqualityGroup(createRaw(parseBuildOptions("--test_arg=3"), "@testrepo2", false))
+        .testEquals();
+  }
+
   /**
    * Partial verification of deserialized BuildConfigurationValue.
    *

src/test/java/com/google/devtools/build/lib/analysis/util/BUILD

@@ -49,6 +49,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:config/config_matching_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/core_option_converters",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment",
+        "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_factory",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/host_transition",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/invalid_configuration_exception",