feat(toolchain): include err in rustc_version output#4741
feat(toolchain): include err in rustc_version output#4741Bogay wants to merge 4 commits intorust-lang:mainfrom
Conversation
Bogay
force-pushed
the
improve-rustc-version-error-message
branch
from
2ceb33c to
b8c00f3
Compare
Bogay
force-pushed
the
improve-rustc-version-error-message
branch
from
b8c00f3 to
1ad60d1
Compare
Bogay
force-pushed
the
improve-rustc-version-error-message
branch
from
1ad60d1 to
44ec134
Compare
| String::from("(error reading rustc version)") | ||
| let out = child | ||
| .stdout | ||
| .expect("Child::stdout requested but not present"); |
There was a problem hiding this comment.
Should we return a string instead of panic here? The doc of rustc_version says it's infallible function
There was a problem hiding this comment.
No. If the function panics here it means the Command setup is wrong and this is a programming error, and we should expose the error instead of covering it up.
| .with_stdout(snapbox::str![ | ||
| r#" | ||
| ... | ||
| compiler: (rustc does not exist: [..]) | ||
| ... | ||
| "# | ||
| ]) |
There was a problem hiding this comment.
This test failed (example job) because it the Toolchain::create_command in Toolchain::rustc_version does not return Err even rustc doesn't exist in toolchain. AFAIK it fallback to the rustc in PATH, which I believe related to #3387 . It's also hard to produce a env that don't have rustc in PATH because mock_bin use rustc to compile the binary. (If we can restrict rustup proxy to not use rustc inside PATH, I think that would be easier.)
| .is_err(); | ||
| } | ||
|
|
||
| #[cfg(unix)] |
There was a problem hiding this comment.
I use shell script to mock the exit code of rustc in toolchain, which I think can only be executed on unix. Not sure whether it's acceptable in rustup test suite. 🤔
There was a problem hiding this comment.
Thanks for asking! Actually we have mock_bin_src.rs which you can use instead of this dirty hack. You can modify it to make the thing crash in a cross-platform and deterministic way. IIRC the rustc we call in other tests is actually this instead of the real rustc.
|
Sorry for getting sidetracked for a while... I'll find some time to give it another look. |
2 participants
This PR includes error details in
Toolchain::rustc_versionso that it's more clear to end-user what happened.Close #3607