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

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

Merged
merged 3 commits into from
Mar 28, 2025
Merged

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

merged 3 commits into from
Mar 28, 2025

Conversation

Shourya742
Copy link
Contributor

closes: #138925

@rustbot
Copy link
Collaborator

rustbot commented Mar 26, 2025

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

@rustbot rustbot 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
Copy link
Collaborator

rustbot commented Mar 26, 2025

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

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

@Shourya742
Copy link
Contributor Author

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
@Shourya742 Shourya742 force-pushed the 2025-03-25-add-ignore-to-change-id branch from 3d54f33 to d58b5d5 Compare March 26, 2025 13:56
@jieyouxu
Copy link
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

@Shourya742
Copy link
Contributor Author

Shourya742 commented Mar 27, 2025 •
edited
Loading

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
Kobzol
Kobzol reviewed Mar 27, 2025
View reviewed changes
@Shourya742 Shourya742 force-pushed the 2025-03-25-add-ignore-to-change-id branch 3 times, most recently from 3cbe872 to 2579dd1 Compare March 27, 2025 16:17
@Shourya742
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.
@Shourya742
add changeInfo to change tracker
0244432
@Shourya742 Shourya742 force-pushed the 2025-03-25-add-ignore-to-change-id branch from 2579dd1 to 0244432 Compare March 27, 2025 16:33
@Shourya742
Copy link
Contributor Author

Hello @Kobzol , all suggestions have been addressed.

Kobzol
Kobzol reviewed Mar 27, 2025
View reviewed changes
Copy link
Contributor

@Kobzol Kobzol left a comment •
edited
Loading

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!

@rustbot
Copy link
Collaborator

rustbot commented Mar 27, 2025

This PR modifies bootstrap.example.toml.

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

@Shourya742
Copy link
Contributor Author

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

Kobzol
Kobzol approved these changes Mar 27, 2025
View reviewed changes
Copy link
Contributor

@Kobzol Kobzol left a comment

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+

@Shourya742 Shourya742 force-pushed the 2025-03-25-add-ignore-to-change-id branch from 7a55d91 to b24083b Compare March 28, 2025 03:10
@Shourya742
Copy link
Contributor Author

@bors r=kobzol

@bors
Copy link
Contributor

bors commented Mar 28, 2025

@Shourya742: 🔑 Insufficient privileges: Not in reviewers

@thaliaarchi thaliaarchi mentioned this pull request Mar 28, 2025
Merged
@Kobzol
Copy link
Contributor

Kobzol commented Mar 28, 2025

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

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 28, 2025

📌 Commit b24083b has been approved by Kobzol

It is now in the queue for this repository.

@bors bors 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
@matthiaskrgr matthiaskrgr 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 of rust-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 of rust-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:master Mar 28, 2025
6 checks passed
@rustbot rustbot 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 for rust-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
@Shourya742 Shourya742 deleted the 2025-03-25-add-ignore-to-change-id branch March 29, 2025 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kobzol Kobzol

Assignees

@Kobzol Kobzol

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant 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
@Shourya742 @rustbot @rust-log-analyzer @jieyouxu @bors @Kobzol