Repository: bazelbuild/bazel
Issue: Support custom skylark build flags in external dependencies
Labels: P2
team-Configurability

### Description of the problem / feature request:

The implementation of Skylark support for custom build flags
(#5577) does not work for custom build flags in external repositories.

### Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Given the following setup:
```
mkdir my-external
touch my-external/WORKSPACE nothing.txt myext.txt top.txt

cat >my-external/BUILD.bazel <<EOF
load("@bazel_skylib//rules:common_settings.bzl", "bool_flag")
bool_flag(
    name = "flag_in_myext",
    build_setting_default = False, visibility = ["//visibility:public"],
)
EOF

cat >BUILD.bazel <<EOF
load("@bazel_skylib//rules:common_settings.bzl", "bool_flag")
bool_flag(
    name = "flag_at_top",
    build_setting_default = False,
)
alias(
    name = "alias_for_flag_in_myext",
    actual = "@myext//:flag_in_myext",
)
config_setting(
    name = "is_flag_at_top_enabled",
    flag_values = {":flag_at_top": "True"},
)
config_setting(
    name = "is_flag_in_myext_enabled",
    flag_values = {":alias_for_flag_in_myext": "True"},
)
filegroup(
    name = "everything",
    srcs = select({
        ":is_flag_at_top_enabled": ["top.txt"],
        ":is_flag_in_myext_enabled": ["myext.txt"],
        "//conditions:default": ["nothing.text"],
    }),
)
EOF

cat >WORKSPACE <<EOF
local_repository(name = "myext", path = "my-external")
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
http_archive(
    name = "bazel_skylib",
    url = "https://github.com/bazelbuild/bazel-skylib/archive/b113ed5d05ccddee3093bb157b9b02ab963c1c32.zip",
    sha256 = "cea47b31962206b7ebf2088f749243868d5d9305273205bdd8651567d3e027fc",
    strip_prefix = "bazel-skylib-b113ed5d05ccddee3093bb157b9b02ab963c1c32",
)
EOF
```
the output looks like
```
$ bazel cquery 'kind("source file", deps(:everything))'
//:nothing.text (null)
$ bazel cquery 'kind("source file", deps(:everything))' --//:flag_at_top
//:top.txt (null)
$ bazel cquery 'kind("source file", deps(:everything))' --@myext//:flag_in_myext
ERROR: Unrecognized option: --@myext//:flag_in_myext
$ bazel cquery 'kind("source file", deps(:everything))' --//:alias_for_flag_in_myext
ERROR: Unrecognized option: //:alias_for_flag_in_myext
```
I would expect the bottom two cases to work and output `//:myext.txt (null)`. The alias would be very convenient for the user to shorten the command line.

### What operating system are you running Bazel on?

Linux

### What's the output of `bazel info release`?

`release 1.0.0`

###  Have you found anything relevant by searching the web?

@gregestren suggested reporting this bug/feature request as the core functionality was implemented in #5577. 


Issue comments:
This is worth at least clarifying which part of the code base is failing to make the connection. From there we can prioritize the right fix. 
PR to support Starlark-defined flags starting with `--@`: https://github.com/bazelbuild/bazel/pull/10052

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
index 027fa3432a4401..53d3cdb3ee0e28 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java
@@ -653,6 +653,7 @@ private OptionsParser createOptionsParser(BlazeCommand command)
         OptionsParser.builder()
             .optionsData(optionsData)
             .skippedPrefix("--//")
+            .skippedPrefix("--@")
             .allowResidue(annotation.allowResidue())
             .build();
     return parser;
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParser.java b/src/main/java/com/google/devtools/common/options/OptionsParser.java
index c495b7f98e567c..88253ff5f880f4 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParser.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -183,7 +183,7 @@ public Builder argsPreProcessor(ArgsPreProcessor preProcessor) {
     }
 
     /** Any flags with this prefix will be skipped during processing. */
-    public Builder skippedPrefix(@Nullable String skippedPrefix) {
+    public Builder skippedPrefix(String skippedPrefix) {
       this.implBuilder.skippedPrefix(skippedPrefix);
       return this;
     }
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
index d8b47e4a20b44b..a753a9e76deb4e 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -47,7 +47,7 @@ class OptionsParserImpl {
   static final class Builder {
     private OptionsData optionsData;
     private ArgsPreProcessor argsPreProcessor = args -> args;
-    @Nullable private String skippedPrefix;
+    private ArrayList<String> skippedPrefixes = new ArrayList<>();
     private boolean ignoreInternalOptions = true;
 
     /** Set the {@link OptionsData} to be used in this instance. */
@@ -63,8 +63,8 @@ public Builder argsPreProcessor(ArgsPreProcessor preProcessor) {
     }
 
     /** Any flags with this prefix will be skipped during processing. */
-    public Builder skippedPrefix(@Nullable String skippedPrefix) {
-      this.skippedPrefix = skippedPrefix;
+    public Builder skippedPrefix(String skippedPrefix) {
+      this.skippedPrefixes.add(skippedPrefix);
       return this;
     }
 
@@ -77,7 +77,7 @@ public Builder ignoreInternalOptions(boolean ignoreInternalOptions) {
     /** Returns a newly-initialized {@link OptionsParserImpl}. */
     public OptionsParserImpl build() {
       return new OptionsParserImpl(
-          this.optionsData, this.argsPreProcessor, this.skippedPrefix, this.ignoreInternalOptions);
+          this.optionsData, this.argsPreProcessor, this.skippedPrefixes, this.ignoreInternalOptions);
     }
   }
 
@@ -124,17 +124,17 @@ public static Builder builder() {
 
   private final List<String> warnings = new ArrayList<>();
   private final ArgsPreProcessor argsPreProcessor;
-  @Nullable private final String skippedPrefix;
+  private final List<String> skippedPrefixes;
   private final boolean ignoreInternalOptions;
 
   OptionsParserImpl(
       OptionsData optionsData,
       ArgsPreProcessor argsPreProcessor,
-      @Nullable String skippedPrefix,
+      List<String> skippedPrefixes,
       boolean ignoreInternalOptions) {
     this.optionsData = optionsData;
     this.argsPreProcessor = argsPreProcessor;
-    this.skippedPrefix = skippedPrefix;
+    this.skippedPrefixes = skippedPrefixes;
     this.ignoreInternalOptions = ignoreInternalOptions;
   }
 
@@ -145,10 +145,13 @@ OptionsData getOptionsData() {
 
   /** Returns a {@link Builder} that is configured the same as this parser. */
   Builder toBuilder() {
-    return builder()
+    Builder builder = builder()
         .optionsData(optionsData)
-        .argsPreProcessor(argsPreProcessor)
-        .skippedPrefix(skippedPrefix);
+        .argsPreProcessor(argsPreProcessor);
+    for (String skippedPrefix : skippedPrefixes) {
+      builder.skippedPrefix(skippedPrefix);
+    }
+    return builder;
   }
 
   /** Implements {@link OptionsParser#asCompleteListOfParsedOptions()}. */
@@ -350,7 +353,7 @@ private ResidueAndPriority parse(
         continue; // not an option arg
       }
 
-      if (skippedPrefix != null && arg.startsWith(skippedPrefix)) {
+      if (skippedPrefixes.stream().anyMatch(prefix -> arg.startsWith(prefix))) {
         unparsedArgs.add(arg);
         continue;
       }


Test patch:
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 cce15bc6571645..26e375d3195414 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
@@ -161,6 +161,19 @@ public void testFlagSpaceValueForm() throws Exception {
     assertThat(result.getResidue()).isEmpty();
   }
 
+  // test --@workspace//flag=value
+  @Test
+  public void testFlagNameWithWorkspace() throws Exception {
+    writeBasicIntFlag();
+    rewriteWorkspace("workspace(name = 'starlark_options_test')");
+
+    OptionsParsingResult result = parseStarlarkOptions("--@starlark_options_test//test:my_int_setting=666");
+
+    assertThat(result.getStarlarkOptions()).hasSize(1);
+    assertThat(result.getStarlarkOptions().get("@starlark_options_test//test:my_int_setting")).isEqualTo(666);
+    assertThat(result.getResidue()).isEmpty();
+  }
+
   // test --fake_flag=value
   @Test
   public void testBadFlag_equalsForm() throws Exception {
diff --git a/src/test/shell/integration/starlark_configurations_test.sh b/src/test/shell/integration/starlark_configurations_test.sh
index a8697d49af88e7..a9bce39f6d3694 100755
--- a/src/test/shell/integration/starlark_configurations_test.sh
+++ b/src/test/shell/integration/starlark_configurations_test.sh
@@ -61,8 +61,14 @@ add_to_bazelrc "build --package_path=%workspace%"
 #### HELPER FXNS #######################################################
 
 function write_build_setting_bzl() {
- cat > $pkg/build_setting.bzl <<EOF
+  declare -r at_workspace="${1:-}"
+
+  cat > $pkg/provider.bzl <<EOF
 BuildSettingInfo = provider(fields = ['name', 'value'])
+EOF
+
+  cat > $pkg/build_setting.bzl <<EOF
+load("@${WORKSPACE_NAME}//$pkg:provider.bzl", "BuildSettingInfo")
 
 def _build_setting_impl(ctx):
   return [BuildSettingInfo(name = ctx.attr.name, value = ctx.build_setting_value)]
@@ -74,7 +80,7 @@ drink_attribute = rule(
 EOF
 
   cat > $pkg/rules.bzl <<EOF
-load("//$pkg:build_setting.bzl", "BuildSettingInfo")
+load("@${WORKSPACE_NAME}//$pkg:provider.bzl", "BuildSettingInfo")
 
 def _impl(ctx):
   _type_name = ctx.attr._type[BuildSettingInfo].name
@@ -88,8 +94,8 @@ def _impl(ctx):
 drink = rule(
   implementation = _impl,
   attrs = {
-    "_type":attr.label(default = Label("//$pkg:type")),
-    "_temp":attr.label(default = Label("//$pkg:temp")),
+    "_type":attr.label(default = Label("$at_workspace//$pkg:type")),
+    "_temp":attr.label(default = Label("$at_workspace//$pkg:temp")),
   },
   fragments = ["java"],
 )
@@ -101,8 +107,16 @@ load("//$pkg:rules.bzl", "drink")
 
 drink(name = 'my_drink')
 
-drink_attribute(name = 'type', build_setting_default = 'unknown')
-drink_attribute(name = 'temp', build_setting_default = 'unknown')
+drink_attribute(
+  name = 'type',
+  build_setting_default = 'unknown',
+  visibility = ['//visibility:public'],
+)
+drink_attribute(
+  name = 'temp',
+  build_setting_default = 'unknown',
+  visibility = ['//visibility:public'],
+)
 EOF
 }
 
@@ -133,6 +147,19 @@ function test_set_flag() {
   expect_log "type=coffee"
 }
 
+function test_set_flag_with_workspace_name() {
+  local -r pkg=$FUNCNAME
+  mkdir -p $pkg
+
+  write_build_setting_bzl "@${WORKSPACE_NAME}"
+
+  bazel build //$pkg:my_drink --@"${WORKSPACE_NAME}"//$pkg:type="coffee" \
+    --experimental_build_setting_api > output 2>"$TEST_log" \
+    || fail "Expected success"
+
+  expect_log "type=coffee"
+}
+
 function test_starlark_and_native_flag() {
   local -r pkg=$FUNCNAME
   mkdir -p $pkg