From 7f51c8b374f6f6c5b2185e50baf8bb9aa9131fd3 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 23 Nov 2022 15:40:28 -0800 Subject: [PATCH] Introduce NoConfigTransition for rules that don't need configs. - Provides an alternative for rules that use the host transition as a proxy for nonconfigurable targets. Host transition does that only as a side effect, and isn't the right tool for that purpose - Unblocks host config logic removal - Replace environment()'s host transition NoConfigTransition returns a mostly empty BuildOptions: it has CoreOptions but nothing else. Ideally it'd be completely empty. But too much core Bazel logic reads CoreOptions. We should remove those dependencies. But that's a larger task. See comments in PR diffs for details. This is very similar to NullTransition. This PR doesn't try to merge them because of deep Bazel invariants that NullTransition == source file. Perhaps that can be relaxed. But that's also a larger task. PiperOrigin-RevId: 490598476 Change-Id: Ic4dedc37659d7dc932d7bedc99db17a948566169 --- .../google/devtools/build/lib/analysis/BUILD | 20 +++- .../lib/analysis/DependencyResolver.java | 4 +- .../config/BuildConfigurationValue.java | 5 +- .../lib/analysis/config/BuildOptions.java | 17 ++++ .../transitions/NoConfigTransition.java | 99 +++++++++++++++++++ .../analysis/constraints/EnvironmentRule.java | 4 +- .../bazel/rules/BazelRuleClassProvider.java | 3 + .../rules/python/PythonVersionTransition.java | 8 +- .../build/lib/skyframe/AspectFunction.java | 2 +- .../skyframe/BuildConfigurationFunction.java | 4 +- .../skyframe/ConfiguredTargetFunction.java | 8 +- .../lib/skyframe/PlatformMappingValue.java | 5 + .../build/lib/skyframe/SkyframeExecutor.java | 4 +- .../google/devtools/build/lib/analysis/BUILD | 2 +- .../transitions/NoConfigTransitionTest.java | 99 +++++++++++++++++++ .../skyframe/PlatformMappingValueTest.java | 2 +- 16 files changed, 271 insertions(+), 15 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/analysis/config/transitions/NoConfigTransition.java create mode 100644 src/test/java/com/google/devtools/build/lib/analysis/config/transitions/NoConfigTransitionTest.java diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index 61362937cced96..380c0256c3aa12 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -373,7 +373,6 @@ java_library( ":transitive_info_provider_map", ":transitive_info_provider_map_builder", ":visibility_provider", - ":visibility_provider_impl", "//src/main/java/com/google/devtools/build/lib:build-request-options", "//src/main/java/com/google/devtools/build/lib:runtime/build_event_streamer_utils", "//src/main/java/com/google/devtools/build/lib/actions", @@ -2052,6 +2051,23 @@ java_library( ], ) +java_library( + name = "config/transitions/no_config_transition", + srcs = ["config/transitions/NoConfigTransition.java"], + deps = [ + ":config/build_options", + ":config/core_options", + ":config/fragment_options", + ":config/transitions/patch_transition", + ":config/transitions/transition_factory", + "//src/main/java/com/google/devtools/build/lib/events", + "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", + "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant", + "//third_party:auto_value", + "//third_party:guava", + ], +) + java_library( name = "config/transitions/patch_transition", srcs = ["config/transitions/PatchTransition.java"], @@ -2153,7 +2169,7 @@ java_library( srcs = ["constraints/EnvironmentRule.java"], deps = [ ":analysis_cluster", - ":config/host_transition", + ":config/transitions/no_config_transition", ":constraints/constraint_constants", ":constraints/environment", ":rule_definition_environment", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java index 45342a4d8acd5a..a0a45138500a73 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java @@ -620,7 +620,9 @@ public static Object resolveLateBoundDefault( Class fragmentClass = lateBoundDefault.getFragmentClass(); // TODO(b/65746853): remove this when nothing uses it anymore - if (BuildConfigurationValue.class.equals(fragmentClass)) { + if (BuildConfigurationValue.class.equals(fragmentClass) + // noconfig targets can't meaningfully parse late-bound defaults. See NoConfigTransition. + && !ruleConfig.getOptions().hasNoConfig()) { return lateBoundDefault.resolve(rule, attributeMap, fragmentClass.cast(ruleConfig)); } if (Void.class.equals(fragmentClass)) { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java index f2770c01f111a2..f537693c06e711 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java @@ -167,7 +167,10 @@ public static BuildConfigurationValue create( FragmentFactory fragmentFactory) throws InvalidConfigurationException { - FragmentClassSet fragmentClasses = globalProvider.getFragmentRegistry().getAllFragments(); + FragmentClassSet fragmentClasses = + buildOptions.hasNoConfig() + ? FragmentClassSet.of(ImmutableSet.of()) + : globalProvider.getFragmentRegistry().getAllFragments(); ImmutableSortedMap, Fragment> fragments = getConfigurationFragments(buildOptions, fragmentClasses, fragmentFactory); diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java index 6adf55da1d3c90..1575a07572b6fe 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java @@ -24,6 +24,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; +import com.google.common.collect.Iterables; import com.google.common.collect.MapDifference; import com.google.common.collect.Maps; import com.google.common.collect.Multimap; @@ -146,6 +147,22 @@ public boolean contains(Class optionsClass) { return fragmentOptionsMap.containsKey(optionsClass); } + /** + * Are these options "empty", meaning they contain no meaningful configuration information? + * + *

See {@link com.google.devtools.build.lib.analysis.config.transitions.NoConfigTransition}. + */ + public boolean hasNoConfig() { + // Ideally the implementation is fragmentOptionsMap.isEmpty() && starlarkOptionsMap.isEmpty(). + // See NoConfigTransition for why CoreOptions stays included. + return fragmentOptionsMap.size() == 1 + && Iterables.getOnlyElement(fragmentOptionsMap.values()) + .getClass() + .getSimpleName() + .equals("CoreOptions") + && starlarkOptionsMap.isEmpty(); + } + /** Returns a hex digest string uniquely identifying the build options. */ public String checksum() { if (checksum == null) { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/NoConfigTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/NoConfigTransition.java new file mode 100644 index 00000000000000..93a43448b993ae --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/NoConfigTransition.java @@ -0,0 +1,99 @@ +// Copyright 2022 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.analysis.config.transitions; + +import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.analysis.config.BuildOptions; +import com.google.devtools.build.lib.analysis.config.BuildOptionsView; +import com.google.devtools.build.lib.analysis.config.CoreOptions; +import com.google.devtools.build.lib.analysis.config.FragmentOptions; +import com.google.devtools.build.lib.events.EventHandler; +import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant; + +/** + * Transitions to a stable, empty configuration for rules that don't rely on configuration. + * + *

This prevents unnecessary configured target forking, which prevents unnecessary build graph + * bloat. That in turn reduces build time and memory use. + * + *

For example, imagine {@code cc_library //:foo} in config A depends on config-independent + * target {@code //:noconfig} and {@code cc_library //:bar} in config B also depends on {@code + * //:noconfig}. Without transitions, {@code //:noconfig} will be configured and analyzed twice: for + * configs A and B. This is completely wasteful if {@code //:noconfig} does the same thing + * regardless of configuration. Instead, apply this transition to {@code //:noconfig}. + * + *

This is safest for rules that don't produce actions and don't have dependencies. Remember that + * even if a rule doesn't read configuration, if any of its transitive dependencies read + * configuration or if the rule has a {@code select()}, its output may still be + * configuration-dependent. So use with careful discretion. + */ +public class NoConfigTransition implements PatchTransition { + + @SerializationConstant public static final NoConfigTransition INSTANCE = new NoConfigTransition(); + + private NoConfigTransition() {} + + @Override + public ImmutableSet> requiresOptionFragments() { + return ImmutableSet.of(CoreOptions.class); + } + + @Override + public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) { + // Ideally the build options should be empty: no fragment options and no flags. But core Bazel + // code assumes CoreOptions exists. For example CoreOptions.check_visibility is required for + // basic configured target graph evaluation. So we provide CoreOptions with default values + // (not inherited from parent configuration). This means flags like --check_visibility may not + // be consistently applied. If this becomes a problem in practice we can carve out exceptions + // to flags like that to propagate. + // TODO(bazel-team): break out flags that configure Bazel's analysis phase into their own + // FragmentOptions and propagate them to this configuration. Those flags should also be + // ineligible outputs for other transitions because they're not meant for rule logic. That + // would guarantee consistency of flags like --check_visibility while still preventing forking. + return BuildOptions.builder().addFragmentOptions(options.get(CoreOptions.class)).build(); + } + + /** Returns a {@link TransitionFactory} instance that generates the transition. */ + public static TransitionFactory createFactory() { + return new AutoValue_NoConfigTransition_Factory<>(); + } + + /** + * Returns {@code true} if the given {@link TransitionFactory} is an instance of this transition. + */ + public static boolean isInstance( + TransitionFactory instance) { + return instance instanceof NoConfigTransition.Factory; + } + + /** A {@link TransitionFactory} implementation that generates the transition. */ + @AutoValue + abstract static class Factory implements TransitionFactory { + @Override + public PatchTransition create(T unused) { + return INSTANCE; + } + + @Override + public boolean isHost() { + return true; + } + + @Override + public boolean isTool() { + return true; + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentRule.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentRule.java index 1f5993a3f36852..191b8a70ccc495 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentRule.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentRule.java @@ -20,7 +20,7 @@ import com.google.devtools.build.lib.analysis.BaseRuleClasses; import com.google.devtools.build.lib.analysis.RuleDefinition; import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment; -import com.google.devtools.build.lib.analysis.config.HostTransition; +import com.google.devtools.build.lib.analysis.config.transitions.NoConfigTransition; import com.google.devtools.build.lib.packages.BuildType; import com.google.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.packages.Type; @@ -36,7 +36,7 @@ public class EnvironmentRule implements RuleDefinition { @Override public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) { return builder - .cfg(HostTransition.createFactory()) + .cfg(NoConfigTransition.createFactory()) .override( attr("tags", Type.STRING_LIST) // No need to show up in ":all", etc. target patterns. diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java index 98fa115fc21468..9f2ef78615d065 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java @@ -214,6 +214,9 @@ static PathFragment getShellExecutableForOs(OS os, ShellConfiguration.Options op public static final Function SHELL_ACTION_ENV = (BuildOptions options) -> { + if (options.hasNoConfig()) { + return ActionEnvironment.EMPTY; + } boolean strictActionEnv = options.get(StrictActionEnvOptions.class).useStrictActionEnv; OS os = OS.getCurrent(); // TODO(ulfjack): instead of using the OS Bazel runs on, we need to use the exec platform, diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonVersionTransition.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonVersionTransition.java index 481009df4700a3..f980879ff20f13 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonVersionTransition.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonVersionTransition.java @@ -82,8 +82,12 @@ public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) { : determineNewVersion(options); checkArgument(newVersion.isTargetValue(), newVersion); - PythonOptions opts = options.get(PythonOptions.class); - if (!opts.canTransitionPythonVersion(newVersion)) { + // PythonOptions aren't present after NoConfigTransition. That implies rules that don't read + // configuration and don't produce build actions. The only time those rules trigger this code + // is in ExecutionTool.createConvenienceSymlinks. + PythonOptions opts = + options.underlying().hasNoConfig() ? null : options.get(PythonOptions.class); + if (opts == null || !opts.canTransitionPythonVersion(newVersion)) { return options.underlying(); } return cache.applyTransition(options, newVersion); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java index 497ec9f71df683..ebb8157b5b04b1 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java @@ -582,7 +582,7 @@ private static ComputedToolchainContexts getUnloadedToolchainContexts( return ConfiguredTargetFunction.computeUnloadedToolchainContexts( env, key.getLabel(), - true, + !configuration.getOptions().hasNoConfig(), Predicates.alwaysFalse(), configuration.getKey(), aspect.getDefinition().getToolchainTypes(), diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java index 731401d0f5e238..6a95da81ecdc5d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationFunction.java @@ -83,7 +83,9 @@ public SkyValue compute(SkyKey skyKey, Environment env) BuildOptions targetOptions = key.getOptions(); String transitionDirectoryNameFragment; - if (targetOptions + if (targetOptions.hasNoConfig()) { + transitionDirectoryNameFragment = "noconfig"; // See NoConfigTransition. + } else if (targetOptions .get(CoreOptions.class) .outputDirectoryNamingScheme .equals(CoreOptions.OutputDirectoryNamingScheme.DIFF_AGAINST_BASELINE)) { 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 7193f82b038d8a..1ae1996f843310 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 @@ -618,11 +618,12 @@ public static ComputedToolchainContexts computeUnloadedToolchainContexts( BuildConfigurationKey toolchainConfig = BuildConfigurationKey.withoutPlatformMapping(toolchainOptions); + PlatformConfiguration platformConfig = configuration.getFragment(PlatformConfiguration.class); return computeUnloadedToolchainContexts( env, label, - rule.useToolchainResolution(), - l -> configuration.getFragment(PlatformConfiguration.class).debugToolchainResolution(l), + platformConfig != null && rule.useToolchainResolution(), + l -> platformConfig != null && platformConfig.debugToolchainResolution(l), toolchainConfig, toolchainTypes, defaultExecConstraintLabels, @@ -739,6 +740,9 @@ static ComputedToolchainContexts computeUnloadedToolchainContexts( */ public static ImmutableSet