Support --no@//:bool_flag on the command line.

[ghost]

This applies label canonicalization to starlark flags having a --no
prefix.

Relates to #11128.

[gregestren]

Oh, nice! While I like this contribution, I vaguely recall some effort to move away from the --no<flag> syntax for all flags (both Starlark and native) as a best practice.

I may be wrong on that, so as next step let me dig into discussion history and see if I can confirm/deny my claim.

[ghost]

Aha, good to know. Thanks for looking into it!

FYI, without this we can crash Bazel (even at master/HEAD) by mixing --no//:flag and --no@//:flag on the command line.

$ bazelisk build --no//:bool_flag --no@//:bool_flag
INFO: Invocation ID: 960e93ef-d3ac-416d-8fb8-b53d90d430ff
WARNING: Usage: bazel build <options> <targets>.
Invoke `bazel help build` for full description of usage and options.
Your request is correct, but requested an empty set of targets. Nothing will be built.
FATAL: bazel crashed due to an internal error. Printing stack trace:
java.lang.IllegalStateException: Duplicate key //:bool_flag (attempted merging values false and false)
        at java.base/java.util.stream.Collectors.duplicateKeyException(Unknown Source)
        at java.base/java.util.stream.Collectors.lambda$uniqKeysMapAccumulator$1(Unknown Source)
        at java.base/java.util.stream.ReduceOps$3ReducingSink.accept(Unknown Source)
        at com.google.common.collect.CollectSpliterators$1WithCharacteristics.lambda$forEachRemaining$1(CollectSpliterators.java:67)
        at java.base/java.util.stream.Streams$RangeIntSpliterator.forEachRemaining(Unknown Source)
        at com.google.common.collect.CollectSpliterators$1WithCharacteristics.forEachRemaining(CollectSpliterators.java:67)
        at java.base/java.util.stream.AbstractPipeline.copyInto(Unknown Source)
        at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(Unknown Source)
        at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(Unknown Source)
        at java.base/java.util.stream.AbstractPipeline.evaluate(Unknown Source)
        at java.base/java.util.stream.ReferencePipeline.collect(Unknown Source)
        at com.google.devtools.build.lib.analysis.config.BuildOptions.labelizeStarlarkOptions(BuildOptions.java:85)
        at com.google.devtools.build.lib.analysis.config.BuildOptions.of(BuildOptions.java:147)
        at com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider.createBuildOptions(ConfiguredRuleClassProvider.java:726)
        at com.google.devtools.build.lib.runtime.BlazeRuntime.createBuildOptions(BlazeRuntime.java:749)
        at com.google.devtools.build.lib.buildtool.BuildTool.buildTargets(BuildTool.java:136)
        at com.google.devtools.build.lib.buildtool.BuildTool.processRequest(BuildTool.java:401)
        at com.google.devtools.build.lib.runtime.commands.BuildCommand.exec(BuildCommand.java:97)
        at com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.execExclusively(BlazeCommandDispatcher.java:579)
        at com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.exec(BlazeCommandDispatcher.java:231)
        at com.google.devtools.build.lib.server.GrpcServerImpl.executeCommand(GrpcServerImpl.java:543)
        at com.google.devtools.build.lib.server.GrpcServerImpl.lambda$run$1(GrpcServerImpl.java:606)
        at io.grpc.Context$1.run(Context.java:579)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
        at java.base/java.lang.Thread.run(Unknown Source)

[gregestren]

Okay, so this simply extend the existing --no support to support workspace prefixes too. It's reasonable to treat that consistently, and not crash.

My comments about --no<flag> above was indeed wrong. There was a drive to remove --no_<flag> (i.e. a flag that starts with no and an underscore). But that's unrelated.

There was also discussion about extra subtleties in the Starlark vs. native flag parser. Julie Xia, who best knew this and has since moved onto other things, wrote:

The starlark options parser operates on the residue of the regular options parser. Take the cases of bazel build //foo --//bar value vs bazel build //foo --//bar=value vs bazel build //foo --//bool/typed/flag //some/other/target/to/build. The regular options parser knows to put args that start with "--//" in residue to pass to the starlark options parser, but has no way to know when it should also grab the next arg. It can't do this without loading the starlark option to figure out what type it is. Currently the regular options parser makes no effort to figure out if it needs to grab the next arg, it just doesn't.

So even though the starlark options parser can support it, the regular options parser can't formulate the right kind of residue to give to the starlark options parser. So right now, remove support for the space syntax from the starlark options parser. We can always reimplement correctly if there's a need. This simplifies the whole thing nicely and I don't see a major argument for supporting the space syntax right now.

That led to 28442ae.

If I'm understanding this correctly, that means --//some_starlark_flag --some_other_value is universally considered two flags. That should be consistent with boolean flags. I don't think anything you're doing adds to these complications. But I just wanted to fill out the fuller considerations on desirable flag syntax.