Skip to content

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

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
Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

yaahc
Copy link
Member

@yaahc yaahc commented Nov 26, 2024 •
edited
Loading

supercedes #133351

part of #129485

r? @estebank

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc 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 Nov 26, 2024
@yaahc yaahc force-pushed the tidy-features-dump branch from b5f3538 to a14aeae Compare November 26, 2024 21:23
@rustbot
Copy link
Collaborator

rustbot commented Nov 26, 2024

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.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
estebank
estebank approved these changes Nov 26, 2024
View reviewed changes
Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

r=me

src/tools/features-status-dump/src/main.rs
Comment on lines +14 to +16
let library_path_str = env::args_os().nth(1).expect("library/ path required");
let compiler_path_str = env::args_os().nth(2).expect("compiler/ path required");
let output_path_str = env::args_os().nth(3).expect("output path required");
Copy link
Contributor

Choose a reason for hiding this comment

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

We might be able to extend x.py to either set these automatically, or provide default paths here.

Copy link
Member

@jieyouxu jieyouxu Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggestion: what you can do is that in the bootstrap build step, pass these paths (which bootstrap will know about) as cli flags to this tool. I'll take a look tmrw.

Copy link
Member

Choose a reason for hiding this comment

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

See #133514 (comment)

@jieyouxu jieyouxu self-assigned this Nov 27, 2024
@jieyouxu
Copy link
Member

(Feel free to ping me if you need bootstrap reviews for metrics things)

@jieyouxu
Copy link
Member

jieyouxu commented Nov 28, 2024 •
edited
Loading

@yaahc see master...jieyouxu:rust:pr-133514:

  • I modified the tool to take the paths from bootstrap, or otherwise ask the user for --compiler-path, --library-path and --output-path. Slightly adjusted to make sure we create all parent dirs of output path.
  • I also registered the tool as a runnable bootstrap tool so you can either just build via ./x build src/tools/features-status-dump or run via ./x run src/tools/features-status-dump. When run in this bootstrap tool configuration, I made it default output to bootstrap build directory, i.e. output path defaults to build/features-status-dump.json.

Feel free to use the 2 additional commits on top of your changes if you want :3

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 28, 2024
@yaahc
Copy link
Member Author

yaahc commented Jan 8, 2025

I'll definitely pull those changes in, thank you very much @jieyouxu

@jieyouxu
Copy link
Member

(I kinda forgor, but would also be nice to register a ./x check src/tools/features-status-dump step for working on the tool itself)

@rustbot rustbot added A-tidy Area: The tidy tool has-merge-commits PR has merge commits, merge with caution. labels Jan 17, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 17, 2025

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease

The following commits are merge commits:

@yaahc
Copy link
Member Author

yaahc commented Jan 17, 2025

hey I'm running into some trouble trying to merge your branch into this PR so I think I might just open a new PR directly from your branch and work from there

This was referenced Jan 17, 2025
Closed
Merged
@yaahc
Copy link
Member Author

yaahc commented Jan 24, 2025 •
edited
Loading

superseded by #135844

@yaahc yaahc closed this Jan 24, 2025
jhpratt added a commit to jhpratt/rust that referenced this pull request Feb 4, 2025
@jhpratt
Rollup merge of rust-lang#135844 - yaahc:tidy-feature-status-dump, r=…
648fd0e 
…jieyouxu

 Add new tool for dumping feature status based on tidy

sequel to rust-lang#133514

meaning ...

supercedes rust-lang#133351

part of rust-lang#129485

r? `@jieyouxu`
cc `@estebank`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 4, 2025
@rust-timer
Unrolled build for rust-lang#135844
b691044 
Rollup merge of rust-lang#135844 - yaahc:tidy-feature-status-dump, r=jieyouxu

 Add new tool for dumping feature status based on tidy

sequel to rust-lang#133514

meaning ...

supercedes rust-lang#133351

part of rust-lang#129485

r? `@jieyouxu`
cc `@estebank`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jieyouxu jieyouxu jieyouxu left review comments

@estebank estebank estebank approved these changes

Assignees

@estebank estebank

@jieyouxu jieyouxu

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc A-tidy Area: The tidy tool 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. 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.
5 participants
@yaahc @rustbot @rust-log-analyzer @jieyouxu @estebank