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
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 3e48b7a..db4fe08 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.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.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 @@
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 @@
}
/**
+ * 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 @@
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 62307e4..4e37488 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 @@
}
/**
+ * 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 @@
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 @@
}
/**
+ * 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 c8bd09d..26dbe4c 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.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 @@
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 5a5b58d..cef8c16 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 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 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 56cc775..f974c35 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.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 @@
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 ec418e8..b334b10 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 @@
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 04c3368..1e6e1b7 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 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 @@
}
/**
+ * 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 3019857..3ce85d1 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.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 @@
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 @@
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 b28b237..77d8394 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 @@
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) {
