bazel icon indicating copy to clipboard operation
bazel copied to clipboard

Pass singleton list to transition for allow_multiple setting default

Open fmeum opened this issue 2 years ago • 15 comments

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

fmeum avatar Jun 10 '22 08:06 fmeum

@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.

fmeum avatar Jun 10 '22 08:06 fmeum

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.

gregestren avatar Jun 22 '22 21:06 gregestren

I think it would. I'm back on June 30, feel free to send an invite.

fmeum avatar Jun 23 '22 12:06 fmeum

Is https://github.com/bazelbuild/bazel/issues/15653#issuecomment-1152598263 still active even with this PR?

gregestren avatar Jun 23 '22 23:06 gregestren

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.

fmeum avatar Jun 24 '22 06:06 fmeum

Where were we at with this PR, in the context of the wider discussion we all had last month?

gregestren avatar Jul 25 '22 19:07 gregestren

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?

fmeum avatar Jul 25 '22 21:07 fmeum

Since the code is here, we might as well have it work a bit better as long as this API exists.

gregestren avatar Jul 26 '22 16:07 gregestren

Do you know what's up with the branch conflicts?

gregestren avatar Jul 26 '22 16:07 gregestren

@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.

fmeum avatar Jul 26 '22 16:07 fmeum

@aranguyen Could you review this PR? It has been stuck for a while, maybe we can move it forward.

fmeum avatar Sep 07 '22 22:09 fmeum

@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 avatar Sep 07 '22 22:09 aranguyen

@aranguyen Friendly ping, would be great to get this into Bazel 6.

fmeum avatar Sep 19 '22 19:09 fmeum

@fmeum it's on my list. I'm also working on another 6.0 release blocker. Sorry for the delay.

aranguyen avatar Sep 19 '22 20:09 aranguyen

LGTM! @sgowroji FYI, I've already started the internal merge.

aranguyen avatar Sep 20 '22 04:09 aranguyen