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.

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

Suggest to specify a target triple when lang item is missing #91820

Merged
merged 1 commit into from
Dec 14, 2021

Conversation

rukai
Copy link
Contributor

@rukai rukai commented Dec 12, 2021

It is very common for newbies to embedded to hit this confusing error when forgetting to specify the target.
Source: me googling this error many times.

Possible changes

  • We could possibly restrict the note+help to only be included on eh_personality lang item if that helped reduce false positives, but its also possible doing so would just increase false negatives
  • Open to any suggestions on rewriting the messages
  • We could possibly remove the .cargo/config alternative to avoid the message getting too noisy but I think its valuable to have as its the correct approach for most embedded projects so that cargo build just works.

r? rust-lang/diagnostics

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 12, 2021
@camelid camelid added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 13, 2021
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

I think this is a sensible change, I've left some suggestions for the wording/style but otherwise looks good.

.sess
.diagnostic()
.struct_err(&format!("language item required, but not found: `{}`", name))
.note(&format!("This occurs when a binary crate with `#![no_std]` is compiled for a target that usually gets `{}` from the standard library.", name))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.note(&format!("This occurs when a binary crate with `#![no_std]` is compiled for a target that usually gets `{}` from the standard library.", name))
.note(&format!("this can occur when a binary crate with `#![no_std]` is compiled for a target where `{}` is defined in the standard library.", name))

nit: wording change + lowercase diagnostic message

.struct_err(&format!("language item required, but not found: `{}`", name))
.note(&format!("This occurs when a binary crate with `#![no_std]` is compiled for a target that usually gets `{}` from the standard library.", name))
.help(&format!(
r#"If you want to compile for a specific target_triple that doesnt need `{}`, then make sure to run with `--target target_triple` or add the following to the crate's `.cargo/config` file:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
r#"If you want to compile for a specific target_triple that doesnt need `{}`, then make sure to run with `--target target_triple` or add the following to the crate's `.cargo/config` file:
"you may be able to compile for a target that doesn't need `{}`, specify a target with `--target` or in `.cargo/config`"

nit: wording

I don't know if there are other diagnostics where we reference what to do in Cargo, might be worth checking if there is precedent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I searched for other references to .cargo/config and didnt find any.
But I did stumble across this earlier:

// NOTE: this suggests using rustup, even though the user may not have it installed.
// That's because they could choose to install it; or this may give them a hint which
// target they need to install from their distro.
if missing_core {
err.help(&format!(
"consider downloading the target with `rustup target add {}`",

Which references what to do in rustup.
It also makes the case that its useful even when the user doesnt have rustup because it gives a hint what you would have to do in the equivalent tool.
In this case it hints that you should probably set the target in your build system of choice.

@@ -70,7 +70,19 @@ fn verify<'tcx>(tcx: TyCtxt<'tcx>, items: &lang_items::LanguageItems) {
tcx.sess.note_without_error("Use `#![feature(default_alloc_error_handler)]` for a default error handler");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tcx.sess.note_without_error("Use `#![feature(default_alloc_error_handler)]` for a default error handler");
tcx.sess.note_without_error("use `#![feature(default_alloc_error_handler)]` for a default error handler");

pre-existing: our diagnostics are typically always lowercase, could you add a commit changing this?

@davidtwco davidtwco 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 Dec 13, 2021
@rukai rukai force-pushed the help_with_personality_issues branch from 018d5ae to fae40c5 Compare December 13, 2021 13:04
@rukai
Copy link
Contributor Author

rukai commented Dec 13, 2021

The suggested changes are a big improvement, thanks!

@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 13, 2021

📌 Commit fae40c5 has been approved by davidtwco

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 13, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 14, 2021
… r=davidtwco

Suggest to specify a target triple when lang item is missing

It is very common for newbies to embedded to hit this confusing error when forgetting to specify the target.
Source: me googling this error many times.

## Possible changes
* We could possibly restrict the note+help to only be included on eh_personality lang item if that helped reduce false positives, but its also possible doing so would just increase false negatives
* Open to any suggestions on rewriting the messages
* We could possibly remove the `.cargo/config` alternative to avoid the message getting too noisy but I think its valuable to have as its the correct approach for most embedded projects so that `cargo build` just works.

r? rust-lang/diagnostics
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2021
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#91529 (add BinaryHeap::try_reserve and BinaryHeap::try_reserve_exact)
 - rust-lang#91820 (Suggest to specify a target triple when lang item is missing)
 - rust-lang#91851 (Make `MaybeUninit::zeroed` `const`)
 - rust-lang#91875 (Use try_normalize_erasing_regions in RevealAllVisitor)
 - rust-lang#91887 (Remove `in_band_lifetimes` from `rustc_const_eval`)
 - rust-lang#91892 (Fix HashStable implementation on InferTy)
 - rust-lang#91893 (Remove `in_band_lifetimes` from `rustc_hir`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit dfec47f into rust-lang:master Dec 14, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants