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

add timeout functionality for bisecting hangs #135

Merged
merged 1 commit into from
Oct 8, 2021
Merged

add timeout functionality for bisecting hangs #135

merged 1 commit into from
Oct 8, 2021

Conversation

savente93
Copy link
Contributor

This is in response to #84. In addition to all the tests still running, I've manually tested it by using it trying to bisect rust-lang/rust#75860 and it seemed to work fine there. Seeing as it's a very small change I'm assuming that that is sufficient, but if more is needed, please let me know.

Couple of things to mention:

  • I have not tested the code on windows as I don't have an env for that, but given that the code uses the std::process::Command and Windows also has a similar functinality I'm assuming that that will be sorted out automatically
  • I'm not a very experienced rust dev so the clones etc. in line 340-342 are probably not necessary but I couldn't figure out how to do it without. If someone knows a better way, feedback is welcome.
    Hope this helps, If there is anything that's missing, please let me know!

Copy link
Contributor

@12101111 12101111 left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. It works fine for me, expect the typo here.

src/toolchains.rs Outdated Show resolved Hide resolved
cmd
}
(None, Some(timeout)) => {
// let mut cmd = Command::new("cargo");
Copy link
Member

Choose a reason for hiding this comment

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

Remove this?

@spastorino
Copy link
Member

Leaving this up to @ehuss

@ehuss
Copy link
Collaborator

ehuss commented Oct 8, 2021

Thanks! I removed the stray comment and resolved the FIXME and squashed the commits.

This probably won't work on Windows, so if someone wants to follow up with a different solution, that is always an option.

@ehuss ehuss merged commit 34b0328 into rust-lang:master Oct 8, 2021
@ehuss ehuss mentioned this pull request Oct 8, 2021
@hkratz
Copy link

hkratz commented Oct 8, 2021

Superb, thanks everybody!

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.

5 participants