[rustdoc] Gracefully handle error in case we cannot run the compiler in doctests #147912
Conversation
rustbot added A-run-make 
I think a message like |
GuillaumeGomez force-pushed the graceful-doctest-error-handling branch from 4e0541a to 0c495ae Compare 
| Agreed, improved error messages. |
GuillaumeGomez changed the title | ping @lolbinarycat |
| I would prefer using Alternatively, we could just throw the whole command into the error message through it's Debug impl, but that might be a bit noisy. |
GuillaumeGomez force-pushed the graceful-doctest-error-handling branch from 0c495ae to ce42e06 Compare | Thanks, much better this way. |
src/librustdoc/doctest.rs Outdated
| @@ -500,7 +501,7 @@ fn add_exe_suffix(input: String, target: &TargetTuple) -> String { | |||
| input + &exe_suffix | |||
| } | |||
| | |||
| fn wrapped_rustc_command(rustc_wrappers: &[PathBuf], rustc_binary: &Path) -> Command { | |||
| fn wrapped_rustc_command<'a>(rustc_wrappers: &'a [PathBuf], rustc_binary: &'a Path) -> Command { | |||
There was a problem hiding this comment.
Why the explicit lifetime? leftover from when it returned a tuple?
There was a problem hiding this comment.
Oups indeed, removing that.
src/librustdoc/doctest.rs Outdated
| @@ -670,7 +671,14 @@ fn run_test( | |||
| | |||
| debug!("compiler invocation for doctest: {compiler:?}"); | |||
| | |||
| let mut child = compiler.spawn().expect("Failed to spawn rustc process"); | |||
| let binary_path = compiler.get_program().to_os_string(); | |||
There was a problem hiding this comment.
I don't think we need to clone this, it's not like spawn consumes the command or holds a mutable borrow to it, so we just need to make sure we're calling get_program after spawn.
There was a problem hiding this comment.
I was sure spawn was taking self and not &mut self. Well, good to know. :)
src/librustdoc/doctest.rs Outdated
| let binary_path = runner_compiler.get_program().to_os_string(); | ||
| let mut child_runner = match runner_compiler.spawn() { | ||
| Ok(child) => child, | ||
| Err(error) => { | ||
| eprintln!("Failed to spawn {binary_path:?}: {error:?}"); | ||
| return (Duration::default(), Err(TestFailure::CompileError)); | ||
| } |
There was a problem hiding this comment.
| let binary_path = runner_compiler.get_program().to_os_string(); | |
| let mut child_runner = match runner_compiler.spawn() { | |
| Ok(child) => child, | |
| Err(error) => { | |
| eprintln!("Failed to spawn {binary_path:?}: {error:?}"); | |
| return (Duration::default(), Err(TestFailure::CompileError)); | |
| } | |
| let mut child_runner = match runner_compiler.spawn() { | |
| Ok(child) => child, | |
| Err(error) => { | |
| let binary_path = runner_compiler.get_program(); | |
| eprintln!("Failed to spawn {binary_path:?}: {error:?}"); | |
| return (Duration::default(), Err(TestFailure::CompileError)); | |
| } |
as explained above
| .arg("--test-builder") | ||
| .arg(&absolute_path) | ||
| .run_fail(); | ||
| |
There was a problem hiding this comment.
Lets also assert that the exit code is 1 (ICE gives 101, like other panics), seems more robust than string comparisons.
There was a problem hiding this comment.
panic! isn't always 101 as exit code (depends on the target). So we can't really check for that. ^^'
Discovered that while reading https://github.com/rust-lang/rust/blob/master/library/test/src/test_result.rs#L107.
There was a problem hiding this comment.
I mean, you can just stick it behind a cfg(unix) or something, it would still me more reliable than only looking at the strings.
Is normal error exit always 1? Because that's what I was reccomending we actually put in the assertion.
GuillaumeGomez force-pushed the graceful-doctest-error-handling branch from ce42e06 to 29fe476 Compare 
| This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
| Updated! |
3 participants
Fixes bug reported in this comment.
r? @lolbinarycat