From 6c11b66b13c03fd125b4d215266d30288395dfb9 Mon Sep 17 00:00:00 2001
From: Greg Estren <gregce@google.com>
Date: Tue, 31 May 2016 20:31:19 +0000
Subject: 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
---
 .../build/lib/skyframe/SkyframeExecutor.java       | 30 +++++++++++++++-------
 1 file changed, 21 insertions(+), 9 deletions(-)

(limited to 'src')

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));
   }
 
   /**
-- 
cgit v1.2.3