User defined build setting value is ignored when the setting is referenced using its own workspace name

[konste]

Description of the problem / feature request:

User defined build setting specified on the command line is silently ignored (and default value of the setting is used instead) when the setting target label explicitly includes its workspace name.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

I have setup a minimal repro here.

This works as expected:
bazel build first --//:first_flag=ff1
bazel build first --@//:first_flag=ff1

But the next variant is broken:
bazel build first --@first//:first_flag=ff1

What is given after the equal sign is ignored and the value of the flag is ALWAYS what is specified as build_setting_default.

Using explicit workspace name from ANOTHER workspace works again:
bazel build @second//:second --@second//:second_flag=sf1

What operating system are you running Bazel on?

Mac, Linux, Windows

What's the output of bazel info release?

4.0.0

[ghost]

Similarly, Bazel fails to de-dupe the equivalent keys. After adding ff3 as another allowed value, I can make Bazel 4 crash like so: bazel build first --//:first_flag=ff1 --@//first_flag=ff3.

PR hopefully coming shortly.

$ bazelisk --host_jvm_debug build first --@//:first_flag=ff1 --//:first_flag=ff3
Running host JVM under debugger (listening on TCP port 5005).
Starting local Bazel server and connecting to it...
INFO: Invocation ID: d11e3af4-2ed7-4af6-837e-e9e1b958b7db
FATAL: bazel crashed due to an internal error. Printing stack trace:
java.lang.IllegalStateException: Duplicate key //:first_flag (attempted merging values ff3 and ff1)
	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)

[ghost]

Oh! It looks like this was already fixed ~Jan-Feb on master.

#11128 (comment)

So I guess this can be resolved as a duplicate of #11128?

[konste]

@beasleyr-vmw do you know if the fix is going to make it to 4.1.0? I cannot find what the cutoff date was.

[ghost]

@konste It doesn't look like it. :(

A diff of master..release-4.1.0rc4 reveals the following:

@@ -242,11 +154,8 @@ public class StarlarkOptionsParser {

     if (value != null) {
       // --flag=value or -flag=value form
-      Target buildSettingTarget = loadBuildSetting(name, eventHandler);
-      // Use the canonical form to ensure we don't have
-      // duplicate options getting into the starlark options map.
-      unparsedOptions.put(
-          buildSettingTarget.getLabel().getCanonicalForm(), new Pair<>(value, buildSettingTarget));
+      Target buildSettingTarget = loadBuildSetting(name, nativeOptionsParser, eventHandler);
+      unparsedOptions.put(name, new Pair<>(value, buildSettingTarget));
     } else {
       boolean booleanValue = true;
       // check --noflag form

juliexxia's commits might be candidates for cherry-picks (see #13099), though.

c228558 <-- fixes cmdline stuff
2039c75 <-- prereq for next commit
98d376f <-- fixes for transitions

Check w/ the maintainers to see if they'd entertain cherry-picking just the first commit. (The merge conflict is minor.) The second change is trivial, but I don't know enough about the third change to determine if it'd make sense to cherry-pick. (Significant conflicts, and I don't know its prereqs.)

[konste]

Thanks! I'll try.

[katre]

Was this cherrypicked into the 4.2 series? Sorry I didn't see it before now.

If not, it will have to wait for the 5.0 release, scheduled for this Fall.

[gregestren]

Is this bug closable?

[konste]

Yes, the problem is verified fixed in 5.0.