From 062657062575e2c6a22c3190ddef5957be5eb8e1 Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 29 Sep 2023 13:34:39 -0700 Subject: [PATCH] Clean up uses of an "empty" BuildOptions. Work towards platform-based flags: #19409. PiperOrigin-RevId: 569576336 Change-Id: I7c7f37c833987aed83b04aaddac234ef3b17de2e --- .../google/devtools/build/lib/analysis/BUILD | 12 +++++++ .../devtools/build/lib/analysis/Util.java | 9 +++-- .../lib/analysis/config/CommonOptions.java | 36 +++++++++++++++++++ .../transitions/NoConfigTransition.java | 17 ++------- .../lib/skyframe/BuildConfigurationKey.java | 4 +-- 5 files changed, 56 insertions(+), 22 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/analysis/config/CommonOptions.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 398405bf845e79..1277cad96ea7e0 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -283,6 +283,7 @@ java_library( ":config/build_configuration", ":config/build_option_details", ":config/build_options", + ":config/common_options", ":config/config_conditions", ":config/config_matching_provider", ":config/core_options", @@ -1706,6 +1707,16 @@ java_library( ], ) +java_library( + name = "config/common_options", + srcs = ["config/CommonOptions.java"], + deps = [ + ":config/build_options", + ":config/core_options", + "//src/main/java/com/google/devtools/common/options", + ], +) + java_library( name = "config/compilation_mode", srcs = ["config/CompilationMode.java"], @@ -2127,6 +2138,7 @@ java_library( srcs = ["config/transitions/NoConfigTransition.java"], deps = [ ":config/build_options", + ":config/common_options", ":config/core_options", ":config/fragment_options", ":config/transitions/patch_transition", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/Util.java b/src/main/java/com/google/devtools/build/lib/analysis/Util.java index ffea8a32016710..fc95b4b9bdbf3e 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/Util.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/Util.java @@ -17,10 +17,12 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; +import com.google.devtools.build.lib.analysis.config.CommonOptions; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.compacthashset.CompactHashSet; import com.google.devtools.build.lib.packages.AttributeMap; import com.google.devtools.build.lib.packages.Target; +import com.google.devtools.build.lib.skyframe.BuildConfigurationKey; import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey; import com.google.devtools.build.lib.vfs.PathFragment; @@ -103,11 +105,8 @@ public static ImmutableSet findImplicitDeps(RuleContext rul maybeImplicitDeps.add( ConfiguredTargetKey.builder() .setLabel(platformConfiguration.getTargetPlatform()) - // This is technically the wrong configuration, because PlatformRule uses - // NoConfigTransition to reduce configured target fanout. However, it still works, - // because PostAnalysisQueryEnvironment also guesses using the wrong configuration, - // so the target platform dependency is correctly marked as implicit anyway. - .setConfiguration(ruleContext.getConfiguration()) + .setConfigurationKey( + BuildConfigurationKey.withoutPlatformMapping(CommonOptions.EMPTY_OPTIONS)) .build()); } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/CommonOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/CommonOptions.java new file mode 100644 index 00000000000000..eeee3c982a37b5 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/CommonOptions.java @@ -0,0 +1,36 @@ +// Copyright 2023 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; + +import com.google.devtools.common.options.Options; + +/** Common sets of option objects for use in core processing. */ +public final class CommonOptions { + + // Ideally the empty build options should be actually 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. + public static final BuildOptions EMPTY_OPTIONS = + BuildOptions.builder().addFragmentOptions(Options.getDefaults(CoreOptions.class)).build(); + + private CommonOptions() {} +} 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 index 5b8f0158fea5a6..dc5ca8b625ba4e 100644 --- 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 @@ -17,6 +17,7 @@ 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.CommonOptions; 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; @@ -43,8 +44,6 @@ public class NoConfigTransition implements PatchTransition { @SerializationConstant public static final NoConfigTransition INSTANCE = new NoConfigTransition(); - public static final BuildOptions NO_CONFIG_OPTIONS = BuildOptions.builder().build(); - private NoConfigTransition() {} @Override @@ -54,19 +53,7 @@ public ImmutableSet> requiresOptionFragments() @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).getDefault()) - .build(); + return CommonOptions.EMPTY_OPTIONS; } /** Returns a {@link TransitionFactory} instance that generates the transition. */ diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationKey.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationKey.java index 7facee83fb784a..c844c118ed54d9 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationKey.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildConfigurationKey.java @@ -27,6 +27,8 @@ @AutoCodec public final class BuildConfigurationKey implements SkyKey { + private static final SkyKeyInterner interner = SkyKey.newInterner(); + /** * Creates a new configuration key based on the given options, after applying a platform mapping * transformation. @@ -55,8 +57,6 @@ public static BuildConfigurationKey withoutPlatformMapping(BuildOptions options) return interner.intern(new BuildConfigurationKey(options)); } - private static final SkyKeyInterner interner = SkyKey.newInterner(); - private final BuildOptions options; private BuildConfigurationKey(BuildOptions options) {