-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Display valid crate types in error message for --crate-type flag #134720
base: master
Are you sure you want to change the base?
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @BoxyUwU (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
I seem to recall not all the targets support all the crate types. For example, does all of the wasm targets support Though I suppose the user will get another follow-up error anyway 😄 |
You're right 🙂 This error is just to show the valid values for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an ui test for this diagnostics?
Hi, I added the tests. I'm not sure if I did it the right way, though. |
980ca14
to
62353f8
Compare
The UI test looks good. You'll should squash all your changes into one commit |
a60dbd4
to
6315245
Compare
Done 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks like a nice improvement, only some tiny stylistic nits, then LGTM.
Thanks, I'll r+ after PR CI is green. |
This comment has been minimized.
This comment has been minimized.
(You might need to rebless the test) |
Also, can you squash the commits into one? Thanks. |
9a2ef0a
to
b56ec2f
Compare
Cool. |
@rustbot blocked (on rust-lang/cargo#14990 merging and also for a cargo submodule bump in this repo which contains that PR) |
### What does this PR try to resolve? This PR relaxes the `bad_crate_type` test to have it only match the prefix of the unknown crate type error message emitted by rustc. This is so that the cargo test isn't sensitive to (future) suggestions for known crate types that rustc may emit to help the user. ### How should we test and review this PR? This test should already be run as part of cargo CI. (This is definitely run as part of rust-lang/rust CI, lol.) ### Additional information rust-lang/rust side PR that's trying to add a suffix to the bad crate type error message to list all valid `--crate-type` values: rust-lang/rust#134720. Without relaxing this test, the rust-lang/rust side PR [fails with](rust-lang/rust#134720 (comment)): <details> <summary>rust-lang/rust CI fail message</summary> (Ignore the missing colon after `unknown crate type`) ``` ---- bad_config::bad_crate_type stdout ---- running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/cargo build -v` thread 'bad_config::bad_crate_type' panicked at src/tools/cargo/tests/testsuite/bad_config.rs:434:10: ---- expected: tests/testsuite/bad_config.rs:424:27 ++++ actual: stderr 1 1 | [ERROR] failed to run `rustc` to learn about crate-type bad_type information 2 2 | 3 3 | Caused by: 4 4 | process didn't exit successfully: `rustc - --crate-name ___ --print=file-names --crate-type bad_type` ([EXIT_STATUS]: 1) 5 5 | --- stderr 6 - [ERROR] unknown crate type: `bad_type` Error: 6 + [ERROR] unknown crate type `bad_type`, expected one of: `bin`, `cdylib`, `dylib`, `lib`, `proc-macro`, `rlib` 7 7 | ``` </details> Discussed at https://rust-lang.zulipchat.com/#narrow/channel/246057-t-cargo/topic/Reblessing.20a.20cargo.20test.
c211549
to
e20e87d
Compare
Okay done. Sorry that it took so long. Had to rebuild it 2 times and test. |
There's no rush at all, we can't merge this before the cargo submodule bump which contains my cargo-side PR to merge back into this repo anyway lol. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this LGTM now, we just have to wait for the cargo submodule bump before the cargo test in this repo can pass.
This comment has been minimized.
This comment has been minimized.
Oh yeah. I ran x.py test tidy. Not x.py fmt. Didn't know that. Sorry! |
e20e87d
to
0d5087d
Compare
Hey @jieyouxu sorry for pinging, but I think this pr can be merged. |
Thanks for the reminder, I missed the cargo bump. @bors r+ rollup |
…ues, r=jieyouxu Display valid crate types in error message for --crate-type flag This PR improves the error message for the --crate-type flag. When an invalid crate type is provided, the compiler will now show a list of valid options. ### Before ![image](https://github.com/user-attachments/assets/4922e4e5-eeca-40cd-ac1c-1c6319a81aee) ### After ![image](https://github.com/user-attachments/assets/67ea1f35-aa41-4e4f-8691-47c273d0cff9) I based the implementation on `OutputType::shorthands_display` Closes rust-lang#70183
@bors r- presumptive (partial) cause of #135181 (comment) |
Ah right, I added a new test, may have to rebless it |
The job Click to see the possible cause of the failure (guessed by this bot)
|
This PR improves the error message for the --crate-type flag. When an invalid crate type is provided, the compiler will now show a list of valid options.
Before
After
I based the implementation on
OutputType::shorthands_display
Closes #70183