Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pass singleton list to transition for allow_multiple setting default #15655

Closed
wants to merge 1 commit into from
Closed

Pass singleton list to transition for allow_multiple setting default #15655

wants to merge 1 commit into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Jun 10, 2022

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 fmeum mentioned this pull request Jun 10, 2022
Closed
@fmeum
Copy link
Collaborator Author

fmeum commented Jun 10, 2022

@gregestren Another edge case that wouldn't arise if allow_multiple flags used attr.string_list. Do you think that alternatives such as #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.

@sgowroji sgowroji added team-Configurability platforms, toolchains, cquery, select(), config transitions awaiting-review PR is awaiting review from an assigned reviewer labels Jun 10, 2022
@gregestren
Copy link
Contributor

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.

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 23, 2022

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

@gregestren
Copy link
Contributor

Is #15653 (comment) still active even with this PR?

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 24, 2022

Is #15653 (comment) still active even with this PR?

Yes, I don't expect the current PR to fix that bug.

@gregestren
Copy link
Contributor

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

@fmeum
Copy link
Collaborator Author

fmeum commented Jul 25, 2022

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?

@gregestren
Copy link
Contributor

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

@gregestren
Copy link
Contributor

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

@fmeum
Pass singleton list to transition for allow_multiple setting default
12c07f8 
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 bazelbuild#15653
@fmeum
Copy link
Collaborator Author

fmeum commented Jul 26, 2022

@gregestren 2f7d965 caused a large conflict. CI will tell whether I found the right new place for the updated logic.

@fmeum
Copy link
Collaborator Author

fmeum commented Sep 7, 2022

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

@aranguyen
Copy link
Contributor

@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 aranguyen self-assigned this Sep 16, 2022
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 19, 2022

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

@aranguyen
Copy link
Contributor

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

@aranguyen aranguyen added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Sep 20, 2022
@aranguyen
Copy link
Contributor

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

@aranguyen aranguyen self-requested a review September 20, 2022 04:43
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Sep 26, 2022
aiuto pushed a commit to aiuto/bazel that referenced this pull request Oct 12, 2022
@fmeum @aiuto
Pass singleton list to transition for allow_multiple setting default
0f74039 
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 bazelbuild#15653

Closes bazelbuild#15655.

PiperOrigin-RevId: 476375200
Change-Id: Id036f17836e2523aa50c7b026f5397ab0a88952b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aranguyen aranguyen Awaiting requested review from aranguyen

Assignees

@aranguyen aranguyen

Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Milestone
No milestone
4 participants
@fmeum @gregestren @aranguyen @sgowroji