Skip to content

feat(config): Add ChangeId enum for suppressing warnings - #138986

Merged
bors merged 3 commits into
rust-lang:masterfrom
bit-aloo:2025-03-25-add-ignore-to-change-id
Mar 28, 2025
Merged

feat(config): Add ChangeId enum for suppressing warnings#138986
bors merged 3 commits into
rust-lang:masterfrom
bit-aloo:2025-03-25-add-ignore-to-change-id

Conversation

@bit-aloo

@bit-aloobit-aloo commented Mar 26, 2025

Copy link
Copy Markdown
Member

closes: #138925

@rustbot

rustbot commented Mar 26, 2025

Copy link
Copy Markdown
Collaborator

r? @Kobzol

rustbot has assigned @Kobzol.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Mar 26, 2025
@rustbot

rustbot commented Mar 26, 2025

Copy link
Copy Markdown
Collaborator

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@bit-aloo

bit-aloo commented Mar 26, 2025

Copy link
Copy Markdown
MemberAuthor

Interesting stuff—TOML only supports i64. Would it make sense to change change_id from usize to isize? An alternative approach could be using a negative change_id to indicate the suppression of console warnings. The downside is that if we switch to isize, 32-bit architecture machines will have their range reduced to i32. However, on those machines, TOML's i64 range can still easily cover u32 values.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bit-aloo
bit-alooforce-pushed the 2025-03-25-add-ignore-to-change-id branch from 3d54f33 to d58b5d5CompareMarch 26, 2025 13:56
@jieyouxu

jieyouxu commented Mar 26, 2025

Copy link
Copy Markdown
Member

My serde is rusty (hah), but I think there's something like an {deserialize,serialize}_with attr that takes a simple fn instead of a full blown deserializer

@bit-aloo

bit-aloo commented Mar 27, 2025•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

My serde is rusty (hah), but I think there's something like an {deserialize,serialize}_with attr that takes a simple fn instead of a full blown deserializer

I believe deserialize_with and serialize_with can only be applied to struct or enum fields, not directly to types themselves. That said, if we're only ever deserializing ChangeId through ChangeIdWrapper, then this approach could work. I was just assuming that ChangeId might also need to be deserialized independently, in which case implementing Deserialize directly for it made more sense.

Kobzol
Kobzol reviewed Mar 27, 2025
View reviewed changes
Comment threadsrc/bootstrap/src/core/config/config.rs Outdated
Comment threadsrc/bootstrap/src/bin/main.rs Outdated
bit-aloo
bit-aloo commented Mar 27, 2025
View reviewed changes
Comment threadsrc/bootstrap/src/bin/main.rs Outdated
Kobzol
Kobzol reviewed Mar 27, 2025
View reviewed changes
Comment threadsrc/bootstrap/src/utils/change_tracker.rs Outdated
Comment threadsrc/bootstrap/src/bin/main.rs Outdated
@bit-aloo
bit-alooforce-pushed the 2025-03-25-add-ignore-to-change-id branch 3 times, most recently from 3cbe872 to 2579dd1CompareMarch 27, 2025 16:17
@bit-aloo
feat(config): Add ChangeId enum for suppressing warnings
eaa0613
Introduces the `ChangeId` enum to allow suppressing `change_id` warnings.
Now, `ChangeId` supports both numeric values and the string literal `"ignore"`.
Numeric values behave as expected, while `"ignore"` is used to suppress warning messages.
@bit-aloo
add changeInfo to change tracker
0244432
@bit-aloo
bit-alooforce-pushed the 2025-03-25-add-ignore-to-change-id branch from 2579dd1 to 0244432CompareMarch 27, 2025 16:33
@bit-aloo

bit-aloo commented Mar 27, 2025

Copy link
Copy Markdown
MemberAuthor

Hello @Kobzol , all suggestions have been addressed.

Kobzol
Kobzol reviewed Mar 27, 2025
View reviewed changes

@KobzolKobzol left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried this locally, seems to work well. The code is also nicer, thanks! Left a few remaining nits.

Also, to make this new option a bit more discovereable, could you please modify the "to silence this warning" message, to mention this new option? Something like NOTE: to silence this warning, update bootstrap.tomlto usechange-id = 138986orchange-id = "ignore" instead. Thanks!

Comment threadsrc/bootstrap/src/core/config/config.rs Outdated
Comment threadsrc/bootstrap/src/utils/change_tracker.rs Outdated
@rustbot

rustbot commented Mar 27, 2025

Copy link
Copy Markdown
Collaborator

This PR modifies bootstrap.example.toml.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@bit-aloo

bit-aloo commented Mar 27, 2025

Copy link
Copy Markdown
MemberAuthor

Hello @Kobzol, I've updated the descriptions. Thanks a lot for the review

Kobzol
Kobzol approved these changes Mar 27, 2025
View reviewed changes

@KobzolKobzol left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! After fixing the missing space, you can write @bors r=kobzol to approve this PR.

@bors delegate+

Comment threadsrc/bootstrap/src/bin/main.rs Outdated
@bit-aloo
bit-alooforce-pushed the 2025-03-25-add-ignore-to-change-id branch from 7a55d91 to b24083bCompareMarch 28, 2025 03:10
@bit-aloo

bit-aloo commented Mar 28, 2025

Copy link
Copy Markdown
MemberAuthor

@bors r=kobzol

@bors

bors commented Mar 28, 2025

Copy link
Copy Markdown
Collaborator

@Shourya742: 🔑 Insufficient privileges: Not in reviewers

@thaliaarchithaliaarchi mentioned this pull request Mar 28, 2025
Merged
@Kobzol

Kobzol commented Mar 28, 2025

Copy link
Copy Markdown
Member

Looks like bors missed my command, sorry. Thank you for the PR!

@bors r+ rollup

@bors

bors commented Mar 28, 2025

Copy link
Copy Markdown
Collaborator

📌 Commit b24083b has been approved by Kobzol

It is now in the queue for this repository.

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 28, 2025
@matthiaskrgrmatthiaskrgr mentioned this pull request Mar 28, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 28, 2025
@bors
Auto merge ofrust-lang#139054- matthiaskrgr:rollup-2bk2fb4, r=matth…
afaa656
…iaskrgr
Rollup of 7 pull requests
Successful merges:
- rust-lang#137889 (update outdated doc with new example)
- rust-lang#138104 (Greatly simplify doctest parsing and information extraction)
- rust-lang#138678 (rustc_resolve: fix instability in lib.rmeta contents)
- rust-lang#138986 (feat(config): Add ChangeId enum for suppressing warnings)
- rust-lang#139038 (Update target maintainers for thumb targets to reflect new REWG Arm team name)
- rust-lang#139045 (bootstrap: update `test_find` test)
- rust-lang#139047 (Remove ScopeDepth)
Failed merges:
- rust-lang#139044 (bootstrap: Avoid cloning `change-id` list)
r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 28, 2025
@bors
Auto merge ofrust-lang#139054- matthiaskrgr:rollup-2bk2fb4, r=matth…
19f42cb
…iaskrgr
Rollup of 7 pull requests
Successful merges:
- rust-lang#137889 (update outdated doc with new example)
- rust-lang#138104 (Greatly simplify doctest parsing and information extraction)
- rust-lang#138678 (rustc_resolve: fix instability in lib.rmeta contents)
- rust-lang#138986 (feat(config): Add ChangeId enum for suppressing warnings)
- rust-lang#139038 (Update target maintainers for thumb targets to reflect new REWG Arm team name)
- rust-lang#139045 (bootstrap: update `test_find` test)
- rust-lang#139047 (Remove ScopeDepth)
Failed merges:
- rust-lang#139044 (bootstrap: Avoid cloning `change-id` list)
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit 60833b1 into rust-lang:masterMar 28, 2025
@rustbotrustbot added this to the 1.87.0 milestone Mar 28, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 28, 2025
@rust-timer
Unrolled build forrust-lang#138986
ccebcd9
Rollup merge of rust-lang#138986 - Shourya742:2025-03-25-add-ignore-to-change-id, r=Kobzol
feat(config): Add ChangeId enum for suppressing warnings
closes: rust-lang#138925
@bit-aloo
bit-aloo deleted the 2025-03-25-add-ignore-to-change-id branch March 29, 2025 00:37
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@KobzolKobzolKobzol approved these changes

Assignees

@KobzolKobzol

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-bootstrapRelevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Projects

None yet

Milestone

1.87.0

Development

Successfully merging this pull request may close these issues.

Find a Way to Silence change-id Warnings

6 participants

@bit-aloo@rustbot@rust-log-analyzer@jieyouxu@bors@Kobzol