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.

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

[7.4.0] Add a string-to-label-dict attribute type. #23998

Merged
merged 2 commits into from
Oct 16, 2024

Conversation

lberki
Copy link
Contributor

@lberki lberki commented Oct 16, 2024

This really just exposes the existing LABEL_DICT_UNARY type that already exists in Java.

Other than writing some boring conversion logic, all pieces fell together pretty pleasantly.

Work towards #7989. That one calls for a string-to-label-list-dict type, but I'm planning to resolve that as WAI, since string-to-label-dict is close enough.

RELNOTES: None.
PiperOrigin-RevId: 686415025
Change-Id: Ib07ada7ab2ede95220ed1cc2d3569996fc4afb88

@lberki lberki requested a review from a team as a code owner October 16, 2024 10:26
@lberki lberki requested a review from meteorcloudy October 16, 2024 10:26
This really just exposes the existing LABEL_DICT_UNARY type that already exists in Java.

Other than writing some boring conversion logic, all pieces fell together pretty pleasantly.

Work towards #7989. That one calls for a string-to-label-list-dict type, but I'm planning to resolve that as WAI, since string-to-label-dict is close enough.

RELNOTES: None.
PiperOrigin-RevId: 686415025
Change-Id: Ib07ada7ab2ede95220ed1cc2d3569996fc4afb88
@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Configurability platforms, toolchains, cquery, select(), config transitions labels Oct 16, 2024
@lberki lberki force-pushed the lberki-cherrypick-7.4.0-string-label-dict branch from 39b1a80 to 42ff626 Compare October 16, 2024 10:29
@meteorcloudy
Copy link
Member

How important is this? We are already at rc2 for 7.4.0, we would like to push back non critical changes and avoid delaying the release further.

@lberki
Copy link
Contributor Author

lberki commented Oct 16, 2024

Not very, I'd say. I sent out this pull request spurred by @fmeum 's comment #7989 (comment) that if we don't backport this to 7.4.0, it'll take a much longer time for rule sets to make use of this new attribute type. It wasn't complicated, so I thought it was worthwhile but I was not aware of the release schedule of 7.4.0.

@meteorcloudy
Copy link
Member

it'll take a much longer time for rule sets to make use of this new attribute type

Hmm, this sounds like it's probably worth another rc.

@meteorcloudy meteorcloudy added this pull request to the merge queue Oct 16, 2024
Merged via the queue into release-7.4.0 with commit b7a22e7 Oct 16, 2024
52 checks passed
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Oct 16, 2024
@iancha1992 iancha1992 added this to the 7.4.0 release blockers milestone Oct 16, 2024
@iancha1992 iancha1992 changed the title Add a string-to-label-dict attribute type. [7.4.0] Add a string-to-label-dict attribute type. Oct 16, 2024
@tetromino
Copy link
Contributor

FYI: unfortunately this cherry-pick was done without cherry-picking related updates to AttributeInfoExtractor.java, so Stardoc + Bazel 7.4/5 will fail on .bzl file using attr.label_keyed_string_dict() :/

See bazelbuild/stardoc#277

@fmeum
Copy link
Collaborator

fmeum commented Jan 24, 2025

7.5.0 is still open

@Wyverald
Copy link
Member

Indeed; I would prefer that we take this chance to cherry-pick a proper fix, as 7.5.0 is still waiting on the merge of a separate PR, and will have another RC next Monday.

@Wyverald Wyverald deleted the lberki-cherrypick-7.4.0-string-label-dict branch January 24, 2025 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants