diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD index 9f991fa0ad734a..0a917981d65762 100644 --- a/src/main/java/com/google/devtools/build/lib/BUILD +++ b/src/main/java/com/google/devtools/build/lib/BUILD @@ -306,6 +306,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/cmdline", "//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/cmdline:LabelValidator", "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/exec:bin_tools", 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 598c13d9c678a7..ce28e3a2f7ac12 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 @@ private Target loadBuildSetting( /** * Separates out any Starlark options from the given list * + * 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> removeStarlarkO ImmutableList.Builder keep = ImmutableList.builder(); ImmutableList.Builder 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 2ed08a10b29ddf..177d486d96d751 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 @@ 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", 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 68a0dad13fdb7b..d6256f5fb010bc 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,14 +17,19 @@ 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.skylark.util.StarlarkOptionsTestCase; +import com.google.devtools.build.lib.runtime.StarlarkOptionsParser; +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; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** Unit test for the {@code StarlarkOptionsParser}. */ +/** + * Unit test for the {@code StarlarkOptionsParser}. + */ @RunWith(JUnit4.class) public class StarlarkOptionsParsingTest extends StarlarkOptionsTestCase { @@ -314,4 +319,21 @@ public void testOptionsAreParsedWithBuildTestsOnly() throws Exception { assertThat(result.getStarlarkOptions().get("//test:my_int_setting")).isEqualTo(15); } + + @Test + public void testRemoveStarlarkOptionsWorks() throws Exception { + Pair, ImmutableList> 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"); + } }