diff options
author | gregce <gregce@google.com> | 2017-06-02 16:04:07 -0400 |
---|---|---|
committer | John Cater <jcater@google.com> | 2017-06-05 10:18:57 -0400 |
commit | f19fcfebb81759a5ab4fe6bede35195287f89872 (patch) | |
tree | 26a0a1ee0edc18705178a6003cb63c63852139cd /src/main/java/com/google/devtools/build/lib | |
parent | 4169ae7eeea951b9df9b4a77e78411180935a3c6 (diff) |
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
Diffstat (limited to 'src/main/java/com/google/devtools/build/lib')
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) { |