fix main repo starlark options parsing - now flags passed on the command line as --@main_workspace//flag and --//flag will both parse to --//flag. Before this CL, the former maintained its workspace prefix and we would get different entries for these two formats.

juliexxia · katre · commit 80c59dea59d4 · 2021-07-13T10:40:35.000-04:00
work towards bazelbuild#11128 PiperOrigin-RevId: 350575360
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
@@ -109,6 +109,8 @@ public void parse(ExtendedEventHandler eventHandler) throws OptionsParsingExcept
     ImmutableMap.Builder<String, Object> parsedOptions = new ImmutableMap.Builder<>();
     for (Map.Entry<String, Pair<String, Target>> option : unparsedOptions.entrySet()) {
       String loadedFlag = option.getKey();
+      // String loadedFlag =
+      // Label.parseAbsoluteUnchecked(option.getKey()).getDefaultCanonicalForm();
       String unparsedValue = option.getValue().first;
       Target buildSettingTarget = option.getValue().second;
       BuildSetting buildSetting =
@@ -155,7 +157,11 @@ private void parseArg(
     if (value != null) {
       // --flag=value or -flag=value form
       Target buildSettingTarget = loadBuildSetting(name, nativeOptionsParser, eventHandler);
-      unparsedOptions.put(name, new Pair<>(value, buildSettingTarget));
+      // Use the unambiguous canonical form to ensure we don't have
+      // duplicate options getting into the starlark options map.
+      unparsedOptions.put(
+          buildSettingTarget.getLabel().getDefaultCanonicalForm(),
+          new Pair<>(value, buildSettingTarget));
     } else {
       boolean booleanValue = true;
       // check --noflag form
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
@@ -45,18 +45,40 @@ public void testFlagEqualsValueForm() throws Exception {
     assertThat(result.getResidue()).isEmpty();
   }
 
-  // test --@workspace//flag=value
+  // test --@main_workspace//flag=value parses out to //flag=value
+  // test --@other_workspace//flag=value parses out to @other_workspace//flag=value
   @Test
   public void testFlagNameWithWorkspace() throws Exception {
     writeBasicIntFlag();
-    rewriteWorkspace("workspace(name = 'starlark_options_test')");
+    scratch.file("test/repo2/WORKSPACE");
+    scratch.file(
+        "test/repo2/defs.bzl",
+        "def _impl(ctx):",
+        "  pass",
+        "my_flag = rule(",
+        "  implementation = _impl,",
+        "  build_setting = config.int(flag = True),",
+        ")");
+    scratch.file(
+        "test/repo2/BUILD",
+        "load(':defs.bzl', 'my_flag')",
+        "my_flag(name = 'flag2', build_setting_default=2)");
+
+    rewriteWorkspace(
+        "workspace(name = 'starlark_options_test')",
+        "local_repository(",
+        "  name = 'repo2',",
+        "  path = 'test/repo2',",
+        ")");
 
     OptionsParsingResult result =
-        parseStarlarkOptions("--@starlark_options_test//test:my_int_setting=666");
+        parseStarlarkOptions(
+            "--@starlark_options_test//test:my_int_setting=666 --@repo2//:flag2=222");
 
-    assertThat(result.getStarlarkOptions()).hasSize(1);
-    assertThat(result.getStarlarkOptions().get("@starlark_options_test//test:my_int_setting"))
+    assertThat(result.getStarlarkOptions()).hasSize(2);
+    assertThat(result.getStarlarkOptions().get("//test:my_int_setting"))
         .isEqualTo(StarlarkInt.of(666));
+    assertThat(result.getStarlarkOptions().get("@repo2//:flag2")).isEqualTo(StarlarkInt.of(222));
     assertThat(result.getResidue()).isEmpty();
   }
 
