Don't store default values of build settings during Starlark options parsing.
This is the same behavior as during transitions. Since we don't know all related starlark options at the top of the build (like we do with native options) we need to make set to default == unset == unstored.
PiperOrigin-RevId: 251695315
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java b/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java
index aad40d2..471419d 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java
@@ -15,6 +15,7 @@
package com.google.devtools.build.lib.runtime;
import static com.google.devtools.build.lib.analysis.config.CoreOptionConverters.BUILD_SETTING_CONVERTERS;
+import static com.google.devtools.build.lib.packages.RuleClass.Builder.SKYLARK_BUILD_SETTING_DEFAULT_ATTR_NAME;
import static com.google.devtools.build.lib.syntax.Type.BOOLEAN;
import com.google.common.annotations.VisibleForTesting;
@@ -28,6 +29,7 @@
import com.google.devtools.build.lib.packages.BuildSetting;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.StarlarkSemanticsOptions;
+import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.pkgcache.LoadingOptions;
import com.google.devtools.build.lib.pkgcache.PackageCacheOptions;
import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
@@ -92,7 +94,7 @@
throws OptionsParsingException {
ImmutableList.Builder<String> residue = new ImmutableList.Builder<>();
// Map of <option name (label), <unparsed option value, loaded option>>.
- Map<String, Pair<String, BuildSetting>> unparsedOptions =
+ Map<String, Pair<String, Target>> unparsedOptions =
Maps.newHashMapWithExpectedSize(nativeOptionsParser.getResidue().size());
// sort the old residue into starlark flags and legitimate residue
@@ -116,10 +118,12 @@
}
ImmutableMap.Builder<String, Object> parsedOptions = new ImmutableMap.Builder<>();
- for (Map.Entry<String, Pair<String, BuildSetting>> option : unparsedOptions.entrySet()) {
+ for (Map.Entry<String, Pair<String, Target>> option : unparsedOptions.entrySet()) {
String loadedFlag = option.getKey();
String unparsedValue = option.getValue().first;
- BuildSetting buildSetting = option.getValue().second;
+ Target buildSettingTarget = option.getValue().second;
+ BuildSetting buildSetting =
+ buildSettingTarget.getAssociatedRule().getRuleClassObject().getBuildSetting();
// Do not recognize internal options, which are treated as if they did not exist.
if (!buildSetting.isFlag()) {
throw new OptionsParsingException(
@@ -137,7 +141,13 @@
loadedFlag, unparsedValue, unparsedValue, type),
e);
}
- parsedOptions.put(loadedFlag, value);
+ if (!value.equals(
+ buildSettingTarget
+ .getAssociatedRule()
+ .getAttributeContainer()
+ .getAttr(SKYLARK_BUILD_SETTING_DEFAULT_ATTR_NAME))) {
+ parsedOptions.put(loadedFlag, value);
+ }
}
nativeOptionsParser.setStarlarkOptions(parsedOptions.build());
}
@@ -145,7 +155,7 @@
private void parseArg(
String arg,
Iterator<String> unparsedArgs,
- Map<String, Pair<String, BuildSetting>> unparsedOptions,
+ Map<String, Pair<String, Target>> unparsedOptions,
Command command,
ExtendedEventHandler eventHandler)
throws OptionsParsingException {
@@ -158,8 +168,9 @@
if (value != null) {
// --flag=value or -flag=value form
- BuildSetting current = loadBuildSetting(name, nativeOptionsParser, command, eventHandler);
- unparsedOptions.put(name, new Pair<>(value, current));
+ Target buildSettingTarget =
+ loadBuildSetting(name, nativeOptionsParser, command, eventHandler);
+ unparsedOptions.put(name, new Pair<>(value, buildSettingTarget));
} else {
boolean booleanValue = true;
// check --noflag form
@@ -167,10 +178,13 @@
booleanValue = false;
name = name.substring(2);
}
- BuildSetting current = loadBuildSetting(name, nativeOptionsParser, command, eventHandler);
+ Target buildSettingTarget =
+ loadBuildSetting(name, nativeOptionsParser, command, eventHandler);
+ BuildSetting current =
+ buildSettingTarget.getAssociatedRule().getRuleClassObject().getBuildSetting();
if (current.getType().equals(BOOLEAN)) {
// --boolean_flag or --noboolean_flag
- unparsedOptions.put(name, new Pair<>(String.valueOf(booleanValue), current));
+ unparsedOptions.put(name, new Pair<>(String.valueOf(booleanValue), buildSettingTarget));
} else {
if (!booleanValue) {
// --no(non_boolean_flag)
@@ -179,7 +193,7 @@
}
if (unparsedArgs.hasNext()) {
// --flag value
- unparsedOptions.put(name, new Pair<>(unparsedArgs.next(), current));
+ unparsedOptions.put(name, new Pair<>(unparsedArgs.next(), buildSettingTarget));
} else {
throw new OptionsParsingException("Expected value after " + arg);
}
@@ -187,13 +201,13 @@
}
}
- private BuildSetting loadBuildSetting(
+ private Target loadBuildSetting(
String targetToBuild,
OptionsParser optionsParser,
Command command,
ExtendedEventHandler eventHandler)
throws OptionsParsingException {
- Rule associatedRule;
+ Target buildSetting;
try {
TargetPatternPhaseValue result =
skyframeExecutor.loadTargetPatterns(
@@ -204,19 +218,19 @@
SkyframeExecutor.DEFAULT_THREAD_COUNT,
optionsParser.getOptions(KeepGoingOption.class).keepGoing,
command.name().equals("test"));
- associatedRule =
+ buildSetting =
Iterables.getOnlyElement(
- result.getTargets(eventHandler, skyframeExecutor.getPackageManager()))
- .getAssociatedRule();
+ result.getTargets(eventHandler, skyframeExecutor.getPackageManager()));
} catch (InterruptedException | TargetParsingException e) {
Thread.currentThread().interrupt();
throw new OptionsParsingException(
"Error loading option " + targetToBuild + ": " + e.getMessage(), e);
}
+ Rule associatedRule = buildSetting.getAssociatedRule();
if (associatedRule == null || associatedRule.getRuleClassObject().getBuildSetting() == null) {
throw new OptionsParsingException("Unrecognized option: " + targetToBuild);
}
- return associatedRule.getRuleClassObject().getBuildSetting();
+ return buildSetting;
}
@VisibleForTesting
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/StarlarkOptionsParsingTest.java b/src/test/java/com/google/devtools/build/lib/skylark/StarlarkOptionsParsingTest.java
index fa21f9b..2e4500a 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/StarlarkOptionsParsingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/StarlarkOptionsParsingTest.java
@@ -247,10 +247,10 @@
public void testBooleanFlag() throws Exception {
writeBasicBoolFlag();
- OptionsParsingResult result = parseStarlarkOptions("--//test:my_bool_setting");
+ OptionsParsingResult result = parseStarlarkOptions("--//test:my_bool_setting=false");
assertThat(result.getStarlarkOptions()).hasSize(1);
- assertThat(result.getStarlarkOptions().get("//test:my_bool_setting")).isEqualTo(true);
+ assertThat(result.getStarlarkOptions().get("//test:my_bool_setting")).isEqualTo(false);
assertThat(result.getResidue()).isEmpty();
}
@@ -396,4 +396,15 @@
.hasMessageThat()
.isEqualTo("While parsing option //test:my_bool_setting=woohoo: 'woohoo' is not a boolean");
}
+
+ // test --int-flag=same value as default
+ @Test
+ public void testDontStoreDefaultValue() throws Exception {
+ // build_setting_default = 42
+ writeBasicIntFlag();
+
+ OptionsParsingResult result = parseStarlarkOptions("--//test:my_int_setting=42");
+
+ assertThat(result.getStarlarkOptions()).isEmpty();
+ }
}
