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

Teach rustc-fake to tell the truth about subprocess exit statuses #1366

Merged
merged 3 commits into from
Jul 23, 2022

Conversation

yaahc
Copy link
Member

@yaahc yaahc commented Jul 23, 2022

This resolves the ci failure observed in rust-lang/rust#99431

turns out the impl of fn exec() in rustc-perf's rustc-fake is incorrect on windows and only reports a failure when the subprocess fails to spawn, but if it spawns correctly it always reports success rather than checking the exit status of the subprocess. This causes the probe in anyhow to succeed and enable #[cfg(backtrace)] which then causes anyhow to fail to compile, but because it was inside of rustc-fake it reports success and continues! This happens quite a bit and we get some very exciting compiler errors when RUST_LOG=collector=trace is set.

collector/src/rustc-fake.rs Outdated Show resolved Hide resolved
@yaahc
Copy link
Member Author

yaahc commented Jul 23, 2022

nika has also suggested we may also want to make it so exec never returns in any case, and change the return type of the fn to !

collector/src/rustc-fake.rs Outdated Show resolved Hide resolved
Mark-Simulacrum and others added 2 commits July 22, 2022 21:57
Co-authored-by: Eduard-Mihai Burtescu <edy.burt@gmail.com>
Co-authored-by: Jane Losare-Lusby <jlusby42@gmail.com>
@Mark-Simulacrum Mark-Simulacrum enabled auto-merge (squash) July 23, 2022 02:55
@Mark-Simulacrum Mark-Simulacrum merged commit 3c25313 into rust-lang:master Jul 23, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 30, 2022
@@ -467,8 +467,9 @@ fn process_self_profile_output(prof_out_dir: PathBuf, args: &[OsString]) {
#[cfg(windows)]
fn exec(cmd: &mut Command) {
Copy link
Member

Choose a reason for hiding this comment

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

FWIW using return type -> ! (like the Unix version) would have caught this

Copy link
Member

Choose a reason for hiding this comment

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

Ahh I forgot that part when I posted @mystor's suggestion (#1366 (comment)).

@RalfJung RalfJung mentioned this pull request Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants