Skip to content

Commit

Permalink
Introduce NoConfigTransition for rules that don't need configs.
Browse files Browse the repository at this point in the history
 - 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
  • Loading branch information
gregestren authored and copybara-github committed Nov 23, 2022
1 parent 768cf8e commit 7f51c8b
Show file tree
Hide file tree
Showing 16 changed files with 271 additions and 15 deletions.
20 changes: 18 additions & 2 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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"],
Expand Down Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -620,7 +620,9 @@ public static <FragmentT> Object resolveLateBoundDefault(

Class<FragmentT> 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)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<Class<? extends Fragment>, Fragment> fragments =
getConfigurationFragments(buildOptions, fragmentClasses, fragmentFactory);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -146,6 +147,22 @@ public boolean contains(Class<? extends FragmentOptions> optionsClass) {
return fragmentOptionsMap.containsKey(optionsClass);
}

/**
* Are these options "empty", meaning they contain no meaningful configuration information?
*
* <p>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) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
*
* <p>This prevents unnecessary configured target forking, which prevents unnecessary build graph
* bloat. That in turn reduces build time and memory use.
*
* <p>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}.
*
* <p>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<Class<? extends FragmentOptions>> 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 <T extends TransitionFactory.Data> TransitionFactory<T> createFactory() {
return new AutoValue_NoConfigTransition_Factory<>();
}

/**
* Returns {@code true} if the given {@link TransitionFactory} is an instance of this transition.
*/
public static <T extends TransitionFactory.Data> boolean isInstance(
TransitionFactory<T> instance) {
return instance instanceof NoConfigTransition.Factory;
}

/** A {@link TransitionFactory} implementation that generates the transition. */
@AutoValue
abstract static class Factory<T extends TransitionFactory.Data> implements TransitionFactory<T> {
@Override
public PatchTransition create(T unused) {
return INSTANCE;
}

@Override
public boolean isHost() {
return true;
}

@Override
public boolean isTool() {
return true;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,9 @@ static PathFragment getShellExecutableForOs(OS os, ShellConfiguration.Options op

public static final Function<BuildOptions, ActionEnvironment> 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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -739,6 +740,9 @@ static ComputedToolchainContexts computeUnloadedToolchainContexts(
*/
public static ImmutableSet<Label> getExecutionPlatformConstraints(
Rule rule, PlatformConfiguration platformConfiguration) {
if (platformConfiguration == null) {
return ImmutableSet.of(); // See NoConfigTransition.
}
NonconfigurableAttributeMapper mapper = NonconfigurableAttributeMapper.of(rule);
ImmutableSet.Builder<Label> execConstraintLabels = new ImmutableSet.Builder<>();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,11 @@ private BuildConfigurationKey computeMapping(BuildConfigurationKey original)
throws OptionsParsingException {
BuildOptions originalOptions = original.getOptions();

if (originalOptions.hasNoConfig()) {
// The empty configuration (produced by NoConfigTransition) is terminal: it'll never change.
return original;
}

checkArgument(
originalOptions.contains(PlatformOptions.class),
"When using platform mappings, all configurations must contain platform options");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2042,7 +2042,9 @@ private PlatformMappingValue getPlatformMappingValue(
ExtendedEventHandler eventHandler, BuildOptions referenceBuildOptions)
throws InvalidConfigurationException {
PathFragment platformMappingPath =
referenceBuildOptions.get(PlatformOptions.class).platformMappings;
referenceBuildOptions.hasNoConfig()
? null
: referenceBuildOptions.get(PlatformOptions.class).platformMappings;

PlatformMappingValue.Key platformMappingKey =
PlatformMappingValue.Key.create(platformMappingPath);
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_options_cache",
"//src/main/java/com/google/devtools/build/lib/analysis:config/compilation_mode",
"//src/main/java/com/google/devtools/build/lib/analysis:config/config_aware_aspect_builder",
"//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/execution_info_modifier",
"//src/main/java/com/google/devtools/build/lib/analysis:config/execution_transition_factory",
Expand All @@ -89,6 +88,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/composing_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/composing_transition_factory",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/configuration_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/no_config_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/no_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/null_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/patch_transition",
Expand Down
Loading

0 comments on commit 7f51c8b

Please sign in to comment.