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/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();
}
