diff options
10 files changed, 148 insertions, 49 deletions
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 95912b4854..6376fd5821 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 @@ -738,14 +738,17 @@ public class BuildView { } } return configurations.useDynamicConfigurations() - ? trimConfigurations(nodes, eventHandler) + ? getDynamicConfigurations(nodes, eventHandler) : ImmutableList.copyOf(nodes); } /** - * Transforms a collection of <Target, Configuration> pairs by trimming each target's + * <p>If {@link BuildConfiguration.Options#trimConfigurations()} is true, transforms a collection + * of <Target, Configuration> pairs by trimming each target's * configuration to only the fragments the target and its transitive dependencies need. * + * <p>Else returns configurations that unconditionally include all fragments. + * * <p>Preserves the original input order. Uses original (untrimmed) configurations for targets * that can't be evaluated (e.g. due to loading phase errors). * @@ -756,8 +759,9 @@ public class BuildView { */ // TODO(bazel-team): error out early for targets that fail - untrimmed configurations should // never make it through analysis (and especially not seed ConfiguredTargetValues) - private List<TargetAndConfiguration> trimConfigurations(Iterable<TargetAndConfiguration> inputs, - EventHandler eventHandler) throws InterruptedException { + private List<TargetAndConfiguration> getDynamicConfigurations( + Iterable<TargetAndConfiguration> inputs, EventHandler eventHandler) + throws InterruptedException { Map<Label, TargetAndConfiguration> labelsToTargets = new LinkedHashMap<>(); BuildConfiguration topLevelConfig = null; List<Dependency> asDeps = new ArrayList<Dependency>(); @@ -805,12 +809,16 @@ public class BuildView { } /** - * Trims a configuration to the fragments needed by the given target. + * Gets a dynamic configuration for the given target. + * + * <p>If {@link BuildConfiguration.Options#trimConfigurations()} is true, the configuration only + * includes the fragments needed by the fragment and its transitive closure. Else unconditionally + * includes all fragments. */ @VisibleForTesting - public BuildConfiguration trimConfigurationForTesting(Target target, BuildConfiguration config, - EventHandler eventHandler) throws InterruptedException { - return Iterables.getOnlyElement(trimConfigurations( + public BuildConfiguration getDynamicConfigurationForTesting(Target target, + BuildConfiguration config, EventHandler eventHandler) throws InterruptedException { + return Iterables.getOnlyElement(getDynamicConfigurations( ImmutableList.<TargetAndConfiguration>of(new TargetAndConfiguration(target, config)), eventHandler)).getConfiguration(); } 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 90906b89db..036d6a5c0d 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 @@ -860,12 +860,34 @@ public final class BuildConfiguration { ) public List<Label> targetEnvironments; + /** + * Values for --experimental_dynamic_configs. + */ + public static enum DynamicConfigsMode { + /** Don't use dynamic configurations. */ + OFF, + /** Use dynamic configurations, including only the fragments each rule needs. */ + ON, + /** Use dynamic configurations, always including all fragments known to Blaze. */ + NOTRIM, + } + + /** + * Converter for --experimental_dynamic_configs. + */ + public static class DynamicConfigsConverter extends EnumConverter<DynamicConfigsMode> { + public DynamicConfigsConverter() { + super(DynamicConfigsMode.class, "dynamic configurations mode"); + } + } + @Option(name = "experimental_dynamic_configs", - defaultValue = "false", + defaultValue = "off", category = "undocumented", + converter = DynamicConfigsConverter.class, help = "Dynamically instantiates build configurations instead of using the default " + "static globally defined ones") - public boolean useDynamicConfigurations; + public DynamicConfigsMode useDynamicConfigurations; @Option( name = "experimental_enable_runfiles", @@ -1180,7 +1202,7 @@ public final class BuildConfiguration { + ".blazerc or continuous build")); } - if (options.useDynamicConfigurations && !options.useDistinctHostConfiguration) { + if (useDynamicConfigurations() && !options.useDistinctHostConfiguration) { reporter.handle(Event.error( "--nodistinct_host_configuration does not currently work with dynamic configurations")); } @@ -2283,7 +2305,15 @@ public final class BuildConfiguration { * {@link com.google.devtools.build.lib.analysis.ConfigurationCollectionFactory}). */ public boolean useDynamicConfigurations() { - return options.useDynamicConfigurations; + return options.useDynamicConfigurations != Options.DynamicConfigsMode.OFF; + } + + /** + * Returns whether we should trim dynamic configurations to only include the fragments needed + * to correctly analyze a rule. + */ + public boolean trimConfigurations() { + return options.useDynamicConfigurations == Options.DynamicConfigsMode.ON; } /** 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 6a62afdce4..8f369680b2 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,6 +75,7 @@ public class CppConfigurationLoader implements ConfigurationFragmentFactory { } CppConfiguration cppConfig = new CppConfiguration(params); if (options.get(BuildConfiguration.Options.class).useDynamicConfigurations + != BuildConfiguration.Options.DynamicConfigsMode.OFF && cppConfig.getLipoMode() != CrosstoolConfig.LipoMode.OFF) { throw new InvalidConfigurationException( "LIPO does not currently work with dynamic configurations"); 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 0bd188a4f0..48776897bf 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 @@ -301,7 +301,7 @@ final class ConfiguredTargetFunction implements SkyFunction { // Trim each dep's configuration so it only includes the fragments needed by its transitive // closure (only dynamic configurations support this). if (useDynamicConfigurations(ctgValue.getConfiguration())) { - depValueNames = trimConfigurations(env, ctgValue, depValueNames, hostConfiguration, + depValueNames = getDynamicConfigurations(env, ctgValue, depValueNames, hostConfiguration, ruleClassProvider); if (depValueNames == null) { return null; @@ -327,7 +327,7 @@ final class ConfiguredTargetFunction implements SkyFunction { } /** - * Helper class for {@link #trimConfigurations} - encapsulates a set of config fragments and + * Helper class for {@link #getDynamicConfigurations} - encapsulates a set of config fragments and * a dynamic transition. This can be used to determine the exact build options needed to * set a dynamic configuration. */ @@ -365,8 +365,8 @@ final class ConfiguredTargetFunction implements SkyFunction { } /** - * Helper class for {@link #trimConfigurations} - encapsulates an <attribute, label> pair that - * can be used to map from an input dependency to a trimmed dependency. + * Helper class for {@link #getDynamicConfigurations} - encapsulates an <attribute, label> pair + * that can be used to map from an input dependency to a trimmed dependency. */ @Immutable private static final class AttributeAndLabel { @@ -409,9 +409,10 @@ final class ConfiguredTargetFunction implements SkyFunction { * * <p>More specifically: given a set of {@link Dependency} instances holding dynamic config * transition requests (e.g. {@link Dependency#hasStaticConfiguration()} == false}), returns - * equivalent dependencies containing dynamically created configurations that a) apply those - * transitions and b) only contain the fragments needed by the dep and everything in its - * transitive closure. + * equivalent dependencies containing dynamically created configurations applying those + * transitions. If {@link BuildConfiguration.Options#trimConfigurations()} is true, these + * configurations only contain the fragments needed by the dep and its transitive closure. Else + * the configurations unconditionally include all fragments. * * <p>This method is heavily performance-optimized. Because it, in aggregate, reads over every * edge in the configured target graph, small inefficiencies can have observable impact on build @@ -419,7 +420,7 @@ final class ConfiguredTargetFunction implements SkyFunction { * make. */ @Nullable - static OrderedSetMultimap<Attribute, Dependency> trimConfigurations( + static OrderedSetMultimap<Attribute, Dependency> getDynamicConfigurations( Environment env, TargetAndConfiguration ctgValue, OrderedSetMultimap<Attribute, Dependency> originalDeps, @@ -470,16 +471,11 @@ final class ConfiguredTargetFunction implements SkyFunction { } // Figure out the required fragments for this dep and its transitive closure. - SkyKey fragmentsKey = TransitiveTargetValue.key(dep.getLabel()); - TransitiveTargetValue transitiveDepInfo = (TransitiveTargetValue) env.getValue(fragmentsKey); - if (transitiveDepInfo == null) { - // This should only be possible for tests. In actual runs, this was already called - // as a routine part of the loading phase. - // TODO(bazel-team): check this only occurs in a test context. + Set<Class<? extends BuildConfiguration.Fragment>> depFragments = + getTransitiveFragments(env, dep.getLabel(), ctgValue.getConfiguration()); + if (depFragments == null) { return null; } - Set<Class<? extends BuildConfiguration.Fragment>> depFragments = - transitiveDepInfo.getTransitiveConfigFragments().toSet(); // TODO(gregce): remove the below call once we have confidence dynamic configurations always // provide needed fragments. This unnecessarily drags performance on the critical path. checkForMissingFragments(env, ctgValue, attributeAndLabel.attribute.getName(), dep, @@ -519,8 +515,8 @@ final class ConfiguredTargetFunction implements SkyFunction { ImmutableList.Builder<BuildOptions> toOptionsBuilder = ImmutableList.builder(); for (BuildOptions options : getDynamicTransitionOptions(ctgOptions, transition)) { if (!sameFragments) { - options = options.trim(BuildConfiguration.getOptionsClasses( - transitiveDepInfo.getTransitiveConfigFragments(), ruleClassProvider)); + options = options.trim( + BuildConfiguration.getOptionsClasses(depFragments, ruleClassProvider)); } toOptionsBuilder.add(options); } @@ -588,6 +584,32 @@ final class ConfiguredTargetFunction implements SkyFunction { } /** + * Returns the configuration fragments required by a dep and its transitive closure. + * Returns null if Skyframe dependencies aren't yet available. + * + * @param env Skyframe evaluation environment + * @param dep label of the dep to check + * @param parentConfig configuration of the rule depending on the dep + */ + @Nullable + private static Set<Class<? extends BuildConfiguration.Fragment>> getTransitiveFragments( + Environment env, Label dep, BuildConfiguration parentConfig) throws InterruptedException { + Preconditions.checkArgument(parentConfig.useDynamicConfigurations()); + if (!parentConfig.trimConfigurations()) { + return parentConfig.getAllFragments().keySet(); + } + SkyKey fragmentsKey = TransitiveTargetValue.key(dep); + TransitiveTargetValue transitiveDepInfo = (TransitiveTargetValue) env.getValue(fragmentsKey); + if (transitiveDepInfo == null) { + // This should only be possible for tests. In actual runs, this was already called + // as a routine part of the loading phase. + // TODO(bazel-team): check this only occurs in a test context. + return null; + } + return transitiveDepInfo.getTransitiveConfigFragments().toSet(); + } + + /** * Applies a dynamic configuration transition over a set of build options. * * @return the build options for the transitioned configuration. Contains the same fragment diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java index 08eecd7828..bd5eef5c0d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java @@ -108,8 +108,8 @@ public class PostConfiguredTargetFunction implements SkyFunction { resolver.dependentNodeMap( ctgValue, hostConfiguration, /*aspect=*/ null, configConditions); if (ct.getConfiguration() != null && ct.getConfiguration().useDynamicConfigurations()) { - deps = ConfiguredTargetFunction.trimConfigurations(env, ctgValue, deps, hostConfiguration, - ruleClassProvider); + deps = ConfiguredTargetFunction.getDynamicConfigurations(env, ctgValue, deps, + hostConfiguration, ruleClassProvider); } } catch (EvalException e) { throw new PostConfiguredTargetFunctionException(e); 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 142dbc3252..3bc3a2a340 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 @@ -1235,8 +1235,9 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { } /** - * Retrieves the configurations needed for the given deps, trimming down their fragments - * to those only needed by their transitive closures. + * Retrieves the configurations needed for the given deps. If + * {@link BuildConfiguration.Options#trimConfigurations()} is true, trims their fragments to only + * those needed by their transitive closures. Else unconditionally includes all fragments. * * <p>Skips targets with loading phase errors. */ @@ -1246,6 +1247,10 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { Set<Dependency> depsToEvaluate = new HashSet<>(); // Check: if !Configuration.useDynamicConfigs then just return the original configs. + Set<Class<? extends BuildConfiguration.Fragment>> allFragments = null; + if (useUntrimmedDynamicConfigs(fromOptions)) { + allFragments = getAllFragments(); + } // Get the fragments needed for dynamic configuration nodes. final List<SkyKey> transitiveFragmentSkyKeys = new ArrayList<>(); @@ -1256,6 +1261,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { builder.put(key, key.getConfiguration()); } else if (key.getTransition() == Attribute.ConfigurationTransition.NULL) { builder.put(key, null); + } else if (useUntrimmedDynamicConfigs(fromOptions)) { + fragmentsMap.put(key.getLabel(), allFragments); } else { depsToEvaluate.add(key); transitiveFragmentSkyKeys.add(TransitiveTargetValue.key(key.getLabel())); @@ -1281,31 +1288,59 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { // Now get the configurations. final List<SkyKey> configSkyKeys = new ArrayList<>(); for (Dependency key : keys) { - if (!depsToEvaluate.contains(key) || labelsWithErrors.contains(key.getLabel())) { + if (labelsWithErrors.contains(key.getLabel())) { continue; } Set<Class<? extends BuildConfiguration.Fragment>> depFragments = fragmentsMap.get(key.getLabel()); - configSkyKeys.add(BuildConfigurationValue.key(depFragments, - getDynamicConfigOptions(key, fromOptions, depFragments))); + if (depFragments != null) { + configSkyKeys.add(BuildConfigurationValue.key(depFragments, + getDynamicConfigOptions(key, fromOptions, depFragments))); + } } EvaluationResult<SkyValue> configsResult = evaluateSkyKeys(eventHandler, configSkyKeys, /*keepGoing=*/true); for (Dependency key : keys) { - if (!depsToEvaluate.contains(key) || labelsWithErrors.contains(key.getLabel())) { + if (labelsWithErrors.contains(key.getLabel())) { continue; } Set<Class<? extends BuildConfiguration.Fragment>> depFragments = fragmentsMap.get(key.getLabel()); - SkyKey configKey = BuildConfigurationValue.key(depFragments, - getDynamicConfigOptions(key, fromOptions, depFragments)); - builder.put(key, ((BuildConfigurationValue) configsResult.get(configKey)).getConfiguration()); + if (depFragments != null) { + SkyKey configKey = BuildConfigurationValue.key(depFragments, + getDynamicConfigOptions(key, fromOptions, depFragments)); + builder + .put(key, ((BuildConfigurationValue) configsResult.get(configKey)).getConfiguration()); + } } return builder; } /** + * Returns whether dynamic configurations should trim their fragments to only those needed by + * targets and their transitive dependencies. + */ + private static boolean useUntrimmedDynamicConfigs(BuildOptions options) { + return options.get(BuildConfiguration.Options.class).useDynamicConfigurations + == BuildConfiguration.Options.DynamicConfigsMode.NOTRIM; + } + + /** + * Returns all configuration fragments registered with Blaze. + */ + private Set<Class<? extends BuildConfiguration.Fragment>> getAllFragments() { + ImmutableSet.Builder<Class<? extends BuildConfiguration.Fragment>> fragmentsBuilder = + ImmutableSet.builder(); + for (ConfigurationFragmentFactory factory : + ((ConfiguredRuleClassProvider) ruleClassProvider).getConfigurationFragments()) { + fragmentsBuilder.add(factory.creates()); + } + fragmentsBuilder.add(((ConfiguredRuleClassProvider) ruleClassProvider).getUniversalFragment()); + return fragmentsBuilder.build(); + } + + /** * Computes the build options needed for the given key, accounting for transitions possibly * specified in the key. */ diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java index 98455b7e13..73642c5fe4 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java @@ -1182,7 +1182,7 @@ public class BuildViewTest extends BuildViewTestBase { scratch.file("foo/BUILD", "sh_library(name='x', ", " srcs=['x.sh'])"); - useConfiguration("--experimental_dynamic_configs"); + useConfiguration("--experimental_dynamic_configs=on"); AnalysisResult res = update("//foo:x"); ConfiguredTarget topLevelTarget = Iterables.getOnlyElement(res.getTargetsToBuild()); assertThat(topLevelTarget.getConfiguration().getAllFragments().keySet()).containsExactly( @@ -1199,7 +1199,7 @@ public class BuildViewTest extends BuildViewTestBase { "java_library(", " name = 'javalib',", " srcs = ['javalib.java'])"); - useConfiguration("--experimental_dynamic_configs", "--experimental_disable_jvm"); + useConfiguration("--experimental_dynamic_configs=on", "--experimental_disable_jvm"); reporter.removeHandler(failFastHandler); try { update("//foo:ccbin"); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java index 8f9e44311b..b1bdd87e3d 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationTest.java @@ -298,9 +298,12 @@ public class BuildConfigurationTest extends ConfigurationTestCase { @Test public void testNoDistinctHostConfigurationUnsupportedWithDynamicConfigs() throws Exception { - checkError( - "--nodistinct_host_configuration does not currently work with dynamic configurations", - "--nodistinct_host_configuration", "--experimental_dynamic_configs"); + String expectedError = + "--nodistinct_host_configuration does not currently work with dynamic configurations"; + checkError(expectedError, + "--nodistinct_host_configuration", "--experimental_dynamic_configs=on"); + checkError(expectedError, + "--nodistinct_host_configuration", "--experimental_dynamic_configs=notrim"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java index 2ea8887392..a91c4000cd 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java @@ -218,7 +218,7 @@ public abstract class AnalysisTestCase extends FoundationTestCase { optionsParser.parse(new String[] {"--default_visibility=public" }); optionsParser.parse(args); if (defaultFlags().contains(Flag.DYNAMIC_CONFIGURATIONS)) { - optionsParser.parse("--experimental_dynamic_configs"); + optionsParser.parse("--experimental_dynamic_configs=on"); } InvocationPolicyEnforcer optionsPolicyEnforcer = analysisMock.getInvocationPolicyEnforcer(); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java index dc540061ab..1f76af4fc0 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java @@ -391,7 +391,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { String[] actualArgs; if (useDynamicConfigs) { actualArgs = Arrays.copyOf(args, args.length + 1); - actualArgs[args.length] = "--experimental_dynamic_configs"; + actualArgs[args.length] = "--experimental_dynamic_configs=on"; } else { actualArgs = args; } @@ -1282,7 +1282,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase { BuildConfiguration config = targetConfig; if (targetConfig.useDynamicConfigurations()) { try { - config = view.trimConfigurationForTesting(getTarget(label), targetConfig, reporter); + config = view.getDynamicConfigurationForTesting(getTarget(label), targetConfig, reporter); } catch (Exception e) { throw new RuntimeException(e); } |