Fix a bug in SkyframeExecutor.getConfigurations() for dynamic

configurations:

That calls getDynamicConfigOptions() to get the right BuildOptions
for a target, but that fails to trim the option's FragmentOptions.
That means, for example, a BuildConfiguration could get constructed
with just the CppConfiguration fragment but with a BuildOptions
that still has PythonOptions.

This isn't just sloppy. It breaks Bazel tests that use
SkyframeExecutor.getConfiguredTargetForTesting (which follows this
code path) to compare against the production code path (which
already properly trims deps). So two configured targets that should be
equal won't be because their BuildConfigurations won't match.

This is also a prerequisite change for trimming top-level configurations,
coming up soon.

TESTED: BuildViewTest#testNewActionsAreDifferentAndDontConflict (and other
tests) pass with this change + the still-pending top-level-trimming change.

--
MOS_MIGRATED_REVID=123676990

1 files changed, 21 insertions, 9 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 1b57421399..4590afe6e8 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -270,6 +270,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
 
   private final PathFragment blacklistedPackagePrefixesFile;
 
+  private final RuleClassProvider ruleClassProvider;
+
   private static final Logger LOG = Logger.getLogger(SkyframeExecutor.class.getName());
 
   protected SkyframeExecutor(
@@ -308,11 +310,12 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     this.blacklistedPackagePrefixesFile = blacklistedPackagePrefixesFile;
     this.binTools = binTools;
 
+    this.ruleClassProvider = pkgFactory.getRuleClassProvider();
     this.skyframeBuildView = new SkyframeBuildView(
         directories,
         this,
         binTools,
-        (ConfiguredRuleClassProvider) pkgFactory.getRuleClassProvider());
+        (ConfiguredRuleClassProvider) ruleClassProvider);
     this.artifactFactory.set(skyframeBuildView.getArtifactFactory());
     this.externalFilesHelper = new ExternalFilesHelper(
         pkgLocator, this.errorOnExternalFiles, directories);
@@ -1259,16 +1262,20 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       if (!depsToEvaluate.contains(key) || labelsWithErrors.contains(key.getLabel())) {
         continue;
       }
-      configSkyKeys.add(BuildConfigurationValue.key(fragmentsMap.get(key.getLabel()),
-          getDynamicConfigOptions(key, fromOptions)));
+      Set<Class<? extends BuildConfiguration.Fragment>> depFragments =
+          fragmentsMap.get(key.getLabel());
+      configSkyKeys.add(BuildConfigurationValue.key(depFragments,
+          getDynamicConfigOptions(key, fromOptions, depFragments)));
     }
     EvaluationResult<SkyValue> configsResult = evaluateSkyKeys(eventHandler, configSkyKeys);
     for (Dependency key : keys) {
       if (!depsToEvaluate.contains(key) || labelsWithErrors.contains(key.getLabel())) {
         continue;
       }
-      SkyKey configKey = BuildConfigurationValue.key(fragmentsMap.get(key.getLabel()),
-          getDynamicConfigOptions(key, fromOptions));
+      Set<Class<? extends BuildConfiguration.Fragment>> depFragments =
+          fragmentsMap.get(key.getLabel());
+      SkyKey configKey = BuildConfigurationValue.key(depFragments,
+          getDynamicConfigOptions(key, fromOptions, depFragments));
       builder.put(key, ((BuildConfigurationValue) configsResult.get(configKey)).getConfiguration());
     }
 
@@ -1279,14 +1286,19 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
    * Computes the build options needed for the given key, accounting for transitions possibly
    * specified in the key.
    */
-  private BuildOptions getDynamicConfigOptions(Dependency key, BuildOptions fromOptions) {
+  private BuildOptions getDynamicConfigOptions(Dependency key, BuildOptions fromOptions,
+      Iterable<Class<? extends BuildConfiguration.Fragment>> requiredFragments) {
     if (key.hasStaticConfiguration()) {
       return key.getConfiguration().getOptions();
-    } else if (key.getTransition() == Attribute.ConfigurationTransition.NONE) {
-      return fromOptions;
+    }
+    BuildOptions toOptions;
+    if (key.getTransition() == Attribute.ConfigurationTransition.NONE) {
+      toOptions = fromOptions;
     } else {
-      return ((PatchTransition) key.getTransition()).apply(fromOptions);
+      toOptions = ((PatchTransition) key.getTransition()).apply(fromOptions);
     }
+    return toOptions.trim(
+        BuildConfiguration.getOptionsClasses(requiredFragments, ruleClassProvider));
   }
 
   /**