Skip to content

Commit

Permalink
Allow starlark-defined transitions to write build settings.
Browse files Browse the repository at this point in the history 
Previously we just operated on native options (for a few whitelisted users). While this CL allows writing build options, it does not validate that the values the transitions set for build settings are of the proper type. Nor does it allow reading of all build options. That will be coming in a follow up CL. Test added to note these gaps.

RELNOTES: None.
PiperOrigin-RevId: 228513456
  • Loading branch information
@juliexxia
juliexxia authored and Copybara-Service committed Jan 9, 2019
1 parent 569a31b commit 22de6cd
Show file tree
Hide file tree
Showing 4 changed files with 278 additions and 72 deletions.
124 changes: 56 additions & 68 deletions src/main/java/com/google/devtools/build/lib/analysis/skylark/FunctionTransitionUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.packages.StructImpl;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.Mutability;
Expand Down Expand Up @@ -80,9 +81,9 @@ static List<BuildOptions> applyAndValidate(
validateFunctionOutputs(transitions, starlarkTransition);

for (Map<String, Object> transition : transitions) {
BuildOptions options = buildOptions.clone();
applyTransition(options, transition, optionInfoMap, starlarkTransition);
splitBuildOptions.add(options);
BuildOptions transitionedOptions =
applyTransition(buildOptions, transition, optionInfoMap, starlarkTransition);
splitBuildOptions.add(transitionedOptions);
}
return splitBuildOptions.build();

Expand Down Expand Up @@ -117,29 +118,6 @@ private static void validateFunctionOutputs(
}
}

/**
* Given a label-like string representing a command line option, returns the command line option
* string that it represents. This is a temporary measure to support command line options with
* strings that look "label-like", so that migrating users using this experimental syntax is
* easier later.
*
* @throws EvalException if the given string is not a valid format to represent to a command line
* option
*/
private static String commandLineOptionLabelToOption(
String label, StarlarkDefinedConfigTransition starlarkTransition) throws EvalException {
if (label.startsWith(COMMAND_LINE_OPTION_PREFIX)) {
return label.substring(COMMAND_LINE_OPTION_PREFIX.length());
} else {
throw new EvalException(
starlarkTransition.getLocationForErrorReporting(),
String.format(
"Option key '%s' is of invalid form. "
+ "Expected command line option to begin with %s",
label, COMMAND_LINE_OPTION_PREFIX));
}
}

/** For all the options in the BuildOptions, build a map from option name to its information. */
private static Map<String, OptionInfo> buildOptionInfo(BuildOptions buildOptions) {
ImmutableMap.Builder<String, OptionInfo> builder = new ImmutableMap.Builder<>();
Expand Down Expand Up @@ -220,62 +198,70 @@ static SkylarkDict<String, Object> buildSettings(
* Apply the transition dictionary to the build option, using optionInfoMap to look up the option
* info.
*
* @param buildOptions the pre-transition build options
* @param newValues a map of option name: option value entries to override current option
* values in the buildOptions param
* @param optionInfoMap a map of option name: option info for all native options that may
* be accessed in this transition
* @param starlarkTransition transition object that is being applied. Used for error reporting
* and checking for analysis testing
* @param buildOptionsToTransition the pre-transition build options
* @param newValues a map of option name: option value entries to override current option values
* in the buildOptions param
* @param optionInfoMap a map of option name: option info for all native options that may be
* accessed in this transition
* @param starlarkTransition transition object that is being applied. Used for error reporting and
* checking for analysis testing
* @return the post-transition build options
* @throws EvalException If a requested option field is inaccessible
*/
static void applyTransition(
BuildOptions buildOptions,
private static BuildOptions applyTransition(
BuildOptions buildOptionsToTransition,
Map<String, Object> newValues,
Map<String, OptionInfo> optionInfoMap,
StarlarkDefinedConfigTransition starlarkTransition)
throws EvalException {
BuildOptions buildOptions = buildOptionsToTransition.clone();
for (Map.Entry<String, Object> entry : newValues.entrySet()) {
String optionKey = entry.getKey();

// TODO(juliexxia): Handle keys which correspond to build_setting target labels instead
// of assuming every key is for a command line option.
String optionName = commandLineOptionLabelToOption(optionKey, starlarkTransition);
String optionName = entry.getKey();
Object optionValue = entry.getValue();

// Convert NoneType to null.
if (optionValue instanceof NoneType) {
optionValue = null;
}
if (!optionName.startsWith(COMMAND_LINE_OPTION_PREFIX)) {
buildOptions =
BuildOptions.builder()
.merge(buildOptions)
.addStarlarkOption(Label.parseAbsoluteUnchecked(optionName), optionValue)
.build();
} else {
optionName = optionName.substring(COMMAND_LINE_OPTION_PREFIX.length());

try {
if (!optionInfoMap.containsKey(optionName)) {
throw new EvalException(
starlarkTransition.getLocationForErrorReporting(),
String.format(
"transition output '%s' does not correspond to a valid setting", optionKey));
// Convert NoneType to null.
if (optionValue instanceof NoneType) {
optionValue = null;
}

OptionInfo optionInfo = optionInfoMap.get(optionName);
OptionDefinition def = optionInfo.getDefinition();
Field field = def.getField();
FragmentOptions options = buildOptions.get(optionInfo.getOptionClass());
if (optionValue == null || def.getType().isInstance(optionValue)) {
field.set(options, optionValue);
} else if (optionValue instanceof String) {
field.set(options, def.getConverter().convert((String) optionValue));
} else {
try {
if (!optionInfoMap.containsKey(optionName)) {
throw new EvalException(
starlarkTransition.getLocationForErrorReporting(),
String.format(
"transition output '%s' does not correspond to a valid setting",
entry.getKey()));
}

OptionInfo optionInfo = optionInfoMap.get(optionName);
OptionDefinition def = optionInfo.getDefinition();
Field field = def.getField();
FragmentOptions options = buildOptions.get(optionInfo.getOptionClass());
if (optionValue == null || def.getType().isInstance(optionValue)) {
field.set(options, optionValue);
} else if (optionValue instanceof String) {
field.set(options, def.getConverter().convert((String) optionValue));
} else {
throw new EvalException(
starlarkTransition.getLocationForErrorReporting(),
"Invalid value type for option '" + optionName + "'");
}
} catch (IllegalAccessException e) {
throw new RuntimeException(
"IllegalAccess for option " + optionName + ": " + e.getMessage());
} catch (OptionsParsingException e) {
throw new EvalException(
starlarkTransition.getLocationForErrorReporting(),
"Invalid value type for option '" + optionName + "'");
"OptionsParsingError for option '" + optionName + "': " + e.getMessage());
}
} catch (IllegalAccessException e) {
throw new RuntimeException(
"IllegalAccess for option " + optionName + ": " + e.getMessage());
} catch (OptionsParsingException e) {
throw new EvalException(
starlarkTransition.getLocationForErrorReporting(),
"OptionsParsingError for option '" + optionName + "': " + e.getMessage());
}
}

Expand All @@ -286,6 +272,8 @@ static void applyTransition(
buildConfigOptions.evaluatingForAnalysisTest = true;
}
updateOutputDirectoryNameFragment(buildConfigOptions, newValues);

return buildOptions;
}

/**
Expand Down
16 changes: 12 additions & 4 deletions src/main/java/com/google/devtools/build/lib/rules/config/ConfigGlobalLibrary.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import com.google.common.collect.Sets;
import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.skylarkbuildapi.config.ConfigGlobalLibraryApi;
import com.google.devtools.build.lib.skylarkbuildapi.config.ConfigurationTransitionApi;
Expand Down Expand Up @@ -72,15 +73,22 @@ public ConfigurationTransitionApi analysisTestTransition(
private void validateBuildSettingKeys(
Iterable<String> optionKeys, String keyErrorDescriptor, Location location)
throws EvalException {
// TODO(juliexxia): Allow real labels, not just command line option placeholders.

HashSet<String> processedOptions = Sets.newHashSet();

for (String optionKey : optionKeys) {
if (!optionKey.startsWith(COMMAND_LINE_OPTION_PREFIX)) {
throw new EvalException(location,
String.format("invalid transition %s '%s'. If this is intended as a native option, "
+ "it must begin with //command_line_option:", keyErrorDescriptor, optionKey));
try {
Label.parseAbsoluteUnchecked(optionKey);
} catch (IllegalArgumentException e) {
throw new EvalException(
location,
String.format(
"invalid transition %s '%s'. If this is intended as a native option, "
+ "it must begin with //command_line_option:",
keyErrorDescriptor, optionKey),
e);
}
}
if (!processedOptions.add(optionKey)) {
throw new EvalException(location,
Expand Down
74 changes: 74 additions & 0 deletions src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,12 @@
import static org.junit.Assert.fail;

import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.ListMultimap;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.packages.util.BazelMockAndroidSupport;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -702,6 +706,76 @@ public void testInvalidOutputKey_analysisTest() throws Exception {
+ "it must begin with //command_line_option:");
}

@Test
public void testTransitionOnBuildSetting() throws Exception {
setSkylarkSemanticsOptions(
"--experimental_starlark_config_transitions=true", "--experimental_build_setting_api");
writeWhitelistFile();

scratch.file(
"test/skylark/build_settings.bzl",
"BuildSettingInfo = provider(fields = ['value'])",
"def _impl(ctx):",
" return [BuildSettingInfo(value = ctx.build_setting_value)]",
"string_flag = rule(implementation = _impl, build_setting = config.string(flag=True))");

scratch.file(
"test/skylark/rules.bzl",
"load('//test/skylark:build_settings.bzl', 'BuildSettingInfo')",
"def _transition_impl(settings, attr):",
" return {'//test:cute-animal-fact': 'puffins mate for life'}",
"my_transition = transition(",
" implementation = _transition_impl,",
" inputs = [],",
" outputs = ['//test:cute-animal-fact']",
")",
"def _rule_impl(ctx):",
" return struct(dep = ctx.attr.dep)",
"my_rule = rule(",
" implementation = _rule_impl,",
" attrs = {",
" 'dep': attr.label(cfg = my_transition),",
" '_whitelist_function_transition': attr.label(",
" default = '//tools/whitelists/function_transition_whitelist'),",
" }",
")",
"def _dep_rule_impl(ctx):",
" return [BuildSettingInfo(value = ctx.attr.fact[BuildSettingInfo].value)]",
"dep_rule_impl = rule(",
" implementation = _dep_rule_impl,",
" attrs = {",
" 'fact': attr.label(default = '//test/skylark:cute-animal-fact'),",
" }",
")");

scratch.file(
"test/skylark/BUILD",
"load('//test/skylark:rules.bzl', 'my_rule')",
"load('//test/skylark:build_settings.bzl', 'string_flag')",
"my_rule(name = 'test', dep = ':dep')",
"my_rule(name = 'dep')",
"string_flag(",
" name = 'cute-animal-fact',",
" build_setting_default = 'cows produce more milk when they listen to soothing music',",
")");

useConfiguration(ImmutableMap.of("//test:cute-animal-fact", "cats can't taste sugar"));
getConfiguredTarget("//test/skylark:test");

@SuppressWarnings("unchecked")
BuildConfiguration depConfiguration =
getConfiguration(
Iterables.getOnlyElement(
(List<ConfiguredTarget>) getConfiguredTarget("//test/skylark:test").get("dep")));

assertThat(
depConfiguration
.getOptions()
.getStarlarkOptions()
.get(Label.parseAbsoluteUnchecked("//test:cute-animal-fact")))
.isEqualTo("puffins mate for life");
}

@Test
public void testOptionConversionDynamicMode() throws Exception {
// TODO(waltl): check that dynamic_mode is parsed properly.
Expand Down
Loading

0 comments on commit 22de6cd

Please sign in to comment.