commit 9f2542f91e99b40de65506c8f4768ec0003a4e5d
author Fabian Meumertzheim <fabian@meumertzhe.im> Tue Aug 16 08:29:35 2022 -0700
committer Copybara-Service <copybara-worker@google.com> Tue Aug 16 08:30:55 2022 -0700
tree e13d842bc52cd9561225bd94fc24aac76cec4112
parent 41f7f8aa9a7cc714fe5f12a346b91e04ec9d7561

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

src/test/java/com/google/devtools/build/lib/starlark/StarlarkOptionsParsingTest.java

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)