forked from bazelbuild/bazel
-
Notifications
You must be signed in to change notification settings - Fork 1
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Refactor and fix issues with StarlarkTransition BuildSettingsPackages
Two main goals of this CL: * Properly include 'metadata about Starlark build settings' in the transition application cache. * Better isolate where and why the transition infrastructure reads BUILD file information. This unifies previously duplicated logic in SkyframeExecutor. First, the transition application cache, on a miss, would also read defaults, type, aliasing information, etc. from BUILD files but failed to include any of this info in the cache key. This lead to failure to re-evaluate a transition when that build setting information in BUILD files changes. This CL fixes that by introducing a new type StarlarkBuildSettingsDetailsValue, which contains all that information for a given set of Starlark build settings. That value is now included in the cache key. Second, rather than having ConfiguredTargetFunction (via machinery in StarlarkTransition) itself do the Skyframe calls and subsequent computations necessary to compute StarlarkBuildSettingsDetailsValue, a new SkyFunction StarlarkBuildSettingsDetailsFunction was created to do so. This provides a few benefits: 1. ConfiguredTargetFunction now is only registering an edge onto a single StarlarkBuildSettingsDetailsFunction, rather than all the PackageValue needed to resolve the StarlarkBuildSettings. StarlarkBuildSettingsDetailsValue more narrowly describes how sensitive the transition is to change. This means irrelevant changes to a package involved in build setting resolution will not cause as much invalidation. This information is now cached so StarlarkTransition (called via ConfiguredTargetFunction) can more quickly compute the cache key. (Beforehand, it was always unconditionally following any build setting alias chains). 2. SkyframeExecutor can just evaluate StarlarkBuildSettingsDetailsValue rather than having a slightly different implementation getBuildSettingsPackages. (The new getStarlarkBuildSettingsDetailsValue functions that must be synced are much simpler and easier to keep in sync.) As part of this unification, the error messages are now consistent regardless of whether the problematic build settings are asked for when transitioning the top-level target or an underlying target. 3. The split between reading Packages to compute StarlarkBuildSettingsDetailsValue and then later consuming it in StarlarkTransition should be easier to read and maintain. PS: As part of the refactor, significant issues were found in PrepareAnalysisPhaseFunction. Warnings have been added as comments and upcoming work will be deleting that file entirely. This fixes caching issue described at bazelbuild#15732 PiperOrigin-RevId: 462741847 Change-Id: I517a9a841735e6cff26faea5373f50a164334f86
- Loading branch information
1 parent
41cf353
commit 2f7d965
Showing
10 changed files
with
647 additions
and
377 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
98 changes: 98 additions & 0 deletions
98
...va/com/google/devtools/build/lib/analysis/starlark/StarlarkBuildSettingsDetailsValue.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| // 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.starlark; | ||
|
|
||
| import com.google.auto.value.AutoValue; | ||
| import com.google.common.collect.ImmutableMap; | ||
| import com.google.common.collect.ImmutableSet; | ||
| import com.google.devtools.build.lib.cmdline.Label; | ||
| import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; | ||
| import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; | ||
| import com.google.devtools.build.lib.packages.Type; | ||
| import com.google.devtools.build.lib.skyframe.SkyFunctions; | ||
| import com.google.devtools.build.skyframe.SkyFunctionName; | ||
| import com.google.devtools.build.skyframe.SkyKey; | ||
| import com.google.devtools.build.skyframe.SkyValue; | ||
| import com.google.errorprone.annotations.CheckReturnValue; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
|
|
||
| /** | ||
| * This contains information about a list of given Starlark build options, specifically their | ||
| * defaults and the (final) actual values of alias {@link Label}. | ||
| * | ||
| * <p>For memory-efficiency reasons, aliasToActual contains only aliases in keys. Other attributes | ||
| * contain only actual build setting as keys. | ||
| * | ||
| * <p>Potentially aliased targets can be unaliased with aliasToActual().getWithDefault(raw, raw); | ||
| */ | ||
| @CheckReturnValue | ||
| @Immutable | ||
| @ThreadSafe | ||
| @AutoValue | ||
| public abstract class StarlarkBuildSettingsDetailsValue implements SkyValue { | ||
| /** | ||
| * Create a single StarlarkBuildSettingsDetailsValue that can be quickly returned for transitions | ||
| * that use no Starlark build settings | ||
| */ | ||
| public static final StarlarkBuildSettingsDetailsValue EMPTY = | ||
| create(ImmutableMap.of(), ImmutableMap.of(), ImmutableSet.of(), ImmutableMap.of()); | ||
|
|
||
| /** Map from each build option to its default value. Does not include aliases. */ | ||
| public abstract ImmutableMap<Label, Object> buildSettingToDefault(); | ||
|
|
||
| /** Map from each build option to its type information. Does not include aliases. */ | ||
| public abstract ImmutableMap<Label, Type<?>> buildSettingToType(); | ||
|
|
||
| /** If build option is in this set, is an allows_multiple option. Does not include aliases. */ | ||
| public abstract ImmutableSet<Label> buildSettingIsAllowsMultiple(); | ||
|
|
||
| /** Map from an alias Label to actual Label it points to. */ | ||
| public abstract ImmutableMap<Label, Label> aliasToActual(); | ||
|
|
||
| public static StarlarkBuildSettingsDetailsValue create( | ||
| Map<Label, Object> buildSettingDefaults, | ||
| Map<Label, Type<?>> buildSettingToType, | ||
| Set<Label> buildSettingIsAllowsMultiple, | ||
| Map<Label, Label> aliasToActual) { | ||
| return new AutoValue_StarlarkBuildSettingsDetailsValue( | ||
| ImmutableMap.copyOf(buildSettingDefaults), | ||
| ImmutableMap.copyOf(buildSettingToType), | ||
| ImmutableSet.copyOf(buildSettingIsAllowsMultiple), | ||
| ImmutableMap.copyOf(aliasToActual)); | ||
| } | ||
|
|
||
| public static Key key(Set<Label> buildSettings) { | ||
| return Key.create(ImmutableSet.copyOf(buildSettings)); | ||
| } | ||
|
|
||
| /** {@link SkyKey} implementation used for {@link StarlarkBuildSettingsDetailsValue}. */ | ||
| @CheckReturnValue | ||
| @Immutable | ||
| @ThreadSafe | ||
| @AutoValue | ||
| public abstract static class Key implements SkyKey { | ||
| public abstract ImmutableSet<Label> buildSettings(); | ||
|
|
||
| @Override | ||
| public SkyFunctionName functionName() { | ||
| return SkyFunctions.STARLARK_BUILD_SETTINGS_DETAILS; | ||
| } | ||
|
|
||
| static Key create(ImmutableSet<Label> buildSettings) { | ||
| return new AutoValue_StarlarkBuildSettingsDetailsValue_Key(buildSettings); | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.