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

Report an estimate of remaining steps. #178

Merged
merged 2 commits into from
Jul 29, 2022

Conversation

ehuss
Copy link
Collaborator

@ehuss ehuss commented Jul 28, 2022

This adds some reporting to let the user know more about what cargo-bisect-rustc is doing, and provides an estimate of how many more steps it is going to do.

This is not perfect by any means, but seems to provide a decent enough approximation. Some limitations:

  • It doesn't consider toolchains that fail to install (unknown_ranges).
  • It doesn't differentiate between nightlies and commits. So if you start with nightlies it will tell you N steps left, and then when it starts testing commits, the estimate starts over again.
  • It doesn't include the initial start/end check in the estimate. It might be nice, but not trivial to handle.
  • It doesn't consider the last check in print_results. Ideally that step should be removed (see Nightly can be checked thrice in certain cases #85).

Fixes #143

@oli-obk
Copy link
Contributor

oli-obk commented Jul 28, 2022

The behaviour looks good to me. The only impl concern I have is that it distracts from the actual logic. Maybe move the two new fields into a struct and add named methods for all the cases. Then also move cache into the struct and predicate to be a method on the struct.

@ehuss
Copy link
Collaborator Author

ehuss commented Jul 29, 2022

I'm having a bit of difficulty fully understanding your suggestion. remaining and estimate need to be recomputed during each loop, so I'm not sure if they benefit from being in a struct.

Can you say more about what you are thinking?

I pushed a change to move the computation into the closure so that it does not distract from the central algorithm. I also removed the hack adjusting the estimate during an Unknown scan, since that estimate will jump around or otherwise be wrong since unknown_ranges is not considered.

Otherwise I'm having a hard time seeing a way to simplify it more.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 29, 2022

Ah 🤦 I misread the logic. The new thing is much easier to grok, so mission accomplished!

@oli-obk oli-obk merged commit e475d79 into rust-lang:master Jul 29, 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.

feature: report expected number of steps remaining
2 participants