Fixes incomplete support for dynamic split transitions in Bazel's

test infrastructure.

The small picture story is that SkyframeExecutor.getDynamicConfigOptions
(which gets dynamic BuildOptions for tests) wasn't updated with dynamic
split support when that was added to
ConfiguredTargetFunction.getDynamicTransitionOptions (which does the same
thing for production builds). This change plugs that gap. See
373e3e28274cca5b87f48abe33884edb84016dd3 for the original change.

The bigger picture story is that Bazel's configured target creation
logic is forked: test code goes down a similar but sadly not-quite-the-same
path that results in tons of duplicated logic, spaghetti code mess,
and risk of bugs like this one. We'd like to ultimately undo that fork.
But unfortunately it's an involved effort that won't happen overnight. In
the meantime, this change takes one small step by merging the two methods
that caused this bug.

--
MOS_MIGRATED_REVID=139342710

2 files changed, 37 insertions, 45 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
index ed7673e7a7..71a938ab8d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -551,14 +551,8 @@ final class ConfiguredTargetFunction implements SkyFunction {
       List<BuildOptions> toOptions = transitionsMap.get(transitionKey);
       if (toOptions == null) {
         ImmutableList.Builder<BuildOptions> toOptionsBuilder = ImmutableList.builder();
-        for (BuildOptions options : getDynamicTransitionOptions(ctgOptions, transition)) {
-          if (!sameFragments) {
-            options = options.trim(
-                BuildConfiguration.getOptionsClasses(depFragments, ruleClassProvider));
-          }
-          toOptionsBuilder.add(options);
-        }
-        toOptions = toOptionsBuilder.build();
+        toOptions = getDynamicTransitionOptions(ctgOptions, transition, depFragments,
+            ruleClassProvider, !sameFragments);
         transitionsMap.put(transitionKey, toOptions);
       }
 
@@ -655,16 +649,20 @@ final class ConfiguredTargetFunction implements SkyFunction {
   /**
    * Applies a dynamic configuration transition over a set of build options.
    *
-   * @return the build options for the transitioned configuration. Contains the same fragment
-   *     options as the input.
+   * @return the build options for the transitioned configuration. If trimResults is true,
+   *     only options needed by the required fragments are included. Else the same options as the
+   *     original input are included (with different possible values, of course).
    */
-  private static Collection<BuildOptions> getDynamicTransitionOptions(BuildOptions fromOptions,
-      Attribute.Transition transition) {
+  static List<BuildOptions> getDynamicTransitionOptions(BuildOptions fromOptions,
+      Attribute.Transition transition,
+      Iterable<Class<? extends BuildConfiguration.Fragment>> requiredFragments,
+      RuleClassProvider ruleClassProvider, boolean trimResults) {
+    List<BuildOptions> result;
     if (transition == Attribute.ConfigurationTransition.NONE) {
-      return ImmutableList.<BuildOptions>of(fromOptions);
+      result = ImmutableList.<BuildOptions>of(fromOptions);
     } else if (transition instanceof PatchTransition) {
       // TODO(bazel-team): safety-check that this never mutates fromOptions.
-      return ImmutableList.<BuildOptions>of(((PatchTransition) transition).apply(fromOptions));
+      result = ImmutableList.<BuildOptions>of(((PatchTransition) transition).apply(fromOptions));
     } else if (transition instanceof Attribute.SplitTransition) {
       @SuppressWarnings("unchecked") // Attribute.java doesn't have the BuildOptions symbol.
       List<BuildOptions> toOptions =
@@ -673,16 +671,26 @@ final class ConfiguredTargetFunction implements SkyFunction {
         // When the split returns an empty list, it's signaling it doesn't apply to this instance.
         // Check that it's safe to skip the transition and return the original options.
         Verify.verify(transition.defaultsToSelf());
-        return ImmutableList.<BuildOptions>of(fromOptions);
+        result = ImmutableList.<BuildOptions>of(fromOptions);
       } else {
-        return toOptions;
+        result = toOptions;
       }
     } else {
       throw new IllegalStateException(String.format(
           "unsupported dynamic transition type: %s", transition.getClass().getName()));
     }
-  }
 
+    if (!trimResults) {
+      return result;
+    } else {
+      ImmutableList.Builder<BuildOptions> trimmedOptions = ImmutableList.builder();
+      for (BuildOptions toOptions : result) {
+        trimmedOptions.add(toOptions.trim(
+            BuildConfiguration.getOptionsClasses(requiredFragments, ruleClassProvider)));
+      }
+      return trimmedOptions.build();
+    }
+  }
 
   /**
    * Diagnostic helper method for dynamic configurations: checks the config fragments required by
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 a8fa1ae0b0..96b84a86a2 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
@@ -68,7 +68,6 @@ import com.google.devtools.build.lib.analysis.config.BuildOptions;
 import com.google.devtools.build.lib.analysis.config.ConfigurationFactory;
 import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory;
 import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
-import com.google.devtools.build.lib.analysis.config.PatchTransition;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.TargetParsingException;
@@ -1324,29 +1323,33 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     // Now get the configurations.
     final List<SkyKey> configSkyKeys = new ArrayList<>();
     for (Dependency key : keys) {
-      if (labelsWithErrors.contains(key.getLabel())) {
+      if (labelsWithErrors.contains(key.getLabel()) || key.hasStaticConfiguration()) {
         continue;
       }
       Set<Class<? extends BuildConfiguration.Fragment>> depFragments =
           fragmentsMap.get(key.getLabel());
       if (depFragments != null) {
-        configSkyKeys.add(BuildConfigurationValue.key(depFragments,
-            getDynamicConfigOptions(key, fromOptions, depFragments)));
+        for (BuildOptions toOptions : ConfiguredTargetFunction.getDynamicTransitionOptions(
+            fromOptions, key.getTransition(), depFragments, ruleClassProvider, true)) {
+          configSkyKeys.add(BuildConfigurationValue.key(depFragments, toOptions));
+        }
       }
     }
     EvaluationResult<SkyValue> configsResult =
         evaluateSkyKeys(eventHandler, configSkyKeys, /*keepGoing=*/true);
     for (Dependency key : keys) {
-      if (labelsWithErrors.contains(key.getLabel())) {
+      if (labelsWithErrors.contains(key.getLabel()) || key.hasStaticConfiguration()) {
         continue;
       }
       Set<Class<? extends BuildConfiguration.Fragment>> depFragments =
           fragmentsMap.get(key.getLabel());
       if (depFragments != null) {
-        SkyKey configKey = BuildConfigurationValue.key(depFragments,
-            getDynamicConfigOptions(key, fromOptions, depFragments));
-        builder
-            .put(key, ((BuildConfigurationValue) configsResult.get(configKey)).getConfiguration());
+        for (BuildOptions toOptions : ConfiguredTargetFunction.getDynamicTransitionOptions(
+            fromOptions, key.getTransition(), depFragments, ruleClassProvider, true)) {
+          SkyKey configKey = BuildConfigurationValue.key(depFragments, toOptions);
+          builder.put(key,
+              ((BuildConfigurationValue) configsResult.get(configKey)).getConfiguration());
+        }
       }
     }
 
@@ -1363,25 +1366,6 @@ 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,
-      Iterable<Class<? extends BuildConfiguration.Fragment>> requiredFragments) {
-    if (key.hasStaticConfiguration()) {
-      return key.getConfiguration().getOptions();
-    }
-    BuildOptions toOptions;
-    if (key.getTransition() == Attribute.ConfigurationTransition.NONE) {
-      toOptions = fromOptions;
-    } else {
-      toOptions = ((PatchTransition) key.getTransition()).apply(fromOptions);
-    }
-    return toOptions.trim(
-        BuildConfiguration.getOptionsClasses(requiredFragments, ruleClassProvider));
-  }
-
-  /**
    * Evaluates the given sky keys, blocks, and returns their evaluation results. Fails fast
    * on the first evaluation error.
    */