Skip to content

Commit

Permalink
bazel starlark transitions: grab build setting label from Starlark ca…
Browse files Browse the repository at this point in the history
…ll stack in order to canonicalize.

There are 5 locations a user might write the label string of a build setting:
(1) on the command line
(2) in the transition definition inputs
(3) in the transition definition output
(4) in the transition impl function while reading the settings dict
(5) in the transition impl function return dict

We know that, depending on the situation, there are multiple ways to write "a build setting in the main repo of this project" - //myflag, @//myflag, @main_repo//my:flag.

In all 5 places listed above, bazel needs to recognize that the three different versions of //myflag above all map back to a single target. We need this for deduping purposes and consistency. 1 is taken care of in a grandparent CL to this CL. This Cl addresses locations 2-5.

To do this, whenever we are interpreting a user's input at one of the locations above, we covert that input string to the canonical string form of that build setting for our back end book keeping purposes. The logic to do this was modeled off of the `Label()` function logic. We also keep a map of canonicalized form to the user-given form in order to display messages (or key the dict in 4) to the user using the forms of the flags they used themselves.

work towards #11128

PiperOrigin-RevId: 351426279
  • Loading branch information
juliexxia authored and copybara-github committed Jan 12, 2021
1 parent 6d8f067 commit 98d376f
Show file tree
Hide file tree
Showing 7 changed files with 471 additions and 96 deletions.
3 changes: 1 addition & 2 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -1762,6 +1762,7 @@ java_library(
srcs = ["config/StarlarkDefinedConfigTransition.java"],
deps = [
":config/transitions/configuration_transition",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/config",
Expand Down Expand Up @@ -2096,8 +2097,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/common/options",
"//src/main/java/net/starlark/java/eval",
"//src/main/java/net/starlark/java/syntax",
"//third_party:error_prone_annotations",
"//third_party:guava",
"//third_party:jsr305",
],
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.google.devtools.build.lib.analysis.config.CoreOptions;
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition.ValidationException;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
Expand All @@ -34,9 +35,7 @@
import com.google.devtools.common.options.OptionDefinition;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
import com.google.errorprone.annotations.FormatMethod;
import java.lang.reflect.Field;
import java.util.Collection;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.List;
Expand Down Expand Up @@ -96,7 +95,6 @@ static Map<String, BuildOptions> applyAndValidate(
if (transitions == null) {
return null; // errors reported to handler
}
validateFunctionOutputsMatchesDeclaredOutputs(transitions.values(), starlarkTransition);

for (Map.Entry<String, Map<String, Object>> entry : transitions.entrySet()) {
Map<String, Object> newValues = handleImplicitPlatformChange(entry.getValue());
Expand All @@ -113,17 +111,6 @@ static Map<String, BuildOptions> applyAndValidate(
}
}

private static final class ValidationException extends Exception {
ValidationException(String message) {
super(message);
}

@FormatMethod
static ValidationException format(String format, Object... args) {
return new ValidationException(String.format(format, args));
}
}

/**
* If the transition changes --cpu but not --platforms, clear out --platforms.
*
Expand Down Expand Up @@ -166,33 +153,6 @@ private static void checkForDenylistedOptions(StarlarkDefinedConfigTransition tr
}
}

/**
* Validates that function outputs exactly the set of outputs it declares. More thorough checking
* (like type checking of output values) is done elsewhere because it requires loading. see {@link
* StarlarkTransition#validate}
*/
private static void validateFunctionOutputsMatchesDeclaredOutputs(
Collection<Map<String, Object>> transitions,
StarlarkDefinedConfigTransition starlarkTransition)
throws ValidationException {
for (Map<String, Object> transition : transitions) {
LinkedHashSet<String> remainingOutputs =
Sets.newLinkedHashSet(starlarkTransition.getOutputs());
for (String outputKey : transition.keySet()) {
if (!remainingOutputs.remove(outputKey)) {
throw ValidationException.format(
"transition function returned undeclared output '%s'", outputKey);
}
}

if (!remainingOutputs.isEmpty()) {
throw ValidationException.format(
"transition outputs [%s] were not defined by transition function",
Joiner.on(", ").join(remainingOutputs));
}
}
}

/** For all the options in the BuildOptions, build a map from option name to its information. */
static ImmutableMap<String, OptionInfo> buildOptionInfo(BuildOptions buildOptions) {
ImmutableMap.Builder<String, OptionInfo> builder = new ImmutableMap.Builder<>();
Expand Down Expand Up @@ -230,7 +190,10 @@ static Dict<String, Object> buildSettings(
Map<String, OptionInfo> optionInfoMap,
StarlarkDefinedConfigTransition starlarkTransition)
throws ValidationException {
LinkedHashSet<String> remainingInputs = Sets.newLinkedHashSet(starlarkTransition.getInputs());
Map<String, String> inputsCanonicalizedToGiven =
starlarkTransition.getInputsCanonicalizedToGiven();
LinkedHashSet<String> remainingInputs =
Sets.newLinkedHashSet(inputsCanonicalizedToGiven.keySet());

try (Mutability mutability = Mutability.create("build_settings")) {
Dict<String, Object> dict = Dict.of(mutability);
Expand Down Expand Up @@ -261,11 +224,15 @@ static Dict<String, Object> buildSettings(

// Add Starlark options
for (Map.Entry<Label, Object> starlarkOption : buildOptions.getStarlarkOptions().entrySet()) {
if (!remainingInputs.remove(starlarkOption.getKey().toString())) {
String canonicalLabelForm = starlarkOption.getKey().getUnambiguousCanonicalForm();
if (!remainingInputs.remove(canonicalLabelForm)) {
continue;
}
// Convert the canonical form to the user requested form that they expect to see in this
// dict.
String userRequestedLabelForm = inputsCanonicalizedToGiven.get(canonicalLabelForm);
try {
dict.putEntry(starlarkOption.getKey().toString(), starlarkOption.getValue());
dict.putEntry(userRequestedLabelForm, starlarkOption.getValue());
} catch (EvalException ex) {
throw new IllegalStateException(ex); // can't happen
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@ public void storeInThread(StarlarkThread thread) {
}

private final Phase phase;
private final String toolsRepository;
// Only necessary for loading phase threads.
@Nullable private final String toolsRepository;
// Only necessary for loading phase threads to construct configuration_field.
@Nullable private final ImmutableMap<String, Class<?>> fragmentNameToClass;
private final ImmutableMap<RepositoryName, RepositoryName> repoMapping;
private final HashMap<String, Label> convertedLabelsInPackage;
Expand All @@ -57,9 +59,11 @@ public void storeInThread(StarlarkThread thread) {

/**
* @param phase the phase to which this Starlark thread belongs
* @param toolsRepository the name of the tools repository, such as "@bazel_tools"
* @param toolsRepository the name of the tools repository, such as "@bazel_tools" for loading
* phase threads, null for other threads.
* @param fragmentNameToClass a map from configuration fragment name to configuration fragment
* class, such as "apple" to AppleConfiguration.class
* class, such as "apple" to AppleConfiguration.class for loading phase threads, null for
* other threads.
* @param repoMapping a map from RepositoryName to RepositoryName to be used for external
* @param convertedLabelsInPackage a mutable map from String to Label, used during package loading
* of a single package.
Expand All @@ -78,7 +82,7 @@ public void storeInThread(StarlarkThread thread) {
// analysis threads?
public BazelStarlarkContext(
Phase phase,
String toolsRepository,
@Nullable String toolsRepository,
@Nullable ImmutableMap<String, Class<?>> fragmentNameToClass,
ImmutableMap<RepositoryName, RepositoryName> repoMapping,
HashMap<String, Label> convertedLabelsInPackage,
Expand All @@ -99,6 +103,7 @@ public Phase getPhase() {
}

/** Returns the name of the tools repository, such as "@bazel_tools". */
@Nullable
@Override
public String getToolsRepository() {
return toolsRepository;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,14 @@

import static com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition.COMMAND_LINE_OPTION_PREFIX;

import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition.Settings;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.packages.BazelModuleContext;
import com.google.devtools.build.lib.packages.BazelStarlarkContext;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.starlarkbuildapi.config.ConfigGlobalLibraryApi;
import com.google.devtools.build.lib.starlarkbuildapi.config.ConfigurationTransitionApi;
Expand All @@ -28,11 +32,13 @@
import java.util.Map;
import net.starlark.java.eval.Dict;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Module;
import net.starlark.java.eval.Sequence;
import net.starlark.java.eval.Starlark;
import net.starlark.java.eval.StarlarkCallable;
import net.starlark.java.eval.StarlarkSemantics;
import net.starlark.java.eval.StarlarkThread;
import net.starlark.java.syntax.Location;

/**
* Implementation of {@link ConfigGlobalLibraryApi}.
Expand All @@ -59,8 +65,12 @@ public ConfigurationTransitionApi transition(
outputsList,
Settings.OUTPUTS,
semantics.getBool(BuildLanguageOptions.EXPERIMENTAL_STARLARK_CONFIG_TRANSITIONS));
Label parentLabel =
BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread)).label();
Location location = thread.getCallerLocation();
BazelStarlarkContext context = BazelStarlarkContext.from(thread);
return StarlarkDefinedConfigTransition.newRegularTransition(
implementation, inputsList, outputsList, semantics, thread);
implementation, inputsList, outputsList, semantics, parentLabel, location, context);
}

@Override
Expand All @@ -71,8 +81,13 @@ public ConfigurationTransitionApi analysisTestTransition(
Map<String, Object> changedSettingsMap =
Dict.cast(changedSettings, String.class, Object.class, "changed_settings dict");
validateBuildSettingKeys(changedSettingsMap.keySet(), Settings.OUTPUTS, true);
ImmutableMap<RepositoryName, RepositoryName> repoMapping =
BazelStarlarkContext.from(thread).getRepoMapping();
Label parentLabel =
BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread)).label();
Location location = thread.getCallerLocation();
return StarlarkDefinedConfigTransition.newAnalysisTestTransition(
changedSettingsMap, thread.getCallerLocation());
changedSettingsMap, repoMapping, parentLabel, location);
}

private void validateBuildSettingKeys(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -765,7 +765,7 @@ public void testInvalidNativeOptionInput() throws Exception {
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//test/starlark:test");
assertContainsEvent(
"transition inputs [//command_line_option:foop, //command_line_option:barp] "
"transition inputs [//command_line_option:barp, //command_line_option:foop] "
+ "do not correspond to valid settings");
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
// Copyright 2020 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;

import static com.google.common.truth.Truth.assertThat;

import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.testutil.Scratch;
import java.util.Map;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

/**
* Test of common logic between Starlark-defined transitions. Rule-transition- or
* attr-transition-specific logic should be tested in {@link StarlarkRuleTransitionProviderTest} and
* {@link StarlarkAttrTransitionProviderTest}.
*/
@RunWith(JUnit4.class)
public class StarlarkTransitionTest extends BuildViewTestCase {
static void writeAllowlistFile(Scratch scratch) throws Exception {
scratch.file(
"tools/allowlists/function_transition_allowlist/BUILD",
"package_group(",
" name = 'function_transition_allowlist',",
" packages = [",
" '//test/...',",
" ],",
")");
}

@Test
public void testDupeSettingsInInputsThrowsError() throws Exception {
scratch.file(
"test/defs.bzl",
"def _setting_impl(ctx):",
" return []",
"string_flag = rule(",
" implementation = _setting_impl,",
" build_setting = config.string(flag=True),",
")",
"def _transition_impl(settings, attr):",
" return {'//test:formation': 'mesa'}",
"formation_transition = transition(",
" implementation = _transition_impl,",
" inputs = ['@//test:formation', '//test:formation'],", // duplicates here
" outputs = ['//test:formation'],",
")",
"def _impl(ctx):",
" return []",
"state = rule(",
" implementation = _impl,",
" cfg = formation_transition,",
" attrs = {",
" '_allowlist_function_transition': attr.label(",
" default = '//tools/allowlists/function_transition_allowlist',",
" ),",
" })");
scratch.file(
"test/BUILD",
"load('//test:defs.bzl', 'state', 'string_flag')",
"state(name = 'arizona')",
"string_flag(name = 'formation', build_setting_default = 'canyon')");

reporter.removeHandler(failFastHandler);
getConfiguredTarget("//test:arizona");
assertContainsEvent(
"Transition declares duplicate build setting '@//test:formation' in INPUTS");
}

@Test
public void testDupeSettingsInOutputsThrowsError() throws Exception {
scratch.file(
"test/defs.bzl",
"def _setting_impl(ctx):",
" return []",
"string_flag = rule(",
" implementation = _setting_impl,",
" build_setting = config.string(flag=True),",
")",
"def _transition_impl(settings, attr):",
" return {'//test:formation': 'mesa'}",
"formation_transition = transition(",
" implementation = _transition_impl,",
" inputs = ['//test:formation'],",
" outputs = ['@//test:formation', '//test:formation'],", // duplicates here
")",
"def _impl(ctx):",
" return []",
"state = rule(",
" implementation = _impl,",
" cfg = formation_transition,",
" attrs = {",
" '_allowlist_function_transition': attr.label(",
" default = '//tools/allowlists/function_transition_allowlist',",
" ),",
" })");
scratch.file(
"test/BUILD",
"load('//test:defs.bzl', 'state', 'string_flag')",
"state(name = 'arizona')",
"string_flag(name = 'formation', build_setting_default = 'canyon')");

reporter.removeHandler(failFastHandler);
getConfiguredTarget("//test:arizona");
assertContainsEvent(
"Transition declares duplicate build setting '@//test:formation' in OUTPUTS");
}

@Test
public void testDifferentFormsOfFlagInInputsAndOutputs() throws Exception {
writeAllowlistFile(scratch);
scratch.file(
"test/defs.bzl",
"def _setting_impl(ctx):",
" return []",
"string_flag = rule(",
" implementation = _setting_impl,",
" build_setting = config.string(flag=True),",
")",
"def _transition_impl(settings, attr):",
" return {",
" '//test:formation': settings['@//test:formation']+'-transitioned',",
" }",
"formation_transition = transition(",
" implementation = _transition_impl,",
" inputs = ['@//test:formation'],",
" outputs = ['//test:formation'],",
")",
"def _impl(ctx):",
" return []",
"state = rule(",
" implementation = _impl,",
" cfg = formation_transition,",
" attrs = {",
" '_allowlist_function_transition': attr.label(",
" default = '//tools/allowlists/function_transition_allowlist',",
" ),",
" })");
scratch.file(
"test/BUILD",
"load('//test:defs.bzl', 'state', 'string_flag')",
"state(name = 'arizona')",
"string_flag(name = 'formation', build_setting_default = 'canyon')");

Map<Label, Object> starlarkOptions =
getConfiguration(getConfiguredTarget("//test:arizona")).getOptions().getStarlarkOptions();
assertThat(starlarkOptions).hasSize(1);
assertThat(starlarkOptions.get(Label.parseAbsoluteUnchecked("//test:formation")))
.isEqualTo("canyon-transitioned");
}
}

0 comments on commit 98d376f

Please sign in to comment.