From 9eb1cf05fa1db8d7052db366d9d8c93637bfd77a Mon Sep 17 00:00:00 2001 From: Greg Estren Date: Fri, 26 Jun 2015 22:18:35 +0000 Subject: Refactor HOST configuration transitions to be dynamic-configuration friendly. Dynamic configuration transitions require access to Skyframe (since they instantiate BuildConfigurations as Skyframe nodes). There are various places in Bazel where static transitions are done with no convenient Skyframe access. This cl shuffles host transitions, in particular, to places that are more amenable. This change also assumes one host configuration per invocation. While this isn't strictly true (each target configuration can have its own host, and multiple target configurations are possible per build), we don't leverage that functionality in any meaningful way today. So until we have a proper interface for multiple host configurations, let's not block dynamic config progress on it. -- MOS_MIGRATED_REVID=97008479 --- .../devtools/build/lib/analysis/BuildView.java | 11 ++++++++-- .../lib/analysis/ConfiguredTargetFactory.java | 15 ++++++++++---- .../devtools/build/lib/analysis/RuleContext.java | 9 +++++--- .../config/BuildConfigurationCollection.java | 24 ++++++++++++++-------- .../skyframe/ConfigurationCollectionFunction.java | 15 +++++++++++++- .../lib/skyframe/ConfiguredTargetFunction.java | 14 ++++++++----- .../build/lib/skyframe/SkyframeBuildView.java | 7 +++---- 7 files changed, 68 insertions(+), 27 deletions(-) (limited to 'src') diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java index 07f54837b6..0869785195 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java @@ -897,7 +897,8 @@ public class BuildView { /*isSystemEnv=*/false, config.extendedSanityChecks(), eventHandler, /*skyframeEnv=*/null, config.isActionsEnabled(), binTools); return new RuleContext.Builder(analysisEnvironment, - (Rule) target.getTarget(), config, ruleClassProvider.getPrerequisiteValidator()) + (Rule) target.getTarget(), config, getHostConfigurationForTesting(config), + ruleClassProvider.getPrerequisiteValidator()) .setVisibility(NestedSetBuilder.create( Order.STABLE_ORDER, PackageSpecification.EVERYTHING)) .setPrerequisites(getPrerequisiteMapForTesting(target)) @@ -911,8 +912,9 @@ public class BuildView { */ @VisibleForTesting public RuleContext getRuleContextForTesting(ConfiguredTarget target, AnalysisEnvironment env) { + BuildConfiguration targetConfig = target.getConfiguration(); return new RuleContext.Builder( - env, (Rule) target.getTarget(), target.getConfiguration(), + env, (Rule) target.getTarget(), targetConfig, getHostConfigurationForTesting(targetConfig), ruleClassProvider.getPrerequisiteValidator()) .setVisibility(NestedSetBuilder.create( Order.STABLE_ORDER, PackageSpecification.EVERYTHING)) @@ -921,6 +923,11 @@ public class BuildView { .build(); } + private BuildConfiguration getHostConfigurationForTesting(BuildConfiguration config) { + // TODO(bazel-team): support dynamic transitions in tests. + return config == null ? null : config.getConfiguration(Attribute.ConfigurationTransition.HOST); + } + /** * For a configured target dependentTarget, returns the desired configured target * that is depended upon. Useful for obtaining the a target with aspects 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 92687d6e65..c5d7a671f2 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 @@ -156,12 +156,12 @@ public final class ConfiguredTargetFactory { @Nullable public final ConfiguredTarget createConfiguredTarget(AnalysisEnvironment analysisEnvironment, ArtifactFactory artifactFactory, Target target, BuildConfiguration config, - ListMultimap prerequisiteMap, + BuildConfiguration hostConfig, ListMultimap prerequisiteMap, Set configConditions) throws InterruptedException { if (target instanceof Rule) { - return createRule( - analysisEnvironment, (Rule) target, config, prerequisiteMap, configConditions); + return createRule(analysisEnvironment, (Rule) target, config, hostConfig, + prerequisiteMap, configConditions); } // Visibility, like all package groups, doesn't have a configuration @@ -203,10 +203,11 @@ public final class ConfiguredTargetFactory { @Nullable private ConfiguredTarget createRule( AnalysisEnvironment env, Rule rule, BuildConfiguration configuration, + BuildConfiguration hostConfiguration, ListMultimap prerequisiteMap, Set configConditions) throws InterruptedException { // Visibility computation and checking is done for every rule. - RuleContext ruleContext = new RuleContext.Builder(env, rule, configuration, + RuleContext ruleContext = new RuleContext.Builder(env, rule, configuration, hostConfiguration, ruleClassProvider.getPrerequisiteValidator()) .setVisibility(convertVisibility(prerequisiteMap, env.getEventHandler(), rule, null)) .setPrerequisites(prerequisiteMap) @@ -272,6 +273,7 @@ public final class ConfiguredTargetFactory { RuleContext.Builder builder = new RuleContext.Builder(env, associatedTarget.getTarget(), associatedTarget.getConfiguration(), + getHostConfiguration(associatedTarget.getConfiguration()), ruleClassProvider.getPrerequisiteValidator()); RuleContext ruleContext = builder .setVisibility(convertVisibility( @@ -286,6 +288,11 @@ public final class ConfiguredTargetFactory { return aspectFactory.create(associatedTarget, ruleContext); } + private static BuildConfiguration getHostConfiguration(BuildConfiguration config) { + // TODO(bazel-team): support dynamic transitions. + return config == null ? null : config.getConfiguration(Attribute.ConfigurationTransition.HOST); + } + /** * A pseudo-implementation for configured targets that creates fail actions for all declared * outputs, both implicit and explicit. 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 932bc2b672..b81ea289b5 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 @@ -133,6 +133,7 @@ public final class RuleContext extends TargetContext private final AttributeMap attributes; private final ImmutableSet features; private final Map attributeMap; + private final BuildConfiguration hostConfiguration; private ActionOwner actionOwner; @@ -152,6 +153,7 @@ public final class RuleContext extends TargetContext ConfiguredAttributeMapper.of(builder.rule, configConditions); this.features = getEnabledFeatures(); this.attributeMap = attributeMap; + this.hostConfiguration = builder.hostConfiguration; } private ImmutableSet getEnabledFeatures() { @@ -211,9 +213,7 @@ public final class RuleContext extends TargetContext * host configurations, even during a single build. */ public BuildConfiguration getHostConfiguration() { - BuildConfiguration configuration = getConfiguration(); - // Note: the Builder checks that the configuration is non-null. - return configuration.getConfiguration(ConfigurationTransition.HOST); + return hostConfiguration; } /** @@ -1131,16 +1131,19 @@ public final class RuleContext extends TargetContext private final AnalysisEnvironment env; private final Rule rule; private final BuildConfiguration configuration; + private final BuildConfiguration hostConfiguration; private final PrerequisiteValidator prerequisiteValidator; private ListMultimap prerequisiteMap; private Set configConditions; private NestedSet visibility; Builder(AnalysisEnvironment env, Rule rule, BuildConfiguration configuration, + BuildConfiguration hostConfiguration, PrerequisiteValidator prerequisiteValidator) { this.env = Preconditions.checkNotNull(env); this.rule = Preconditions.checkNotNull(rule); this.configuration = Preconditions.checkNotNull(configuration); + this.hostConfiguration = Preconditions.checkNotNull(hostConfiguration); this.prerequisiteValidator = prerequisiteValidator; } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationCollection.java index e36a6817bc..41c169337d 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationCollection.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationCollection.java @@ -54,10 +54,13 @@ import java.util.Set; @ThreadSafe public final class BuildConfigurationCollection implements Serializable { private final ImmutableList targetConfigurations; + private final BuildConfiguration hostConfiguration; - public BuildConfigurationCollection(List targetConfigurations) + public BuildConfigurationCollection(List targetConfigurations, + BuildConfiguration hostConfiguration) throws InvalidConfigurationException { this.targetConfigurations = ImmutableList.copyOf(targetConfigurations); + this.hostConfiguration = hostConfiguration; // Except for the host configuration (which may be identical across target configs), the other // configurations must all have different cache keys or we will end up with problems. @@ -71,13 +74,6 @@ public final class BuildConfigurationCollection implements Serializable { } } - /** - * Creates an empty configuration collection which will return null for everything. - */ - public BuildConfigurationCollection() { - this.targetConfigurations = ImmutableList.of(); - } - public static BuildConfiguration configureTopLevelTarget(BuildConfiguration topLevelConfiguration, Target toTarget) { if (toTarget instanceof InputFile || toTarget instanceof PackageGroup) { @@ -90,6 +86,18 @@ public final class BuildConfigurationCollection implements Serializable { return targetConfigurations; } + /** + * Returns the host configuration for this collection. + * + *

Don't use this method. It's limited in that it assumes a single host configuration for + * the entire collection. This may not be true in the future and more flexible interfaces based + * on dynamic configurations will likely supplant this interface anyway. Its main utility is + * to keep Bazel working while dynamic configuration progress is under way. + */ + public BuildConfiguration getHostConfiguration() { + return hostConfiguration; + } + /** * Returns all configurations that can be reached from the target configuration through any kind * of configuration transition. 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 b00f2c8cb8..7d2efe8214 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 @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.skyframe; import com.google.common.base.Supplier; +import com.google.common.base.Verify; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection; @@ -23,6 +24,7 @@ import com.google.devtools.build.lib.analysis.config.InvalidConfigurationExcepti import com.google.devtools.build.lib.analysis.config.PackageProviderForConfigurations; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.events.StoredEventHandler; +import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition; import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.skyframe.ConfigurationCollectionValue.ConfigurationCollectionKey; import com.google.devtools.build.skyframe.SkyFunction; @@ -105,7 +107,18 @@ public class ConfigurationCollectionFunction implements SkyFunction { } targetConfigurations.add(targetConfiguration); } - return new BuildConfigurationCollection(targetConfigurations); + + // Sanity check that all host configs are the same. This may not be true once we have + // better support for multi-host builds. + BuildConfiguration hostConfiguration = + targetConfigurations.get(0).getConfiguration(ConfigurationTransition.HOST); + for (BuildConfiguration targetConfig : + targetConfigurations.subList(1, targetConfigurations.size())) { + Verify.verify( + targetConfig.getConfiguration(ConfigurationTransition.HOST).equals(hostConfiguration)); + } + + return new BuildConfigurationCollection(targetConfigurations, hostConfiguration); } @Nullable 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 68fb4b8638..dff91d6976 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 @@ -170,8 +170,13 @@ final class ConfiguredTargetFunction implements SkyFunction { ListMultimap depValueMap = computeDependencies(env, resolver, ctgValue, null, configConditions); + + // TODO(bazel-team): Support dynamically created host configurations. + BuildConfiguration hostConfiguration = configuration == null + ? null : configuration.getConfiguration(Attribute.ConfigurationTransition.HOST); + return createConfiguredTarget( - view, env, target, configuration, depValueMap, configConditions); + view, env, target, configuration, hostConfiguration, depValueMap, configConditions); } catch (DependencyEvaluationException e) { throw new ConfiguredTargetFunctionException(e.getRootCauseSkyKey(), e.getCause()); } @@ -465,10 +470,9 @@ final class ConfiguredTargetFunction implements SkyFunction { @Nullable private ConfiguredTargetValue createConfiguredTarget(SkyframeBuildView view, Environment env, Target target, BuildConfiguration configuration, - ListMultimap depValueMap, + BuildConfiguration hostConfiguration, ListMultimap depValueMap, Set configConditions) - throws ConfiguredTargetFunctionException, - InterruptedException { + throws ConfiguredTargetFunctionException, InterruptedException { StoredEventHandler events = new StoredEventHandler(); BuildConfiguration ownerConfig = (configuration == null) ? null : configuration.getArtifactOwnerConfiguration(); @@ -480,7 +484,7 @@ final class ConfiguredTargetFunction implements SkyFunction { } ConfiguredTarget configuredTarget = view.createConfiguredTarget(target, configuration, - analysisEnvironment, depValueMap, configConditions); + hostConfiguration, analysisEnvironment, depValueMap, configConditions); events.replayOn(env.getListener()); if (events.hasErrors()) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java index d094eba092..fbae0e5438 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java @@ -369,14 +369,13 @@ public final class SkyframeBuildView { */ @Nullable ConfiguredTarget createConfiguredTarget(Target target, BuildConfiguration configuration, - CachingAnalysisEnvironment analysisEnvironment, + BuildConfiguration hostConfiguration, CachingAnalysisEnvironment analysisEnvironment, ListMultimap prerequisiteMap, - Set configConditions) - throws InterruptedException { + Set configConditions) throws InterruptedException { Preconditions.checkState(enableAnalysis, "Already in execution phase %s %s", target, configuration); return factory.createConfiguredTarget(analysisEnvironment, artifactFactory, target, - configuration, prerequisiteMap, configConditions); + configuration, hostConfiguration, prerequisiteMap, configConditions); } @Nullable -- cgit v1.2.3