diff options
Diffstat (limited to 'src/main/java/com')
9 files changed, 35 insertions, 151 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 87a31e2718..b1ec35db8b 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,8 +28,6 @@ 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; @@ -58,13 +56,10 @@ 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; @@ -147,21 +142,14 @@ public final class ConfiguredTargetFactory { return null; } - /** - * 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 { + private Artifact getOutputArtifact(OutputFile outputFile, BuildConfiguration configuration, + boolean isFileset, ArtifactFactory artifactFactory) { Rule rule = outputFile.getAssociatedRule(); Root root = rule.hasBinaryOutput() ? configuration.getBinDirectory(rule.getRepository()) : configuration.getGenfilesDirectory(rule.getRepository()); - ArtifactOwner owner = new ConfiguredTargetKey(rule.getLabel(), - getArtifactOwnerConfiguration(analysisEnvironment.getSkyframeEnv(), configuration)); - if (analysisEnvironment.getSkyframeEnv().valuesMissing()) { - return null; - } + ArtifactOwner owner = + new ConfiguredTargetKey(rule.getLabel(), configuration.getArtifactOwnerConfiguration()); PathFragment rootRelativePath = outputFile.getLabel().getPackageIdentifier().getSourceRoot().getRelative( outputFile.getLabel().getName()); @@ -174,37 +162,6 @@ 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}. * @@ -230,11 +187,7 @@ public final class ConfiguredTargetFactory { if (target instanceof OutputFile) { OutputFile outputFile = (OutputFile) target; boolean isFileset = outputFile.getGeneratingRule().getRuleClass().equals("Fileset"); - Artifact artifact = - getOutputArtifact(analysisEnvironment, outputFile, config, isFileset, artifactFactory); - if (analysisEnvironment.getSkyframeEnv().valuesMissing()) { - return null; - } + Artifact artifact = getOutputArtifact(outputFile, config, isFileset, artifactFactory); 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 3ff7f470df..170c9c604e 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,18 +207,6 @@ 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. * @@ -1999,21 +1987,16 @@ public final class BuildConfiguration implements BuildEvent { if (currentTransition == ConfigurationTransition.NONE) { currentTransition = ruleClassTransition; } else { - currentTransition = new ComposingSplitTransition(currentTransition, - ruleClassTransition); + currentTransition = new ComposingSplitTransition(ruleClassTransition, + currentTransition); } } } - /** - * 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. - */ + // 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. if (associatedRule != null) { @SuppressWarnings("unchecked") RuleClass.Configurator<?, ?> func = @@ -2642,38 +2625,15 @@ 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() { - Preconditions.checkState(!useDynamicConfigurations()); - return transitions.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(); } /** 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 26dbe4c04f..c8bd09d590 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,7 +24,6 @@ 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 { @@ -73,8 +72,7 @@ public final class BazelCcBinaryRule implements RuleDefinition { attr("linkshared", BOOLEAN) .value(false) .nonconfigurable("used to *determine* the rule's configuration")) - .cfg(BazelCppRuleClasses.LIPO_ON_DEMAND) // static configuration version - .cfg(CppRuleClasses.LIPO_ON_DEMAND) // dynamic configuration version + .cfg(BazelCppRuleClasses.LIPO_ON_DEMAND) .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 698cf58cca..9293b093ac 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 @@ -758,7 +758,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, + Preconditions.checkState(this.transitionFactory == null && this.configurator == NO_CHANGE, "Property cfg has already been set"); Preconditions.checkNotNull(transition); this.transitionFactory = new FixedTransitionFactory(transition); @@ -768,7 +768,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, + Preconditions.checkState(this.transitionFactory == null && this.configurator == NO_CHANGE, "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 765f4d6f6d..c9e148ca12 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,7 +43,6 @@ 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; @@ -2159,11 +2158,6 @@ 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 b334b10923..ec418e82fa 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,7 +75,14 @@ public class CppConfigurationLoader implements ConfigurationFragmentFactory { if (params == null) { return null; } - return new CppConfiguration(params); + 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; } /** 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 1e6e1b7fb8..04c3368cc2 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,14 +33,11 @@ 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; @@ -87,20 +84,6 @@ 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 06b61e6fde..427554fbba 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,7 +36,6 @@ 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 +1072,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,11 +1118,8 @@ final class ConfiguredTargetFunction implements SkyFunction { NestedSetBuilder<Package> transitivePackages) throws ConfiguredTargetFunctionException, InterruptedException { StoredEventHandler events = new StoredEventHandler(); - BuildConfiguration ownerConfig = - ConfiguredTargetFactory.getArtifactOwnerConfiguration(env, configuration); - if (env.valuesMissing()) { - return null; - } + BuildConfiguration ownerConfig = (configuration == null) + ? null : configuration.getArtifactOwnerConfiguration(); 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 77d8394679..b28b237eab 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,17 +1466,10 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { BuildConfiguration configuration, Attribute.Transition 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; - } + Preconditions.checkArgument(configuration == null + || configuration.useDynamicConfigurations() + || transition == ConfigurationTransition.NONE, + "Dynamic configurations required for test configuration using a transition"); if (memoizingEvaluator.getExistingValueForTesting( PrecomputedValue.WORKSPACE_STATUS_KEY.getKeyForTesting()) == null) { |