Preserve relative order of explicit and expanded Starlark flags
Previously, Starlark flags expanded from a --config stanze would always
be added to the end of the residue containing the explicitly given
Starlark flags. As a result, Starlark flags expanded from --config could
not be overridden.
With this commit, Starlark flags are parsed with the same semantics as
a regular allowMultiple option, thus preserving their order through
expansions.
This is implemented by introducing a synthetic allowMultiple option of
type List<String> in OptionsParserImpl that skipped args are parsed
into.
As a result, Starlark options are now available from the new
getSkippedArgs() method on OptionsParser rather than as part of the
residue, with the latter now only containing build targets.
Fixes #13231
Fixes #15679
Closes #15807.
PiperOrigin-RevId: 467931815
Change-Id: Ic64c6e075c08d898e5e7b8bf4c777827134d89fa
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 edde8cf..ee55f17 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
@@ -25,8 +25,6 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.LinkedListMultimap;
import com.google.common.collect.Multimap;
-import com.google.devtools.build.lib.cmdline.LabelValidator;
-import com.google.devtools.build.lib.cmdline.LabelValidator.BadLabelException;
import com.google.devtools.build.lib.cmdline.TargetParsingException;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
@@ -131,25 +129,19 @@
// require multiple rounds of parsing to fit starlark-defined options into native option format.
@VisibleForTesting
public void parse(ExtendedEventHandler eventHandler) throws OptionsParsingException {
- ImmutableList.Builder<String> residue = new ImmutableList.Builder<>();
+ parseGivenArgs(eventHandler, nativeOptionsParser.getSkippedArgs());
+ }
+
+ @VisibleForTesting
+ public void parseGivenArgs(ExtendedEventHandler eventHandler, List<String> args)
+ throws OptionsParsingException {
// Map of <option name (label), <unparsed option value, loaded option>>.
Multimap<String, Pair<String, Target>> unparsedOptions = LinkedListMultimap.create();
- // sort the old residue into starlark flags and legitimate residue
- for (String arg : nativeOptionsParser.getPreDoubleDashResidue()) {
- // TODO(bazel-team): support single dash options?
- if (!arg.startsWith("--")) {
- residue.add(arg);
- continue;
- }
-
+ for (String arg : args) {
parseArg(arg, unparsedOptions, eventHandler);
}
- List<String> postDoubleDashResidue = nativeOptionsParser.getPostDoubleDashResidue();
- residue.addAll(postDoubleDashResidue);
- nativeOptionsParser.setResidue(residue.build(), postDoubleDashResidue);
-
if (unparsedOptions.isEmpty()) {
return;
}
@@ -236,6 +228,9 @@
Multimap<String, Pair<String, Target>> unparsedOptions,
ExtendedEventHandler eventHandler)
throws OptionsParsingException {
+ if (!arg.startsWith("--")) {
+ throw new OptionsParsingException("Invalid options syntax: " + arg, arg);
+ }
int equalsAt = arg.indexOf('=');
String name = equalsAt == -1 ? arg.substring(2) : arg.substring(2, equalsAt);
if (name.trim().isEmpty()) {
@@ -308,51 +303,6 @@
return buildSetting;
}
- /**
- * Separates out any Starlark options from the given list
- *
- * <p>This method doesn't go through the trouble to actually load build setting targets and verify
- * they are build settings, it just assumes all strings that look like they could be build
- * settings, aka are formatted like a flag and can parse out to a proper label, are build
- * settings. Use actual parsing functions above to do full build setting verification.
- *
- * @param list List of strings from which to parse out starlark options
- * @return Returns a pair of string lists. The first item contains the list of starlark options
- * that were removed; the second contains the remaining string from the original list.
- */
- public static Pair<ImmutableList<String>, ImmutableList<String>> removeStarlarkOptions(
- List<String> list) {
- ImmutableList.Builder<String> keep = ImmutableList.builder();
- ImmutableList.Builder<String> remove = ImmutableList.builder();
- for (String name : list) {
- // Check if the string is a flag and trim off "--" if so.
- if (!name.startsWith("--")) {
- keep.add(name);
- continue;
- }
- String potentialStarlarkFlag = name.substring(2);
- // Check if the string uses the "no" prefix for setting boolean flags to false, trim
- // off "no" if so.
- if (potentialStarlarkFlag.startsWith("no")) {
- potentialStarlarkFlag = potentialStarlarkFlag.substring(2);
- }
- // Check if the string contains a value, trim off the value if so.
- int equalsIdx = potentialStarlarkFlag.indexOf('=');
- if (equalsIdx > 0) {
- potentialStarlarkFlag = potentialStarlarkFlag.substring(0, equalsIdx);
- }
- // Check if we can properly parse the (potentially trimmed) string as a label. If so, count
- // as starlark flag, else count as regular residue.
- try {
- LabelValidator.validateAbsoluteLabel(potentialStarlarkFlag);
- remove.add(name);
- } catch (BadLabelException e) {
- keep.add(name);
- }
- }
- return Pair.of(remove.build(), keep.build());
- }
-
@VisibleForTesting
public static StarlarkOptionsParser newStarlarkOptionsParserForTesting(
SkyframeExecutor skyframeExecutor,
@@ -364,11 +314,6 @@
}
@VisibleForTesting
- public void setResidueForTesting(List<String> residue) {
- nativeOptionsParser.setResidue(residue, ImmutableList.of());
- }
-
- @VisibleForTesting
public OptionsParser getNativeOptionsParserFortesting() {
return nativeOptionsParser;
}
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java
index 248dee2..163d8fd 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/CleanCommand.java
@@ -16,7 +16,6 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Joiner;
import com.google.common.base.Strings;
-import com.google.common.collect.ImmutableList;
import com.google.common.flogger.GoogleLogger;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.analysis.NoBuildEvent;
@@ -29,7 +28,6 @@
import com.google.devtools.build.lib.runtime.BlazeRuntime;
import com.google.devtools.build.lib.runtime.Command;
import com.google.devtools.build.lib.runtime.CommandEnvironment;
-import com.google.devtools.build.lib.runtime.StarlarkOptionsParser;
import com.google.devtools.build.lib.runtime.commands.events.CleanStartingEvent;
import com.google.devtools.build.lib.server.FailureDetails;
import com.google.devtools.build.lib.server.FailureDetails.CleanCommand.Code;
@@ -39,7 +37,6 @@
import com.google.devtools.build.lib.util.CommandBuilder;
import com.google.devtools.build.lib.util.InterruptedFailureDetails;
import com.google.devtools.build.lib.util.OS;
-import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionDocumentationCategory;
@@ -49,6 +46,7 @@
import java.io.FileDescriptor;
import java.io.FileOutputStream;
import java.io.IOException;
+import java.util.List;
import java.util.UUID;
import java.util.logging.LogManager;
@@ -132,17 +130,15 @@
@Override
public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult options) {
- // Assert that the only residue is starlark options and ignore them.
- Pair<ImmutableList<String>, ImmutableList<String>> starlarkOptionsAndResidue =
- StarlarkOptionsParser.removeStarlarkOptions(options.getResidue());
- ImmutableList<String> removedStarlarkOptions = starlarkOptionsAndResidue.getFirst();
- ImmutableList<String> residue = starlarkOptionsAndResidue.getSecond();
- if (!removedStarlarkOptions.isEmpty()) {
+ // Assert that there is no residue and warn about Starlark options.
+ List<String> starlarkOptions = options.getSkippedArgs();
+ List<String> residue = options.getResidue();
+ if (!starlarkOptions.isEmpty()) {
env.getReporter()
.handle(
Event.warn(
"Blaze clean does not support starlark options. Ignoring options: "
- + removedStarlarkOptions));
+ + starlarkOptions));
}
if (!residue.isEmpty()) {
String message = "Unrecognized arguments: " + Joiner.on(' ').join(residue);
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/InfoCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/InfoCommand.java
index 05e6b67..c53cdf5 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/InfoCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/InfoCommand.java
@@ -31,7 +31,6 @@
import com.google.devtools.build.lib.runtime.Command;
import com.google.devtools.build.lib.runtime.CommandEnvironment;
import com.google.devtools.build.lib.runtime.InfoItem;
-import com.google.devtools.build.lib.runtime.StarlarkOptionsParser;
import com.google.devtools.build.lib.runtime.commands.info.BlazeBinInfoItem;
import com.google.devtools.build.lib.runtime.commands.info.BlazeGenfilesInfoItem;
import com.google.devtools.build.lib.runtime.commands.info.BlazeTestlogsInfoItem;
@@ -66,7 +65,6 @@
import com.google.devtools.build.lib.util.DetailedExitCode;
import com.google.devtools.build.lib.util.ExitCode;
import com.google.devtools.build.lib.util.InterruptedFailureDetails;
-import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.io.OutErr;
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionDocumentationCategory;
@@ -191,16 +189,14 @@
}
}
- Pair<ImmutableList<String>, ImmutableList<String>> starlarkOptionsAndResidue =
- StarlarkOptionsParser.removeStarlarkOptions(optionsParsingResult.getResidue());
- ImmutableList<String> removedStarlarkOptions = starlarkOptionsAndResidue.getFirst();
- ImmutableList<String> residue = starlarkOptionsAndResidue.getSecond();
- if (!removedStarlarkOptions.isEmpty()) {
+ List<String> starlarkOptions = optionsParsingResult.getSkippedArgs();
+ List<String> residue = optionsParsingResult.getResidue();
+ if (!starlarkOptions.isEmpty()) {
env.getReporter()
.handle(
Event.warn(
"info command does not support starlark options. Ignoring options: "
- + removedStarlarkOptions));
+ + starlarkOptions));
}
env.getEventBus().post(new NoBuildEvent());
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParser.java b/src/main/java/com/google/devtools/common/options/OptionsParser.java
index a055cb0..6462f1a 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParser.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -23,6 +23,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSortedMap;
+import com.google.common.collect.Iterables;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.MoreCollectors;
import com.google.common.escape.Escaper;
@@ -695,12 +696,7 @@
Preconditions.checkNotNull(priority);
Preconditions.checkArgument(priority != OptionPriority.PriorityCategory.DEFAULT);
OptionsParserImplResult optionsParserImplResult = impl.parse(priority, sourceFunction, args);
- residue.addAll(optionsParserImplResult.getResidue());
- postDoubleDashResidue.addAll(optionsParserImplResult.postDoubleDashResidue);
- if (!allowResidue && !residue.isEmpty()) {
- String errorMsg = "Unrecognized arguments: " + Joiner.on(' ').join(residue);
- throw new OptionsParsingException(errorMsg);
- }
+ addResidueFromResult(optionsParserImplResult);
aliases.putAll(optionsParserImplResult.aliases);
}
@@ -726,10 +722,16 @@
args);
OptionsParserImplResult optionsParserImplResult =
impl.parseArgsAsExpansionOfOption(optionToExpand, o -> source, args);
- residue.addAll(optionsParserImplResult.getResidue());
- postDoubleDashResidue.addAll(optionsParserImplResult.postDoubleDashResidue);
- if (!allowResidue && !residue.isEmpty()) {
- String errorMsg = "Unrecognized arguments: " + Joiner.on(' ').join(residue);
+ addResidueFromResult(optionsParserImplResult);
+ }
+
+ private void addResidueFromResult(OptionsParserImplResult result) throws OptionsParsingException {
+ residue.addAll(result.getResidue());
+ postDoubleDashResidue.addAll(result.postDoubleDashResidue);
+ if (!allowResidue && (!getSkippedArgs().isEmpty() || !residue.isEmpty())) {
+ String errorMsg =
+ "Unrecognized arguments: "
+ + Joiner.on(' ').join(Iterables.concat(getSkippedArgs(), residue));
throw new OptionsParsingException(errorMsg);
}
}
@@ -771,7 +773,12 @@
}
@Override
- public List<String> getResidue() {
+ public ImmutableList<String> getSkippedArgs() {
+ return ImmutableList.copyOf(impl.getSkippedArgs());
+ }
+
+ @Override
+ public ImmutableList<String> getResidue() {
return ImmutableList.copyOf(residue);
}
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
index 41b6a3a..4be0136 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -28,6 +28,7 @@
import com.google.devtools.common.options.OptionValueDescription.ExpansionBundle;
import com.google.devtools.common.options.OptionsParser.OptionDescription;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
+import com.google.errorprone.annotations.Keep;
import java.lang.reflect.Constructor;
import java.util.ArrayList;
import java.util.Arrays;
@@ -37,6 +38,7 @@
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.Set;
import java.util.function.Function;
import java.util.stream.Collectors;
@@ -177,6 +179,33 @@
@Nullable private final String aliasFlag;
@Nullable private final Object conversionContext;
+ /**
+ * This option is used to collect skipped arguments while preserving the relative ordering between
+ * those given explicitly on the command line and those expanded by {@code ConfigExpander}. The
+ * field itself is not used for any purpose other than retrieving its {@link Option} annotation.
+ */
+ @Keep
+ @Option(
+ name = "skipped args",
+ allowMultiple = true,
+ defaultValue = "null",
+ documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+ effectTags = {OptionEffectTag.NO_OP},
+ help = "Only used internally by OptionsParserImpl")
+ private final List<String> skippedArgs = new ArrayList<>();
+
+ private static final OptionDefinition skippedArgsDefinition;
+
+ static {
+ try {
+ skippedArgsDefinition =
+ OptionDefinition.extractOptionDefinition(
+ OptionsParserImpl.class.getDeclaredField("skippedArgs"));
+ } catch (NoSuchFieldException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+
OptionsParserImpl(
OptionsData optionsData,
ArgsPreProcessor argsPreProcessor,
@@ -249,6 +278,7 @@
/** Implements {@link OptionsParser#canonicalize}. */
List<ParsedOptionDescription> asCanonicalizedListOfParsedOptions() {
return optionValues.keySet().stream()
+ .filter(k -> !Objects.equals(k, skippedArgsDefinition))
.map(optionDefinition -> optionValues.get(optionDefinition).getCanonicalInstances())
.flatMap(Collection::stream)
// Return the effective (canonical) options in the order they were applied.
@@ -388,6 +418,15 @@
return optionValues.get(optionDefinition) != null;
}
+ @SuppressWarnings("unchecked")
+ List<String> getSkippedArgs() {
+ OptionValueDescription value = optionValues.get(skippedArgsDefinition);
+ if (value == null) {
+ return ImmutableList.of();
+ }
+ return (List<String>) value.getValue();
+ }
+
/**
* Parses the args, and returns what it doesn't parse. May be called multiple times, and may be
* called recursively. The option's definition dictates how it reacts to multiple settings. By
@@ -435,19 +474,33 @@
arg = swapShorthandAlias(arg);
- if (containsSkippedPrefix(arg)) {
- unparsedArgs.add(arg);
- continue;
- }
-
if (arg.equals("--")) { // "--" means all remaining args aren't options
Iterators.addAll(unparsedPostDoubleDashArgs, argsIterator);
break;
}
- ParsedOptionDescription parsedOption =
- identifyOptionAndPossibleArgument(
- arg, argsIterator, priority, sourceFunction, implicitDependent, expandedFrom);
+ ParsedOptionDescription parsedOption;
+ if (containsSkippedPrefix(arg)) {
+ // Parse the skipped arg into a synthetic allowMultiple option to preserve its order
+ // relative to skipped args coming from expansions. Simply adding it to the residue would
+ // end up placing expanded skipped args after all explicitly given skipped args, which isn't
+ // correct.
+ parsedOption =
+ ParsedOptionDescription.newParsedOptionDescription(
+ skippedArgsDefinition,
+ arg,
+ arg,
+ new OptionInstanceOrigin(
+ priority,
+ sourceFunction.apply(skippedArgsDefinition),
+ implicitDependent,
+ expandedFrom),
+ conversionContext);
+ } else {
+ parsedOption =
+ identifyOptionAndPossibleArgument(
+ arg, argsIterator, priority, sourceFunction, implicitDependent, expandedFrom);
+ }
handleNewParsedOption(parsedOption);
priority = OptionPriority.nextOptionPriority(priority);
}
@@ -595,7 +648,11 @@
// As much as possible, we want the behaviors of these different types of flags to be
// identical, as this minimizes the number of edge cases, but we do not yet track these values
// in the same way.
- if (parsedOption.getImplicitDependent() == null) {
+
+ // Do not list the internal "skipped args" option that is only used to accumulate skipped
+ // arguments.
+ if (parsedOption.getImplicitDependent() == null
+ && !Objects.equals(parsedOption.getOptionDefinition(), skippedArgsDefinition)) {
// Log explicit options and expanded options in the order they are parsed (can be sorted
// later). This information is needed to correctly canonicalize flags.
parsedOptions.add(parsedOption);
@@ -638,7 +695,7 @@
} else if (arg.startsWith("--")) { // --long_option
int equalsAt = arg.indexOf('=');
- int nameStartsAt = arg.startsWith("--") ? 2 : 1;
+ int nameStartsAt = 2;
String name =
equalsAt == -1 ? arg.substring(nameStartsAt) : arg.substring(nameStartsAt, equalsAt);
if (name.trim().isEmpty()) {
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParsingResult.java b/src/main/java/com/google/devtools/common/options/OptionsParsingResult.java
index c3d2685..64fc9a3 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParsingResult.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParsingResult.java
@@ -32,9 +32,12 @@
OptionValueDescription getOptionValueDescription(String name);
/**
- * Returns an immutable copy of the residue, that is, the arguments that
- * have not been parsed.
+ * Returns an immutable copy of all arguments that were skipped because they matched a skipped
+ * prefix.
*/
+ List<String> getSkippedArgs();
+
+ /** Returns an immutable copy of the residue, that is, the arguments that have not been parsed. */
List<String> getResidue();
/**
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java
index 9c497cd..5382931 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java
@@ -13,11 +13,13 @@
// limitations under the License.
package com.google.devtools.build.lib.runtime;
+import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertThrows;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ListMultimap;
import com.google.devtools.build.lib.events.Event;
@@ -26,6 +28,7 @@
import com.google.devtools.common.options.OptionsBase;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
+import com.google.devtools.common.options.ParsedOptionDescription;
import com.google.devtools.common.options.TestOptions;
import java.util.Arrays;
import org.junit.Before;
@@ -54,7 +57,11 @@
ImmutableList.of(TestOptions.class, CommonCommandOptions.class, ClientOptions.class);
BlazeOptionHandlerTestHelper helper =
- new BlazeOptionHandlerTestHelper(optionsClasses, /* allowResidue= */ true);
+ new BlazeOptionHandlerTestHelper(
+ optionsClasses,
+ /* allowResidue= */ true,
+ /* aliasFlag= */ null,
+ /* skipStarlarkPrefixes= */ true);
eventHandler = helper.getEventHandler();
parser = helper.getOptionsParser();
optionHandler = helper.getOptionHandler();
@@ -336,6 +343,73 @@
}
@Test
+ public void testExpandConfigOptions_skippedArgsOrderPreserved() throws Exception {
+ ImmutableListMultimap<String, RcChunkOfArgs> rcContent =
+ ImmutableListMultimap.of(
+ "c0:config1",
+ new RcChunkOfArgs("rc1", ImmutableList.of("--//f=2", "--//f=3")),
+ "c0:config2b",
+ new RcChunkOfArgs("rc1", ImmutableList.of("--//f=6")),
+ "c0:config2",
+ new RcChunkOfArgs("rc1", ImmutableList.of("--config=config2a", "--config=config2b")),
+ "c0:config2a",
+ new RcChunkOfArgs("rc1", ImmutableList.of("--//f=5")));
+ parser.parse(
+ "--test_multiple_string=1",
+ "--//f=1",
+ "--test_multiple_string=2",
+ "--config=config1",
+ "--test_multiple_string=3",
+ "--//f=4",
+ "--test_multiple_string=4",
+ "--config=config2",
+ "--test_multiple_string=5",
+ "--//f=7",
+ "--test_multiple_string=6");
+ optionHandler.expandConfigOptions(eventHandler, rcContent);
+
+ assertThat(parser.getSkippedArgs())
+ .containsExactly(
+ "--//f=1", "--//f=2", "--//f=3", "--//f=4", "--//f=5", "--//f=6", "--//f=7")
+ .inOrder();
+
+ // Verify that the order of non-skipped args is as expected and that skipped args are not
+ // reported as parsed.
+ assertThat(
+ parser.asListOfCanonicalOptions().stream()
+ .map(ParsedOptionDescription::getCanonicalForm)
+ .collect(toImmutableList()))
+ .containsExactly(
+ "--test_multiple_string=1",
+ "--test_multiple_string=2",
+ "--config=config1",
+ "--test_multiple_string=3",
+ "--test_multiple_string=4",
+ "--config=config2",
+ "--config=config2a",
+ "--config=config2b",
+ "--test_multiple_string=5",
+ "--test_multiple_string=6")
+ .inOrder();
+ assertThat(
+ parser.asCompleteListOfParsedOptions().stream()
+ .map(ParsedOptionDescription::getCanonicalForm)
+ .collect(toImmutableList()))
+ .containsExactly(
+ "--test_multiple_string=1",
+ "--test_multiple_string=2",
+ "--config=config1",
+ "--test_multiple_string=3",
+ "--test_multiple_string=4",
+ "--config=config2",
+ "--config=config2a",
+ "--config=config2b",
+ "--test_multiple_string=5",
+ "--test_multiple_string=6")
+ .inOrder();
+ }
+
+ @Test
public void testUndefinedConfig() throws Exception {
parser.parse("--config=invalid");
OptionsParsingException e =
@@ -364,6 +438,24 @@
}
@Test
+ public void testParseOptions_disallowResidue_skippedArgsLeadToFailure() throws Exception {
+ ImmutableList<Class<? extends OptionsBase>> optionsClasses =
+ ImmutableList.of(TestOptions.class, CommonCommandOptions.class, ClientOptions.class);
+
+ BlazeOptionHandlerTestHelper helper =
+ new BlazeOptionHandlerTestHelper(
+ optionsClasses,
+ /* allowResidue= */ false,
+ /* aliasFlag= */ null,
+ /* skipStarlarkPrefixes= */ true);
+ OptionsParser parser = helper.getOptionsParser();
+
+ OptionsParsingException e =
+ assertThrows(OptionsParsingException.class, () -> parser.parse("--//f=1"));
+ assertThat(e).hasMessageThat().isEqualTo("Unrecognized arguments: --//f=1");
+ }
+
+ @Test
public void testParseOptions_explicitOption() {
optionHandler.parseOptions(
ImmutableList.of("c0", "--test_multiple_string=explicit"), eventHandler);
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/FlagAliasTest.java b/src/test/java/com/google/devtools/build/lib/runtime/FlagAliasTest.java
index 3ff89c6..5f675e8 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/FlagAliasTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/FlagAliasTest.java
@@ -116,7 +116,7 @@
ImmutableList.of(
"c0", "--rc_source=/somewhere/.blazerc", "--flag_alias=foo=//bar", "--foo");
optionHandler.parseOptions(args, eventHandler);
- assertThat(parser.getResidue()).contains("--//bar");
+ assertThat(parser.getSkippedArgs()).contains("--//bar");
}
@Test
@@ -142,9 +142,9 @@
"--foo",
"--flag_alias=foo=//baz",
"--foo");
- ImmutableList<String> expectedResidue = ImmutableList.of("--//bar", "--//baz");
+ ImmutableList<String> expectedSkippedArgs = ImmutableList.of("--//bar", "--//baz");
optionHandler.parseOptions(args, eventHandler);
- assertThat(parser.getResidue()).isEqualTo(expectedResidue);
+ assertThat(parser.getSkippedArgs()).isEqualTo(expectedSkippedArgs);
}
@Test
@@ -155,9 +155,9 @@
"--default_override=0:c0=--flag_alias=foo=//bar",
"--default_override=0:c0=--foo",
"--rc_source=/somewhere/.blazerc");
- ImmutableList<String> expectedResidue = ImmutableList.of("--//bar");
+ ImmutableList<String> expectedSkippedArgs = ImmutableList.of("--//bar");
optionHandler.parseOptions(args, eventHandler);
- assertThat(parser.getResidue()).isEqualTo(expectedResidue);
+ assertThat(parser.getSkippedArgs()).isEqualTo(expectedSkippedArgs);
}
@Test
@@ -168,9 +168,9 @@
"--default_override=0:c0=--flag_alias=foo=//bar",
"--rc_source=/somewhere/.blazerc",
"--foo");
- ImmutableList<String> expectedResidue = ImmutableList.of("--//bar");
+ ImmutableList<String> expectedSkippedArgs = ImmutableList.of("--//bar");
optionHandler.parseOptions(args, eventHandler);
- assertThat(parser.getResidue()).isEqualTo(expectedResidue);
+ assertThat(parser.getSkippedArgs()).isEqualTo(expectedSkippedArgs);
}
@Test
@@ -178,9 +178,9 @@
ImmutableList<String> args =
ImmutableList.of(
"c0", "--rc_source=/somewhere/.blazerc", "--flag_alias=foo=//bar", "--foo=7");
- ImmutableList<String> expectedResidue = ImmutableList.of("--//bar=7");
+ ImmutableList<String> expectedSkippedArgs = ImmutableList.of("--//bar=7");
optionHandler.parseOptions(args, eventHandler);
- assertThat(parser.getResidue()).isEqualTo(expectedResidue);
+ assertThat(parser.getSkippedArgs()).isEqualTo(expectedSkippedArgs);
}
// Regression test for b/172453517
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkOptionsParsingTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkOptionsParsingTest.java
index 6579eec..b42aee4 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkOptionsParsingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkOptionsParsingTest.java
@@ -23,9 +23,7 @@
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
import com.google.devtools.build.lib.pkgcache.TargetParsingCompleteEvent;
-import com.google.devtools.build.lib.runtime.StarlarkOptionsParser;
import com.google.devtools.build.lib.starlark.util.StarlarkOptionsTestCase;
-import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.common.options.OptionsParsingException;
import com.google.devtools.common.options.OptionsParsingResult;
import java.util.ArrayList;
@@ -160,10 +158,11 @@
public void testSingleDash_notAllowed() throws Exception {
writeBasicIntFlag();
- OptionsParsingResult result = parseStarlarkOptions("-//test:my_int_setting=666");
-
- assertThat(result.getStarlarkOptions()).isEmpty();
- assertThat(result.getResidue()).containsExactly("-//test:my_int_setting=666");
+ OptionsParsingException e =
+ assertThrows(
+ OptionsParsingException.class,
+ () -> parseStarlarkOptions("-//test:my_int_setting=666"));
+ assertThat(e).hasMessageThat().isEqualTo("Invalid options syntax: -//test:my_int_setting=666");
}
// test --non_flag_setting=value
@@ -380,35 +379,6 @@
.isEqualTo(StarlarkInt.of(15));
}
- @Test
- public void testRemoveStarlarkOptionsWorks() throws Exception {
- Pair<ImmutableList<String>, ImmutableList<String>> residueAndStarlarkOptions =
- StarlarkOptionsParser.removeStarlarkOptions(
- ImmutableList.of(
- "--//local/starlark/option",
- "--//local/starlark/option=with_value",
- "--@some_repo//external/starlark/option",
- "--@some_repo//external/starlark/option=with_value",
- "--@//main/repo/option",
- "--@//main/repo/option=with_value",
- "some-random-residue",
- "--mangled//external/starlark/option",
- "--mangled//external/starlark/option=with_value"));
- assertThat(residueAndStarlarkOptions.getFirst())
- .containsExactly(
- "--//local/starlark/option",
- "--//local/starlark/option=with_value",
- "--@some_repo//external/starlark/option",
- "--@some_repo//external/starlark/option=with_value",
- "--@//main/repo/option",
- "--@//main/repo/option=with_value");
- assertThat(residueAndStarlarkOptions.getSecond())
- .containsExactly(
- "some-random-residue",
- "--mangled//external/starlark/option",
- "--mangled//external/starlark/option=with_value");
- }
-
/**
* When Starlark flags are only set as flags, they shouldn't produce {@link
* TargetParsingCompleteEvent}s. That's intended to communicate (to the build event protocol)
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/util/StarlarkOptionsTestCase.java b/src/test/java/com/google/devtools/build/lib/starlark/util/StarlarkOptionsTestCase.java
index ba64f41..d7baf5f 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/util/StarlarkOptionsTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/starlark/util/StarlarkOptionsTestCase.java
@@ -63,8 +63,8 @@
}
protected OptionsParsingResult parseStarlarkOptions(String options) throws Exception {
- starlarkOptionsParser.setResidueForTesting(Arrays.asList(options.split(" ")));
- starlarkOptionsParser.parse(new StoredEventHandler());
+ starlarkOptionsParser.parseGivenArgs(
+ new StoredEventHandler(), Arrays.asList(options.split(" ")));
return starlarkOptionsParser.getNativeOptionsParserFortesting();
}
diff --git a/src/test/py/bazel/BUILD b/src/test/py/bazel/BUILD
index a59f023..0e58a53 100644
--- a/src/test/py/bazel/BUILD
+++ b/src/test/py/bazel/BUILD
@@ -282,3 +282,9 @@
":test_base",
],
)
+
+py_test(
+ name = "starlark_options_test",
+ srcs = ["starlark_options_test.py"],
+ deps = [":test_base"],
+)
diff --git a/src/test/py/bazel/starlark_options_test.py b/src/test/py/bazel/starlark_options_test.py
new file mode 100644
index 0000000..f6433b5
--- /dev/null
+++ b/src/test/py/bazel/starlark_options_test.py
@@ -0,0 +1,82 @@
+# 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.
+
+import unittest
+
+from src.test.py.bazel import test_base
+
+
+class StarlarkOptionsTest(test_base.TestBase):
+
+ def testCanOverrideStarlarkFlagInBazelrcConfigStanza(self):
+ self.ScratchFile("WORKSPACE.bazel")
+ self.ScratchFile("bazelrc", [
+ "build:red --//f:color=red",
+ ])
+ self.ScratchFile("f/BUILD.bazel", [
+ 'load(":f.bzl", "color", "r")',
+ "color(",
+ ' name = "color",',
+ ' build_setting_default = "white",',
+ ")",
+ 'r(name = "r")',
+ ])
+ self.ScratchFile("f/f.bzl", [
+ 'ColorValue = provider("color")',
+ "def _color_impl(ctx):",
+ " return [ColorValue(color = ctx.build_setting_value)]",
+ "color = rule(",
+ " implementation = _color_impl,",
+ "build_setting = config.string(flag = True),",
+ ")",
+ "def _r_impl(ctx):",
+ " print(ctx.attr._color[ColorValue].color)",
+ " return [DefaultInfo()]",
+ "r = rule(",
+ " implementation = _r_impl,",
+ ' attrs = {"_color": attr.label(default = "//f:color")},',
+ ")",
+ ])
+
+ exit_code, _, stderr = self.RunBazel([
+ "--bazelrc=bazelrc",
+ "build",
+ "--nobuild",
+ "//f:r",
+ "--config=red",
+ "--//f:color=green",
+ ])
+ self.AssertExitCode(exit_code, 0, stderr)
+ self.assertTrue(
+ any("/f/f.bzl:9:10: green" in line for line in stderr),
+ "\n".join(stderr),
+ )
+
+ exit_code, _, stderr = self.RunBazel([
+ "--bazelrc=bazelrc",
+ "build",
+ "--nobuild",
+ "//f:r",
+ "--//f:color=green",
+ "--config=red",
+ ])
+ self.AssertExitCode(exit_code, 0, stderr)
+ self.assertTrue(
+ any("/f/f.bzl:9:10: red" in line for line in stderr),
+ "\n".join(stderr),
+ )
+
+
+if __name__ == "__main__":
+ unittest.main()
