Automated g4 rollforward of commit b71e99b1f3746103e5d6802eebc24096b3494959.

(Automated g4 rollback of commit de92f9d8ea093416fae999073bbfcf3cf501ab55).

*** Reason for rollback ***

The problems that forced commit de92f9d8ea093416fae999073bbfcf3cf501ab55 were fixed in commit e6392cd380fce14d719890c78d5eb2657e8a6cfc .

*** Original change description being rolled forward ***

Implement dynamically configured LIPO builds.

Quick overview:
 - provide a dynamic interface for getting the artifact owner
   configuration
 - provide a (dynamic) RuleTransitionFactory LIPO_ON_DEMAND to replace
   the (static) RuleClass.Configurator LIPO_ON_DEMAND. Eventually
   we'll remove the rule class configurator interface entirely....

***

ROLLBACK_OF=156180015
PiperOrigin-RevId: 157865224

9 files changed, 151 insertions, 35 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
index 3e48b7a59c..db4fe08c5c 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
@@ -28,6 +28,8 @@ import com.google.devtools.build.lib.actions.Root;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment;
 import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
+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.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
@@ -56,10 +58,13 @@ import com.google.devtools.build.lib.packages.SkylarkProviderIdentifier;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.rules.SkylarkRuleConfiguredTargetBuilder;
 import com.google.devtools.build.lib.rules.fileset.FilesetProvider;
+import com.google.devtools.build.lib.skyframe.BuildConfigurationValue;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
 import com.google.devtools.build.lib.util.OrderedSetMultimap;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.skyframe.SkyFunction;
+
 import java.util.ArrayList;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -142,14 +147,21 @@ public final class ConfiguredTargetFactory {
     return null;
   }
 
-  private Artifact getOutputArtifact(OutputFile outputFile, BuildConfiguration configuration,
-      boolean isFileset, ArtifactFactory artifactFactory) {
+  /**
+   * Returns the output artifact for the given file, or null if Skyframe deps are missing.
+   */
+  private Artifact getOutputArtifact(AnalysisEnvironment analysisEnvironment, OutputFile outputFile,
+      BuildConfiguration configuration, boolean isFileset, ArtifactFactory artifactFactory)
+      throws InterruptedException {
     Rule rule = outputFile.getAssociatedRule();
     Root root = rule.hasBinaryOutput()
         ? configuration.getBinDirectory(rule.getRepository())
         : configuration.getGenfilesDirectory(rule.getRepository());
-    ArtifactOwner owner =
-        new ConfiguredTargetKey(rule.getLabel(), configuration.getArtifactOwnerConfiguration());
+    ArtifactOwner owner = new ConfiguredTargetKey(rule.getLabel(),
+        getArtifactOwnerConfiguration(analysisEnvironment.getSkyframeEnv(), configuration));
+    if (analysisEnvironment.getSkyframeEnv().valuesMissing()) {
+      return null;
+    }
     PathFragment rootRelativePath =
         outputFile.getLabel().getPackageIdentifier().getSourceRoot().getRelative(
             outputFile.getLabel().getName());
@@ -162,6 +174,37 @@ public final class ConfiguredTargetFactory {
   }
 
   /**
+   * Returns the configuration's artifact owner (which may be null). Also returns null if the
+   * owning configuration isn't yet available from Skyframe.
+   */
+  public static BuildConfiguration getArtifactOwnerConfiguration(SkyFunction.Environment env,
+      BuildConfiguration fromConfig) throws InterruptedException {
+    if (fromConfig == null) {
+      return null;
+    }
+    if (!fromConfig.useDynamicConfigurations()) {
+      return fromConfig.getArtifactOwnerConfiguration();
+    }
+    PatchTransition ownerTransition = fromConfig.getArtifactOwnerTransition();
+    if (ownerTransition == null) {
+      return fromConfig;
+    }
+    try {
+      BuildConfigurationValue ownerConfig = (BuildConfigurationValue) env.getValueOrThrow(
+          BuildConfigurationValue.key(
+              fromConfig.fragmentClasses(), ownerTransition.apply(fromConfig.getOptions())),
+          InvalidConfigurationException.class);
+      return ownerConfig == null ? null : ownerConfig.getConfiguration();
+    } catch (InvalidConfigurationException e) {
+      // We don't expect to have to handle an invalid configuration because in practice the owning
+      // configuration should already exist. For example, the main user of this feature, the LIPO
+      // context collector, expects the owning configuration to be the top-level target config.
+      throw new IllegalStateException(
+          "this method should only return a pre-existing valid configuration");
+    }
+  }
+
+  /**
    * Invokes the appropriate constructor to create a {@link ConfiguredTarget} instance.
    * <p>For use in {@code ConfiguredTargetFunction}.
    *
@@ -187,7 +230,11 @@ public final class ConfiguredTargetFactory {
     if (target instanceof OutputFile) {
       OutputFile outputFile = (OutputFile) target;
       boolean isFileset = outputFile.getGeneratingRule().getRuleClass().equals("Fileset");
-      Artifact artifact = getOutputArtifact(outputFile, config, isFileset, artifactFactory);
+      Artifact artifact =
+          getOutputArtifact(analysisEnvironment, outputFile, config, isFileset, artifactFactory);
+      if (analysisEnvironment.getSkyframeEnv().valuesMissing()) {
+        return null;
+      }
       TransitiveInfoCollection rule = targetContext.findDirectPrerequisite(
           outputFile.getGeneratingRule().getLabel(), config);
       if (isFileset) {
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 62307e489c..4e37488ad6 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
@@ -207,6 +207,18 @@ public final class BuildConfiguration implements BuildEvent {
     }
 
     /**
+     * Returns the transition that produces the "artifact owner" for this configuration, or null
+     * if this configuration is its own owner.
+     *
+     * <p>If multiple fragments return the same transition, that transition is only applied
+     * once. Multiple fragments may not return different non-null transitions.
+     */
+    @Nullable
+    public PatchTransition getArtifactOwnerTransition() {
+      return null;
+    }
+
+    /**
      * Returns an extra transition that should apply to top-level targets in this
      * configuration. Returns null if no transition is needed.
      *
@@ -1997,16 +2009,21 @@ public final class BuildConfiguration implements BuildEvent {
           if (currentTransition == ConfigurationTransition.NONE) {
             currentTransition = ruleClassTransition;
           } else {
-            currentTransition = new ComposingSplitTransition(ruleClassTransition,
-                currentTransition);
+            currentTransition = new ComposingSplitTransition(currentTransition,
+                ruleClassTransition);
           }
         }
       }
 
-      // We don't support rule class configurators (which may need intermediate configurations to
-      // apply). The only current use of that is LIPO, which can't currently be invoked with dynamic
-      // configurations (e.g. this code can never get called for LIPO builds). So check that
-      // if there is a configurator, it's for LIPO, in which case we can ignore it.
+      /**
+       * Dynamic configurations don't support rule class configurators (which may need intermediate
+       * configurations to apply). The only current use of that is LIPO, which dynamic
+       * configurations have a different code path for:
+       * {@link com.google.devtools.build.lib.rules.cpp.CppRuleClasses.LIPO_ON_DEMAND}.
+       *
+       * So just check that if there is a configurator, it's for LIPO, in which case we can ignore
+       * it.
+       */
       if (associatedRule != null) {
         @SuppressWarnings("unchecked")
         RuleClass.Configurator<?, ?> func =
@@ -2646,15 +2663,38 @@ public final class BuildConfiguration implements BuildEvent {
   }
 
   /**
+   * Returns the transition that produces the "artifact owner" for this configuration, or null
+   * if this configuration is its own owner.
+   *
+   * <p>This is the dynamic configuration version of {@link #getArtifactOwnerConfiguration}.
+   */
+  @Nullable
+  public PatchTransition getArtifactOwnerTransition() {
+    Preconditions.checkState(useDynamicConfigurations());
+    PatchTransition ownerTransition = null;
+    for (Fragment fragment : fragments.values()) {
+      PatchTransition fragmentTransition = fragment.getArtifactOwnerTransition();
+      if (fragmentTransition != null) {
+        if (ownerTransition != null) {
+          Verify.verify(ownerTransition == fragmentTransition,
+              String.format(
+                  "cannot determine owner transition: fragments returning both %s and %s",
+                  ownerTransition.toString(), fragmentTransition.toString()));
+        }
+        ownerTransition = fragmentTransition;
+      }
+    }
+    return ownerTransition;
+  }
+
+  /**
    * See {@code BuildConfigurationCollection.Transitions.getArtifactOwnerConfiguration()}.
+   *
+   * <p>This is the static configuration version of {@link #getArtifactOwnerTransition}.
    */
   public BuildConfiguration getArtifactOwnerConfiguration() {
-    // Dynamic configurations inherit transitions objects from other configurations exclusively
-    // for use of Transitions.getDynamicTransition. No other calls to transitions should be
-    // made for dynamic configurations.
-    // TODO(bazel-team): enforce the above automatically (without having to explicitly check
-    // for dynamic configuration mode).
-    return useDynamicConfigurations() ? this : transitions.getArtifactOwnerConfiguration();
+    Preconditions.checkState(!useDynamicConfigurations());
+    return transitions.getArtifactOwnerConfiguration();
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCcBinaryRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCcBinaryRule.java
index c8bd09d590..26dbe4c04f 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCcBinaryRule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCcBinaryRule.java
@@ -24,6 +24,7 @@ import com.google.devtools.build.lib.bazel.rules.cpp.BazelCppRuleClasses.CcBinar
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.packages.RuleClass.Builder;
 import com.google.devtools.build.lib.rules.cpp.CppConfiguration;
+import com.google.devtools.build.lib.rules.cpp.CppRuleClasses;
 
 /** Rule definition for cc_binary rules. */
 public final class BazelCcBinaryRule implements RuleDefinition {
@@ -72,7 +73,8 @@ public final class BazelCcBinaryRule implements RuleDefinition {
             attr("linkshared", BOOLEAN)
                 .value(false)
                 .nonconfigurable("used to *determine* the rule's configuration"))
-        .cfg(BazelCppRuleClasses.LIPO_ON_DEMAND)
+        .cfg(BazelCppRuleClasses.LIPO_ON_DEMAND) // static configuration version
+        .cfg(CppRuleClasses.LIPO_ON_DEMAND) // dynamic configuration version
         .build();
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index 5a5b58de92..cef8c16a6a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -760,7 +760,7 @@ public class RuleClass {
     public Builder cfg(Transition transition) {
       Preconditions.checkState(type != RuleClassType.ABSTRACT,
           "Setting not inherited property (cfg) of abstract rule class '%s'", name);
-      Preconditions.checkState(this.transitionFactory == null && this.configurator == NO_CHANGE,
+      Preconditions.checkState(this.transitionFactory == null,
           "Property cfg has already been set");
       Preconditions.checkNotNull(transition);
       this.transitionFactory = new FixedTransitionFactory(transition);
@@ -770,7 +770,7 @@ public class RuleClass {
     public Builder cfg(RuleTransitionFactory transitionFactory) {
       Preconditions.checkState(type != RuleClassType.ABSTRACT,
           "Setting not inherited property (cfg) of abstract rule class '%s'", name);
-      Preconditions.checkState(this.transitionFactory == null && this.configurator == NO_CHANGE,
+      Preconditions.checkState(this.transitionFactory == null,
           "Property cfg has already been set");
       Preconditions.checkNotNull(transitionFactory);
       this.transitionFactory = transitionFactory;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
index 56cc775214..f974c35624 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
@@ -43,6 +43,7 @@ import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.rules.cpp.CppConfigurationLoader.CppConfigurationParameters;
 import com.google.devtools.build.lib.rules.cpp.CppLinkActionConfigs.CppLinkPlatform;
 import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType;
+import com.google.devtools.build.lib.rules.cpp.transitions.ContextCollectorOwnerTransition;
 import com.google.devtools.build.lib.rules.cpp.transitions.DisableLipoTransition;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
@@ -2148,6 +2149,11 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
     return defaultSysroot;
   }
 
+  @Override
+  public PatchTransition getArtifactOwnerTransition() {
+    return isLipoContextCollector() ? ContextCollectorOwnerTransition.INSTANCE : null;
+  }
+
   @Nullable
   @Override
   public PatchTransition topLevelConfigurationHook(Target toTarget) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java
index ec418e82fa..b334b10923 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfigurationLoader.java
@@ -75,14 +75,7 @@ public class CppConfigurationLoader implements ConfigurationFragmentFactory {
     if (params == null) {
       return null;
     }
-    CppConfiguration cppConfig = new CppConfiguration(params);
-    if (options.get(BuildConfiguration.Options.class).useDynamicConfigurations
-        != BuildConfiguration.Options.DynamicConfigsMode.OFF
-        && (cppConfig.isFdo() || cppConfig.getLipoMode() != CrosstoolConfig.LipoMode.OFF)) {
-      throw new InvalidConfigurationException(
-          "LIPO does not currently work with dynamic configurations");
-    }
-    return cppConfig;
+    return new CppConfiguration(params);
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java
index 04c3368cc2..1e6e1b7fb8 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java
@@ -33,11 +33,14 @@ import static com.google.devtools.build.lib.rules.cpp.CppFileTypes.VERSIONED_SHA
 import com.google.devtools.build.lib.analysis.LanguageDependentFragment.LibraryLanguage;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.Attribute.LateBoundLabel;
 import com.google.devtools.build.lib.packages.Attribute.Transition;
 import com.google.devtools.build.lib.packages.AttributeMap;
 import com.google.devtools.build.lib.packages.ImplicitOutputsFunction.SafeImplicitOutputsFunction;
 import com.google.devtools.build.lib.packages.Rule;
+import com.google.devtools.build.lib.packages.RuleTransitionFactory;
+import com.google.devtools.build.lib.rules.cpp.transitions.EnableLipoTransition;
 import com.google.devtools.build.lib.rules.test.InstrumentedFilesCollector.InstrumentationSpec;
 import com.google.devtools.build.lib.util.FileTypeSet;
 
@@ -84,6 +87,20 @@ public class CppRuleClasses {
   }
 
   /**
+   * Rule transition factory that enables LIPO on the LIPO context binary (i.e. applies a DATA ->
+   * TARGET transition).
+   *
+   * <p>This is how dynamic configurations enable LIPO on the LIPO context.
+   */
+  public static final RuleTransitionFactory LIPO_ON_DEMAND =
+      new RuleTransitionFactory() {
+        @Override
+        public Attribute.Transition buildTransitionFor(Rule rule) {
+          return new EnableLipoTransition(rule.getLabel());
+        }
+      };
+
+  /**
    * Label of a pseudo-filegroup that contains all crosstool and libcfiles for all configurations,
    * as specified on the command-line.
    */
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 3019857cc4..3ce85d10a0 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
@@ -36,6 +36,7 @@ import com.google.devtools.build.lib.analysis.AspectCollection.AspectDeps;
 import com.google.devtools.build.lib.analysis.CachingAnalysisEnvironment;
 import com.google.devtools.build.lib.analysis.ConfiguredAspect;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.ConfiguredTargetFactory;
 import com.google.devtools.build.lib.analysis.Dependency;
 import com.google.devtools.build.lib.analysis.DependencyResolver.InconsistentAspectOrderException;
 import com.google.devtools.build.lib.analysis.LabelAndConfiguration;
@@ -1073,7 +1074,7 @@ final class ConfiguredTargetFunction implements SkyFunction {
     boolean failed = false;
     Iterable<SkyKey> depKeys = Iterables.transform(deps, TO_KEYS);
     Map<SkyKey, ValueOrException<ConfiguredValueCreationException>> depValuesOrExceptions =
-        env.getValuesOrThrow(depKeys, ConfiguredValueCreationException.class);
+            env.getValuesOrThrow(depKeys, ConfiguredValueCreationException.class);
     Map<SkyKey, ConfiguredTarget> result =
         Maps.newHashMapWithExpectedSize(depValuesOrExceptions.size());
     for (Map.Entry<SkyKey, ValueOrException<ConfiguredValueCreationException>> entry
@@ -1119,8 +1120,11 @@ final class ConfiguredTargetFunction implements SkyFunction {
       NestedSetBuilder<Package> transitivePackages)
       throws ConfiguredTargetFunctionException, InterruptedException {
     StoredEventHandler events = new StoredEventHandler();
-    BuildConfiguration ownerConfig = (configuration == null)
-        ? null : configuration.getArtifactOwnerConfiguration();
+    BuildConfiguration ownerConfig =
+        ConfiguredTargetFactory.getArtifactOwnerConfiguration(env, configuration);
+    if (env.valuesMissing()) {
+      return null;
+    }
     CachingAnalysisEnvironment analysisEnvironment = view.createAnalysisEnvironment(
         new ConfiguredTargetKey(target.getLabel(), ownerConfig), false,
         events, env, configuration);
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 b28b237eab..77d8394679 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
@@ -1466,10 +1466,17 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       BuildConfiguration configuration,
       Attribute.Transition transition) {
 
-    Preconditions.checkArgument(configuration == null
-        || configuration.useDynamicConfigurations()
-        || transition == ConfigurationTransition.NONE,
-        "Dynamic configurations required for test configuration using a transition");
+    if (configuration != null && !configuration.useDynamicConfigurations()
+        && transition != ConfigurationTransition.NONE) {
+      // It's illegal to apply this transition over a statically configured build. But C++ LIPO
+      // support works by applying a rule configurator for static configurations and a rule
+      // transition applier for dynamic configurations. Dynamically configured builds skip
+      // the configurator and this code makes statically configured builds skip the rule transition
+      // applier.
+      //
+      // This will all get a lot simpler once static configurations are removed entirely.
+      transition = ConfigurationTransition.NONE;
+    }
 
     if (memoizingEvaluator.getExistingValueForTesting(
         PrecomputedValue.WORKSPACE_STATUS_KEY.getKeyForTesting()) == null) {