bazel
bazel copied to clipboard
Pass singleton list to transition for allow_multiple setting default
Previously, the default value of a user-defined string setting with
allow_multiple = True
, which is internally represented as a string,
was passed to transitions as a string, not a list containing the single
string.
Fixes #15653
@gregestren Another edge case that wouldn't arise if allow_multiple
flags used attr.string_list
. Do you think that alternatives such as https://github.com/bazelbuild/bazel/pull/14911 could be considered in time for Bazel 6? I'm a bit worried that usage of allow_multiple
is rising and migrating over to a better representation will become more painful over time.
Would it help for you, I, and @aranguyen to sync on this?
The proliferation of issues, PRs, and designs both illustrate the relevance and is making it harder for me to keep up (all my own fault!) I could find a sync recap helpful.
I think it would. I'm back on June 30, feel free to send an invite.
Is https://github.com/bazelbuild/bazel/issues/15653#issuecomment-1152598263 still active even with this PR?
Is https://github.com/bazelbuild/bazel/issues/15653#issuecomment-1152598263 still active even with this PR?
Yes, I don't expect the current PR to fix that bug.
Where were we at with this PR, in the context of the wider discussion we all had last month?
Where were we at with this PR, in the context of the wider discussion we all had last month?
It's another reason for getting rid of allow_multiple
, but is unfortunately needed as long as that parameter is still used on attr.string()
. Although we may of course just call it broken and refrain from fixing it now that it has been deprecated. @gregestren What do you think?
Since the code is here, we might as well have it work a bit better as long as this API exists.
Do you know what's up with the branch conflicts?
@gregestren https://github.com/bazelbuild/bazel/commit/2f7d965287ddfa056b169cf16144d05f78d03c7d caused a large conflict. CI will tell whether I found the right new place for the updated logic.
@aranguyen Could you review this PR? It has been stuck for a while, maybe we can move it forward.
@fmeum sure I can help. I have no more capacity this week but I can review next week if that is okay with you.
@aranguyen Friendly ping, would be great to get this into Bazel 6.
@fmeum it's on my list. I'm also working on another 6.0 release blocker. Sorry for the delay.
LGTM! @sgowroji FYI, I've already started the internal merge.