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

Add new tool for dumping feature status based on tidy #135844

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add new tool for dumping feature status based on tidy #135844

wants to merge 3 commits into from
+123 −0

Conversation

yaahc
Copy link
Member

@yaahc yaahc commented Jan 21, 2025

sequel to #133514

meaning ...

supercedes #133351

part of #129485

r? @jieyouxu
cc @estebank

@rustbot rustbot added A-tidy Area: The tidy tool 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 Jan 21, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 21, 2025

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 21, 2025
@yaahc yaahc mentioned this pull request Jan 21, 2025
Closed
@jieyouxu
Copy link
Member

jieyouxu commented Jan 22, 2025 •
edited
Loading

May need to rebase the merge commit away (maybe this is a different set of changes?)

@jieyouxu jieyouxu removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 22, 2025
@yaahc
Copy link
Member Author

yaahc commented Jan 24, 2025 •
edited
Loading

yeah I realized that this was a branch named after your branch but which I failed to merge into when I was trying to sort out the commit history stuff, then when I pushed this I took a break because I've been feeling like shit this week and had to rest. Thank you for being so patient with me.

@yaahc yaahc force-pushed the tidy-feature-status-dump branch from b78ca32 to a542953 Compare January 24, 2025 23:05
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@yaahc yaahc mentioned this pull request Jan 24, 2025
Closed
@yaahc yaahc force-pushed the tidy-feature-status-dump branch from bd19bac to 69b79a3 Compare January 24, 2025 23:55
@yaahc
Copy link
Member Author

yaahc commented Jan 24, 2025

alright, I think we're gucchi. I haven't tested it but hopefully CI will do that, gotta run and catch a train.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
jieyouxu
jieyouxu reviewed Jan 26, 2025
View reviewed changes
src/tools/tidy/src/features/version.rs Outdated
@@ -8,6 +8,7 @@ mod tests;
pub const VERSION_PLACEHOLDER: &str = "CURRENT_RUSTC_VERSION";

#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
#[derive(serde::Serialize)]
Copy link
Member

Choose a reason for hiding this comment

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

This is missing a cfg_attr on feature = "build-metrics".

Copy link
Member

Choose a reason for hiding this comment

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

Fixed

src/tools/tidy/Cargo.toml Outdated
Comment on lines 21 to 22
[features]
build-metrics = ["serde"]
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be "dep:serde".

Copy link
Member

Choose a reason for hiding this comment

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

Fixed

@jieyouxu jieyouxu force-pushed the tidy-feature-status-dump branch from 69b79a3 to f5dff8d Compare January 26, 2025 06:44
@jieyouxu jieyouxu force-pushed the tidy-feature-status-dump branch from f5dff8d to cbcba57 Compare January 26, 2025 06:45
jieyouxu
jieyouxu approved these changes Jan 26, 2025
View reviewed changes
Copy link
Member

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

The changes looked good to me, so I checked out your branch locally to test. Only two tiny issues that I needed to change to get the check and run steps to work properly, so I pushed the fix to your branch (and slightly adjusted the commit history). Let me know what you think, otherwise r=me and you.

With the two fixes to get it to build, I tested these locally:

  • ./x check src/tools/features-status-dump... OK
  • ./x check src/tools/features-status-dump (unmodified re-run)... OK
  • ./x check src/tools/features-status-dump (modified)... OK, REBUILDS
  • ./x run src/tools/features-status-dump... OK, produces a JSON under build/
  • ./x run src/tools/features-status-dump (modified)... OK, REBUILDS

@jieyouxu jieyouxu removed the has-merge-commits PR has merge commits, merge with caution. label Jan 26, 2025
@jieyouxu
Copy link
Member

jieyouxu commented Jan 26, 2025 •
edited
Loading

Not sure why it still said has-merge-commits when the commit history clearly doesn't

@jieyouxu
Copy link
Member

yeah I realized that this was a branch named after your branch but which I failed to merge into when I was trying to sort out the commit history stuff, then when I pushed this I took a break because I've been feeling like shit this week and had to rest. Thank you for being so patient with me.

Hey no worries, take it easy, there's no rush. I hope you feel better soon :3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jieyouxu jieyouxu jieyouxu approved these changes

Assignees

@jieyouxu jieyouxu

Labels
A-tidy Area: The tidy tool S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yaahc @rustbot @jieyouxu @rust-log-analyzer