Make split configuration transitions work with Bazel.

Creating the split configurations in Bazel uncovered an incrementality issue: ConfigurationFactory.hostConfigCache kept state between builds untracked by Skyframe, which is not good, and therefore had to be fixed.

--
MOS_MIGRATED_REVID=97106917

6 files changed, 75 insertions, 54 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationCollectionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationCollectionFactory.java
index 4ce7426433..14bb2a9695 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationCollectionFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationCollectionFactory.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.analysis;
 
+import com.google.common.cache.Cache;
 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.ConfigurationFactory;
@@ -32,6 +33,8 @@ public interface ConfigurationCollectionFactory {
    * <p>Also it may create a set of BuildConfigurations and define a transition table over them.
    * All configurations during a build should be accessible from this top-level configuration
    * via configuration transitions.
+   * @param configurationFactory the configuration factory
+   * @param cache a cache for BuildConfigurations
    * @param loadedPackageProvider the package provider
    * @param buildOptions top-level build options representing the command-line
    * @param errorEventListener the event listener for errors
@@ -43,6 +46,7 @@ public interface ConfigurationCollectionFactory {
   @Nullable
   BuildConfiguration createConfigurations(
       ConfigurationFactory configurationFactory,
+      Cache<String, BuildConfiguration> cache,
       PackageProviderForConfigurations loadedPackageProvider,
       BuildOptions buildOptions,
       EventHandler errorEventListener,
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index b81ea289b5..b1598f6748 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -209,8 +209,7 @@ public final class RuleContext extends TargetContext
   }
 
   /**
-   * Returns the host configuration for this rule; keep in mind that there may be multiple different
-   * host configurations, even during a single build.
+   * Returns the host configuration for this rule. 
    */
   public BuildConfiguration getHostConfiguration() {
     return hostConfiguration;
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index 7a2e1ad619..ecd96136ff 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -1152,7 +1152,6 @@ public final class BuildConfiguration implements Serializable {
   }
 
   public Transitions getTransitions() {
-    Preconditions.checkState(this.transitions != null || isHostConfiguration());
     return transitions;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationFactory.java
index e623bdf38d..4ae2349c21 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationFactory.java
@@ -17,7 +17,6 @@ package com.google.devtools.build.lib.analysis.config;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.cache.Cache;
-import com.google.common.cache.CacheBuilder;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSortedMap;
@@ -39,7 +38,7 @@ import javax.annotation.Nullable;
  * and should be simplified in the future, if
  * possible. Right now, creating a {@link BuildConfiguration} instance involves
  * creating the instance itself and the related configurations; the main method
- * is {@link #createConfiguration}.
+ * is {@link #createConfigurations}.
  *
  * <p>Avoid calling into this class, and instead use the skyframe infrastructure to obtain
  * configuration instances.
@@ -51,11 +50,6 @@ import javax.annotation.Nullable;
 public final class ConfigurationFactory {
   private final List<ConfigurationFragmentFactory> configurationFragmentFactories;
   private final ConfigurationCollectionFactory configurationCollectionFactory;
-
-  // A cache of key to configuration instances.
-  private final Cache<String, BuildConfiguration> hostConfigCache =
-      CacheBuilder.newBuilder().softValues().build();
-
   private boolean performSanityCheck = true;
 
   public ConfigurationFactory(
@@ -77,37 +71,30 @@ public final class ConfigurationFactory {
     performSanityCheck = false;
   }
 
-  /** Create the build configurations with the given options. */
+  /** Creates a set of build configurations with top-level configuration having the given options.
+   *
+   * <p>The rest of the configurations are created based on the set of transitions available.
+   */
   @Nullable
-  public BuildConfiguration createConfiguration(
+  public BuildConfiguration createConfigurations(
+      Cache<String, BuildConfiguration> cache,
       PackageProviderForConfigurations loadedPackageProvider, BuildOptions buildOptions,
       EventHandler errorEventListener)
           throws InvalidConfigurationException {
-    return configurationCollectionFactory.createConfigurations(this,
+    return configurationCollectionFactory.createConfigurations(this, cache,
         loadedPackageProvider, buildOptions, errorEventListener, performSanityCheck);
   }
 
   /**
-   * Returns a (possibly new) canonical host BuildConfiguration instance based
-   * upon a given request configuration
-   */
-  @Nullable
-  public BuildConfiguration getHostConfiguration(
-      PackageProviderForConfigurations loadedPackageProvider,
-      BuildOptions buildOptions, boolean fallback) throws InvalidConfigurationException {
-    return getConfiguration(loadedPackageProvider, buildOptions.createHostOptions(fallback),
-        false, hostConfigCache);
-  }
-
-  /**
-   * The core of BuildConfiguration creation. All host and target instances are
-   * constructed and cached here.
+   * Returns a {@link com.google.devtools.build.lib.analysis.config.BuildConfiguration} based on
+   * the given set of build options.
+   *
+   * <p>If the configuration has already been created, re-uses it, otherwise, creates a new one.
    */
   @Nullable
   public BuildConfiguration getConfiguration(PackageProviderForConfigurations loadedPackageProvider,
-      BuildOptions buildOptions,
-      boolean actionsDisabled, Cache<String, BuildConfiguration> cache)
-          throws InvalidConfigurationException {
+      BuildOptions buildOptions, boolean actionsDisabled, Cache<String, BuildConfiguration> cache)
+      throws InvalidConfigurationException {
 
     Map<Class<? extends Fragment>, Fragment> fragments = new HashMap<>();
     // Create configuration fragments
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationCollection.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationCollection.java
index 47689efa0d..d6dd55e262 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationCollection.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationCollection.java
@@ -15,9 +15,10 @@
 package com.google.devtools.build.lib.bazel.rules;
 
 import com.google.common.cache.Cache;
-import com.google.common.cache.CacheBuilder;
+import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.HashBasedTable;
-import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableListMultimap;
+import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Table;
 import com.google.devtools.build.lib.analysis.ConfigurationCollectionFactory;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
@@ -32,6 +33,7 @@ import com.google.devtools.build.lib.bazel.rules.cpp.BazelCppRuleClasses.CppTran
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.AggregatingAttributeMapper;
 import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition;
+import com.google.devtools.build.lib.packages.Attribute.SplitTransition;
 import com.google.devtools.build.lib.packages.Attribute.Transition;
 import com.google.devtools.build.lib.packages.NoSuchThingException;
 import com.google.devtools.build.lib.packages.Rule;
@@ -41,6 +43,7 @@ import com.google.devtools.build.lib.syntax.Label;
 
 import java.util.Collection;
 import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -55,18 +58,11 @@ public class BazelConfigurationCollection implements ConfigurationCollectionFact
   @Nullable
   public BuildConfiguration createConfigurations(
       ConfigurationFactory configurationFactory,
+      Cache<String, BuildConfiguration> cache,
       PackageProviderForConfigurations loadedPackageProvider,
       BuildOptions buildOptions,
       EventHandler errorEventListener,
       boolean performSanityCheck) throws InvalidConfigurationException {
-
-    // We cache all the related configurations for this target configuration in a cache that is
-    // dropped at the end of this method call. We instead rely on the cache for entire collections
-    // for caching the target and related configurations, and on a dedicated host configuration
-    // cache for the host configuration.
-    Cache<String, BuildConfiguration> cache =
-        CacheBuilder.newBuilder().<String, BuildConfiguration>build();
-
     // Target configuration
     BuildConfiguration targetConfiguration = configurationFactory.getConfiguration(
         loadedPackageProvider, buildOptions, false, cache);
@@ -80,11 +76,26 @@ public class BazelConfigurationCollection implements ConfigurationCollectionFact
     // Note that this passes in the dataConfiguration, not the target
     // configuration. This is intentional.
     BuildConfiguration hostConfiguration = getHostConfigurationFromRequest(configurationFactory,
-        loadedPackageProvider, dataConfiguration, buildOptions);
+        loadedPackageProvider, dataConfiguration, buildOptions, cache);
     if (hostConfiguration == null) {
       return null;
     }
 
+    ListMultimap<SplitTransition<?>, BuildConfiguration> splitTransitionsTable =
+        ArrayListMultimap.create();
+    for (SplitTransition<BuildOptions> transition : buildOptions.getPotentialSplitTransitions()) {
+      List<BuildOptions> splitOptionsList = transition.split(buildOptions);
+      for (BuildOptions splitOptions : splitOptionsList) {
+        BuildConfiguration splitConfig = configurationFactory.getConfiguration(
+            loadedPackageProvider, splitOptions, false, cache);
+        splitTransitionsTable.put(transition, splitConfig);
+      }
+    }
+    if (loadedPackageProvider.valuesMissing()) {
+      return null;
+    }
+
+
     // Sanity check that the implicit labels are all in the transitive closure of explicit ones.
     // This also registers all targets in the cache entry and validates them on subsequent requests.
     Set<Label> reachableLabels = new HashSet<>();
@@ -104,7 +115,7 @@ public class BazelConfigurationCollection implements ConfigurationCollectionFact
     }
 
     BuildConfiguration result = setupTransitions(
-        targetConfiguration, dataConfiguration, hostConfiguration);
+        targetConfiguration, dataConfiguration, hostConfiguration, splitTransitionsTable);
     result.reportInvalidOptions(errorEventListener);
     return result;
   }
@@ -130,14 +141,15 @@ public class BazelConfigurationCollection implements ConfigurationCollectionFact
   private BuildConfiguration getHostConfigurationFromRequest(
       ConfigurationFactory configurationFactory,
       PackageProviderForConfigurations loadedPackageProvider,
-      BuildConfiguration requestConfig, BuildOptions buildOptions)
+      BuildConfiguration requestConfig, BuildOptions buildOptions,
+      Cache<String, BuildConfiguration> cache)
       throws InvalidConfigurationException {
     BuildConfiguration.Options commonOptions = buildOptions.get(BuildConfiguration.Options.class);
     if (!commonOptions.useDistinctHostConfiguration) {
       return requestConfig;
     } else {
-      BuildConfiguration hostConfig = configurationFactory.getHostConfiguration(
-          loadedPackageProvider, buildOptions, /*fallback=*/false);
+      BuildConfiguration hostConfig = configurationFactory.getConfiguration(
+          loadedPackageProvider, buildOptions.createHostOptions(false), false, cache);
       if (hostConfig == null) {
         return null;
       }
@@ -146,9 +158,13 @@ public class BazelConfigurationCollection implements ConfigurationCollectionFact
   }
 
   static BuildConfiguration setupTransitions(BuildConfiguration targetConfiguration,
-      BuildConfiguration dataConfiguration, BuildConfiguration hostConfiguration) {
-    Set<BuildConfiguration> allConfigurations = ImmutableSet.of(targetConfiguration,
-        dataConfiguration, hostConfiguration);
+      BuildConfiguration dataConfiguration, BuildConfiguration hostConfiguration,
+      ListMultimap<SplitTransition<?>, BuildConfiguration> splitTransitionsTable) {
+    Set<BuildConfiguration> allConfigurations = new LinkedHashSet<>();
+    allConfigurations.add(targetConfiguration);
+    allConfigurations.add(dataConfiguration);
+    allConfigurations.add(hostConfiguration);
+    allConfigurations.addAll(splitTransitionsTable.values());
 
     Table<BuildConfiguration, Transition, ConfigurationHolder> transitionBuilder =
         HashBasedTable.create();
@@ -177,7 +193,13 @@ public class BazelConfigurationCollection implements ConfigurationCollectionFact
 
     for (BuildConfiguration config : allConfigurations) {
       Transitions outgoingTransitions =
-          new BuildConfigurationCollection.Transitions(config, transitionBuilder.row(config));
+          new BuildConfigurationCollection.Transitions(config, transitionBuilder.row(config),
+              // Split transitions must not have their own split transitions because then they
+              // would be applied twice due to a quirk in DependencyResolver. See the comment in
+              // DependencyResolver.resolveLateBoundAttributes().
+              splitTransitionsTable.values().contains(config)
+                  ? ImmutableListMultimap.<SplitTransition<?>, BuildConfiguration>of()
+                  : splitTransitionsTable);
       // We allow host configurations to be shared between target configurations. In that case, the
       // transitions may already be set.
       // TODO(bazel-team): Check that the transitions are identical, or even better, change the
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationCollectionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationCollectionFunction.java
index 7d2efe8214..3131868943 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationCollectionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationCollectionFunction.java
@@ -15,6 +15,8 @@ package com.google.devtools.build.lib.skyframe;
 
 import com.google.common.base.Supplier;
 import com.google.common.base.Verify;
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection;
@@ -86,11 +88,18 @@ public class ConfigurationCollectionFunction implements SkyFunction {
       PackageProviderForConfigurations loadedPackageProvider, BuildOptions buildOptions,
       ImmutableSet<String> multiCpu)
           throws InvalidConfigurationException {
+    // We cache all the related configurations for this target configuration in a cache that is
+    // dropped at the end of this method call. We instead rely on the cache for entire collections
+    // for caching the target and related configurations, and on a dedicated host configuration
+    // cache for the host configuration.
+    Cache<String, BuildConfiguration> cache =
+        CacheBuilder.newBuilder().<String, BuildConfiguration>build();
     List<BuildConfiguration> targetConfigurations = new ArrayList<>();
+
     if (!multiCpu.isEmpty()) {
       for (String cpu : multiCpu) {
         BuildConfiguration targetConfiguration = createConfiguration(
-            eventHandler, loadedPackageProvider, buildOptions, cpu);
+            cache, eventHandler, loadedPackageProvider, buildOptions, cpu);
         if (targetConfiguration == null || targetConfigurations.contains(targetConfiguration)) {
           continue;
         }
@@ -101,7 +110,7 @@ public class ConfigurationCollectionFunction implements SkyFunction {
       }
     } else {
       BuildConfiguration targetConfiguration = createConfiguration(
-          eventHandler, loadedPackageProvider, buildOptions, null);
+          cache, eventHandler, loadedPackageProvider, buildOptions, null);
       if (targetConfiguration == null) {
         return null;
       }
@@ -122,7 +131,8 @@ public class ConfigurationCollectionFunction implements SkyFunction {
   }
 
   @Nullable
-  public BuildConfiguration createConfiguration(
+  private BuildConfiguration createConfiguration(
+      Cache<String, BuildConfiguration> cache,
       EventHandler originalEventListener,
       PackageProviderForConfigurations loadedPackageProvider,
       BuildOptions buildOptions, String cpuOverride) throws InvalidConfigurationException {
@@ -133,8 +143,8 @@ public class ConfigurationCollectionFunction implements SkyFunction {
       buildOptions.get(BuildConfiguration.Options.class).cpu = cpuOverride;
     }
 
-    BuildConfiguration targetConfig = configurationFactory.get().createConfiguration(
-        loadedPackageProvider, buildOptions, errorEventListener);
+    BuildConfiguration targetConfig = configurationFactory.get().createConfigurations(
+        cache, loadedPackageProvider, buildOptions, errorEventListener);
     if (targetConfig == null) {
       return null;
     }