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

Remove detection of rustup and cargo in 'missing extern crate' diagnostics #87359

Merged
merged 1 commit into from
Jul 24, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Jul 22, 2021

Previously, this would change the test output when RUSTUP_HOME was set:

---- [ui] ui/issues/issue-49851/compiler-builtins-error.rs stdout ----
diff of stderr:

1       error[E0463]: can't find crate for `core`
2          |
3          = note: the `thumbv7em-none-eabihf` target may not be installed
+          = help: consider downloading the target with `rustup target add thumbv7em-none-eabihf`
4
5       error: aborting due to previous error
6

Originally, I fixed it by explicitly unsetting RUSTUP_HOME in
compiletest. Then I realized that almost no one has RUSTUP_HOME set,
since rustup doesn't set it itself. It does set RUST_RECURSION_COUNT
whenever it launches a proxy, though - use that instead.

r? @estebank cc @petrochenkov @kinnison

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 22, 2021
@jyn514 jyn514 added A-diagnostics Area: Messages for errors, warnings, and lints A-metadata Area: Crate metadata T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 22, 2021
@rust-log-analyzer

This comment has been minimized.

@crlf0710
Copy link
Member

I have RUSTUP_HOME set to move all rust related things to a directory under D:\ disk. I didn't really realize the test suite difference is because of this... (and didn't investigate) Thank you for fixing it!

Comment on lines 1083 to 1089
if missing_core && std::env::var("RUST_RECURSION_COUNT").is_ok() {
err.help(&format!(
"consider downloading the target with `rustup target add {}`",
locator.triple
Copy link
Contributor

Choose a reason for hiding this comment

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

An option would be to use the env var only to be more specific on the help:

Suggested change
if missing_core && std::env::var("RUST_RECURSION_COUNT").is_ok() {
err.help(&format!(
"consider downloading the target with `rustup target add {}`",
locator.triple
if missing_core {
err.help(&format!(
"{}consider downloading the target with `rustup target add {}`",
if std::env::var("RUST_RECURSION_COUNT").is_ok() {
"if you have installed `rustc` with `rustup`,
} else {
""
},
locator.triple

An even better check would be to use which (or similar) to see if rustup is available at all before giving the suggestion (but then we could get in the weird situation where rustc was installed by, let's say apt, but rustup is available and the suggestion would be wrong).

@estebank
Copy link
Contributor

r=me if noone objects to this approach

@Mark-Simulacrum
Copy link
Member

It looks like this detection was added in #84450 without any discussion that I can see on the appropriateness of doing this sort of runtime detection to vary error output.

I am curious about the motivation to only expose this if rustup is installed. In practice, even if the user did install with apt or some other tool, the pointer to rustup target install may still be quite helpful (perhaps they should switch to rustup for example). And if they're not running with rustup and don't want to be, the diagnostic is not really misleading IMO -- it may still be helpful to let them find the appropriate target-specific package in their distro's toolchain for example.

So I'd suggest just dropping the env variable check entirely here, rather than trying to change it to a different one. It seems annoying that rerunning tests with and without rustup wrappers will produce potentially different results; it's not something I at least would expect.

@jyn514
Copy link
Member Author

jyn514 commented Jul 22, 2021

I am curious about the motivation to only expose this if rustup is installed. In practice, even if the user did install with apt or some other tool, the pointer to rustup target install may still be quite helpful (perhaps they should switch to rustup for example). And if they're not running with rustup and don't want to be, the diagnostic is not really misleading IMO -- it may still be helpful to let them find the appropriate target-specific package in their distro's toolchain for example.

So I'd suggest just dropping the env variable check entirely here, rather than trying to change it to a different one.

Hmm, ok. That makes sense to me.

It seems annoying that rerunning tests with and without rustup wrappers will produce potentially different results; it's not something I at least would expect.

Well, the idea is that you would never run tests with rustup wrappers, since x.py controls the toolchain you use and it always goes through build/stageN. But I agree that saying this unconditionally is probably a better approach.

@jyn514
Copy link
Member Author

jyn514 commented Jul 22, 2021

By that logic, do you want me to also remove the runtime checking for cargo? https://github.com/rust-lang/rust/blob/32373c40a9781cbbd7ed292a299b28258b2910a8/compiler/rustc_metadata/src/locator.rs#L1100-L1102

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

I think it's a good idea, yes, for similar reasons.

@jyn514 jyn514 changed the title Fix detection of rustup in 'missing extern crate' diagnostics Remove detection of rustup and cargo in 'missing extern crate' diagnostics Jul 24, 2021
@rust-log-analyzer

This comment has been minimized.

…stics

Previously, this would change the test output when RUSTUP_HOME was set:

```
---- [ui] ui/issues/issue-49851/compiler-builtins-error.rs stdout ----
diff of stderr:

1       error[E0463]: can't find crate for `core`
2          |
3          = note: the `thumbv7em-none-eabihf` target may not be installed
+          = help: consider downloading the target with `rustup target add thumbv7em-none-eabihf`
4
5       error: aborting due to previous error
6
```

Originally, I fixed it by explicitly unsetting RUSTUP_HOME in
compiletest. Then I realized that almost no one has RUSTUP_HOME set,
since rustup doesn't set it itself; although it does set RUST_RECURSION_COUNT
whenever it launches a proxy. Then it was pointed out that this runtime
check doesn't really make sense and it's fine to make it unconditional.
@estebank
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 24, 2021

📌 Commit 17f7536 has been approved by estebank

@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 Jul 24, 2021
LeSeulArtichaut added a commit to LeSeulArtichaut/rust that referenced this pull request Jul 24, 2021
Remove detection of rustup and cargo in 'missing extern crate' diagnostics

Previously, this would change the test output when RUSTUP_HOME was set:

```
---- [ui] ui/issues/issue-49851/compiler-builtins-error.rs stdout ----
diff of stderr:

1       error[E0463]: can't find crate for `core`
2          |
3          = note: the `thumbv7em-none-eabihf` target may not be installed
+          = help: consider downloading the target with `rustup target add thumbv7em-none-eabihf`
4
5       error: aborting due to previous error
6
```

Originally, I fixed it by explicitly unsetting RUSTUP_HOME in
compiletest. Then I realized that almost no one has RUSTUP_HOME set,
since rustup doesn't set it itself. It does set RUST_RECURSION_COUNT
whenever it launches a proxy, though - use that instead.

r? `@estebank` cc `@petrochenkov` `@kinnison`
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 24, 2021
Remove detection of rustup and cargo in 'missing extern crate' diagnostics

Previously, this would change the test output when RUSTUP_HOME was set:

```
---- [ui] ui/issues/issue-49851/compiler-builtins-error.rs stdout ----
diff of stderr:

1       error[E0463]: can't find crate for `core`
2          |
3          = note: the `thumbv7em-none-eabihf` target may not be installed
+          = help: consider downloading the target with `rustup target add thumbv7em-none-eabihf`
4
5       error: aborting due to previous error
6
```

Originally, I fixed it by explicitly unsetting RUSTUP_HOME in
compiletest. Then I realized that almost no one has RUSTUP_HOME set,
since rustup doesn't set it itself. It does set RUST_RECURSION_COUNT
whenever it launches a proxy, though - use that instead.

r? ``@estebank`` cc ``@petrochenkov`` ``@kinnison``
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 24, 2021
…arth

Rollup of 9 pull requests

Successful merges:

 - rust-lang#87348 (Fix span when suggesting to add an associated type bound)
 - rust-lang#87359 (Remove detection of rustup and cargo in 'missing extern crate' diagnostics)
 - rust-lang#87370 (Add support for powerpc-unknown-freebsd)
 - rust-lang#87389 (Rename `known_attrs` to `expanded_inert_attrs` and move to rustc_expand)
 - rust-lang#87395 (Clear up std::env::set_var panic section.)
 - rust-lang#87403 (Implement `AssignToDroppingUnionField` in THIR unsafeck)
 - rust-lang#87410 (Mark `format_args_nl` as `#[doc(hidden)]`)
 - rust-lang#87419 (IEEE 754 is not an RFC)
 - rust-lang#87422 (DOC: remove unnecessary feature crate attribute from example code)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bfa0358 into rust-lang:master Jul 24, 2021
@rustbot rustbot added this to the 1.56.0 milestone Jul 24, 2021
@jyn514 jyn514 deleted the bless-rustup branch July 24, 2021 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-metadata Area: Crate metadata 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.

8 participants