Skip to content

Commit

Permalink
Clean up uses of an "empty" BuildOptions.
Browse files Browse the repository at this point in the history
Work towards platform-based flags: #19409.

PiperOrigin-RevId: 569576336
Change-Id: I7c7f37c833987aed83b04aaddac234ef3b17de2e
  • Loading branch information
katre authored and copybara-github committed Sep 29, 2023
1 parent 0656103 commit 0626570
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 22 deletions.
12 changes: 12 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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"],
Expand Down Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -103,11 +105,8 @@ public static ImmutableSet<ConfiguredTargetKey> 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());
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -54,19 +53,7 @@ public ImmutableSet<Class<? extends FragmentOptions>> 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. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
@AutoCodec
public final class BuildConfigurationKey implements SkyKey {

private static final SkyKeyInterner<BuildConfigurationKey> interner = SkyKey.newInterner();

/**
* Creates a new configuration key based on the given options, after applying a platform mapping
* transformation.
Expand Down Expand Up @@ -55,8 +57,6 @@ public static BuildConfigurationKey withoutPlatformMapping(BuildOptions options)
return interner.intern(new BuildConfigurationKey(options));
}

private static final SkyKeyInterner<BuildConfigurationKey> interner = SkyKey.newInterner();

private final BuildOptions options;

private BuildConfigurationKey(BuildOptions options) {
Expand Down

0 comments on commit 0626570

Please sign in to comment.