Update sorting out of starlark flags to also identify external repo s…

…tarlark flags

Fixes #11506

Closes #11510.

PiperOrigin-RevId: 313817350

src/main/java/com/google/devtools/build/lib/BUILD

@@ -304,6 +304,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/buildeventstream/transports",
         "//src/main/java/com/google/devtools/build/lib/clock",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
+        "//src/main/java/com/google/devtools/build/lib/cmdline:LabelValidator",
         "//src/main/java/com/google/devtools/build/lib/collect",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/concurrent",

src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java

@@ -23,6 +23,8 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
+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.ExtendedEventHandler;
 import com.google.devtools.build.lib.events.Reporter;
@@ -219,6 +221,11 @@ private Target loadBuildSetting(
   /**
    * 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.
@@ -228,7 +235,25 @@ public static Pair<ImmutableList<String>, ImmutableList<String>> removeStarlarkO
     ImmutableList.Builder<String> keep = ImmutableList.builder();
     ImmutableList.Builder<String> remove = ImmutableList.builder();
     for (String name : list) {
-      ((name.startsWith("--//") || name.startsWith("--no//")) ? remove : keep).add(name);
+      // 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 (name.startsWith("no")) {
+        potentialStarlarkFlag = potentialStarlarkFlag.substring(2);
+      }
+      // 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());
   }

src/test/java/com/google/devtools/build/lib/skylark/BUILD

@@ -24,6 +24,7 @@ java_test(
         "//src/test/java/com/google/devtools/build/lib:test_runner",
     ],
     deps = [
+        "//src/main/java/com/google/devtools/build/lib:runtime",
         "//src/main/java/com/google/devtools/build/lib:syntax",
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/analysis:actions/parameter_file_write_action",

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

@@ -17,7 +17,10 @@
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertThrows;
 
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.runtime.StarlarkOptionsParser;
 import com.google.devtools.build.lib.skylark.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 org.junit.Test;
@@ -314,4 +317,23 @@ public void testOptionsAreParsedWithBuildTestsOnly() throws Exception {
 
     assertThat(result.getStarlarkOptions().get("//test:my_int_setting")).isEqualTo(15);
   }
+
+  @Test
+  public void testRemoveStarlarkOptionsWorks() throws Exception {
+    Pair<ImmutableList<String>, ImmutableList<String>> residueAndStarlarkOptions =
+        StarlarkOptionsParser.removeStarlarkOptions(
+            ImmutableList.of(
+                "--//local/starlark/option",
+                "--@some_repo//external/starlark/option",
+                "--@//main/repo/option",
+                "some-random-residue",
+                "--mangled//external/starlark/option"));
+    assertThat(residueAndStarlarkOptions.getFirst())
+        .containsExactly(
+            "--//local/starlark/option",
+            "--@some_repo//external/starlark/option",
+            "--@//main/repo/option");
+    assertThat(residueAndStarlarkOptions.getSecond())
+        .containsExactly("some-random-residue", "--mangled//external/starlark/option");
+  }
 }