Update sorting out of starlark flags to also identify external repo starlark flags
Fixes #11506
Closes #11510.
PiperOrigin-RevId: 313817350
diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD
index 9f991fa..c4165ca 100644
--- a/src/main/java/com/google/devtools/build/lib/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/BUILD
@@ -304,6 +304,7 @@
"//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",
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 598c13d..7d6e937 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
@@ -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 @@
/**
* 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 @@
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());
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/BUILD b/src/test/java/com/google/devtools/build/lib/skylark/BUILD
index 2ed08a1..177d486 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/skylark/BUILD
@@ -24,6 +24,7 @@
"//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",
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 68a0dad..48a58d7 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
@@ -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 @@
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");
+ }
}
