Respect precedence of --extra_toolchains during toolchains resolution

[layus]

I have noticed that --extra_toolchains do not override each other as could be expected from the usual semantics of options (rc files, then command line flags, the last one always "wins").

bazel/site/en/run/bazelrc.md

Lines 141 to 143 in 6c02329

	- Options on the command line always take precedence over those in rc files.
	For example, if a rc file says `build -c opt` but the command line flag is
	`-c dbg`, the command line flag takes precedence.

In practice, it is impossible to override an --extra_toolchain from an rc file on the command line.

While the toolchains are indeed accumulated as per the options semantics, the way toolchain selection works is that the first one wins. I had to reverse the options list to make the last defined one to be examined first during selection.

I have confirmed that the test fails without the patch, and that the patch fixes the new test.

Fixes #19945

[katre]

This is an interesting idea. We faced something similar with --extra_execution_platforms, and in the end decided that it made more sense for that flag to not accumulate values: c602cec.

Would that approach work for you? I'd also like to try and get a broader sense of how Bazel users are using --extra_toolchains, and if this change makes sense to them. Would you be willing to a) file an issue describing the problem as you see it, and then b) start a discussion (on either bazel-discuss or by opening a github discussion)?

I'm going to test-import this and see if it causes any problems with our internal version of Bazel: this may take a day or so to see results in any case.

[layus]

To be fair, I think the reason this never was an issue before is because people do not use --extra_toolchains on the command line, and seldom use it in bazelrc files. Because if it can be in your bazelrc, it can probably be in your WORKSPACE, unless you use it behind --config flags. Toolchains are complicated enough that you do not want the have to pass one on the command line.

Also, toolchains should generally not compete for the same config. I have read somewhere that relying on registration order is a bad practice. And this issue only happens when you do rely on registration order between rc files and command line arguments. (if you use only command line arguments, you can fix it by changing the arguments order).

The reason I hit this issue is because in nix packaging of bazel, the default prebuilt java toolchains do not work. To keep the expectation that bazel comes with built-in working java toolchains, we patch @bazel_tools to provide nonprebuilt java toolchains. And we hardcode a special system bazelrc that --extra_toolchains all these toolchains. (And I was unknowingly fighting #19934 at the same time).

Now, we could register them all in a WORSPACE prefix/preloaded part, but that does not work with bzlmod.

So, when I wanted to override the hardcoded system bazelrc with a different toolchain, I discovered that bazel ignores my command line arguments.

A proper fix for nix packaging would be to provide nonprebuilt toolchains in rules_java, and better support for nix/nixos and other uncommon platforms where prebuilt binaries do not work. I am slowly moving towards that, but it is out of scope for this PR.

We faced something similar with --extra_execution_platforms, and in the end decided that it made more sense for that flag to not accumulate values: c602cec.

Would that approach work for you?

That would also work indeed. Not sure it is worth forcing users to scramble everything on one line though. Because where execution platforms add to a set, toolchains do override each other (when they have the same constraints). So to override a previously defined toolchain, just add a new one with higher precedence. With execution platforms there was no way to override a previously added exec platform.

Which is ironically the problem I am trying to fix here. There is no way to override toolchains by adding more flags, because the order is wrong.

I'd also like to try and get a broader sense of how Bazel users are using --extra_toolchains, and if this change makes sense to them. Would you be willing to a) file an issue describing the problem as you see it, and then b) start a discussion (on either bazel-discuss or by opening a github discussion)?

I think this patch fixes a bug, and that starting a whole discussion about it is a bit overkill. The discussion could still happen though, but it is orthogonal. If the discussion happens, it should help decide between the fixed version of accumulated flags, and a non-accumulating flag. Nobody will want the current, broken accumulating flag version.

[katre]

This is fairly convincing: thank you for explaining your reasoning. I would still like you to file an issue, because while I suspect you are correct that Bazel users do not use --extra_toolchains heavily, it is used inside Google and so I need to vet those usages and check for potential errors.

[layus]

There you are #19945

[katre]

See #19945 (comment), I think the better approach is to not accumulate values at all. I'm closing this PR and I'll work on that solution.

Thanks for bringing this to my attention!

[katre]

I am re-viving this after being convinced it's a better approach

[fmeum]

@katre Should this be cherry-picked as an incompatible change?

[katre]

Into Bazel 7, you mean? I'm not sure: it's a fairly big change to the semantics of --extra_toolchains, but downstream testing didn't show any issues so maybe it's safe?

[fmeum]

Into Bazel 7, you mean? I'm not sure: it's a fairly big change to the semantics of --extra_toolchains, but downstream testing didn't show any issues so maybe it's safe?

I do expect breakages to show up in complex end user project at a higher rate than in bazelbuild org projects. It will be breaking no matter which release we include it in, so I would say this is more of a question of how much "breaking changes budget" we have left for Bazel 7.

@iancha1992 Do you happen to know whether the 7.0.0 release already has many breaking changes compared to previous releases?

[iancha1992]

Into Bazel 7, you mean? I'm not sure: it's a fairly big change to the semantics of --extra_toolchains, but downstream testing didn't show any issues so maybe it's safe?

I do expect breakages to show up in complex end user project at a higher rate than in bazelbuild org projects. It will be breaking no matter which release we include it in, so I would say this is more of a question of how much "breaking changes budget" we have left for Bazel 7.

@iancha1992 Do you happen to know whether the 7.0.0 release already has many breaking changes compared to previous releases?

cc: @Wyverald @meteorcloudy

[meteorcloudy]

@iancha1992 Thanks for the notification.

I would say this is more of a question of how much "breaking changes budget" we have left for Bazel 7.

We don't really have a clear defined breaking change budget. As long as it won't break too many downstream projects, I'm fine with getting it into Bazel 7.0.0. I'll leave the decision to @katre

[katre]

I'm fine with cherrypicking this if it's not too late. Do we know yet if there'll be an rc5? Or should it go into 7.1.0 at this point?

[meteorcloudy]

7.1.0 must be fully backwards compatible with 7.0.0, so that's not an option.

Let's sync with the gTech team.

[keertk]

@bazel-io fork 7.0.0