-
Notifications
You must be signed in to change notification settings - Fork 57
Refactor bounds definition. #325
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
Changes from all commits
e26646c
bb4b61f
0ec0fb3
b6dafd2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,224 @@ | ||
//! Definitions of bisection bounds. | ||
|
||
use crate::toolchains::{ | ||
download_progress, parse_to_naive_date, Toolchain, NIGHTLY_SERVER, YYYY_MM_DD, | ||
}; | ||
use crate::GitDate; | ||
use crate::Opts; | ||
use crate::{today, EPOCH_COMMIT}; | ||
use anyhow::bail; | ||
use chrono::NaiveDate; | ||
use reqwest::blocking::Client; | ||
use std::io::Read; | ||
use std::str::FromStr; | ||
|
||
/// A bisection boundary. | ||
#[derive(Clone, Debug)] | ||
pub enum Bound { | ||
Commit(String), | ||
Date(GitDate), | ||
} | ||
|
||
impl FromStr for Bound { | ||
type Err = std::convert::Infallible; | ||
fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
parse_to_naive_date(s) | ||
.map(Self::Date) | ||
.or_else(|_| Ok(Self::Commit(s.to_string()))) | ||
} | ||
} | ||
|
||
impl Bound { | ||
/// Returns the SHA of this boundary. | ||
/// | ||
/// For nightlies, this will fetch from the network. | ||
pub fn sha(&self) -> anyhow::Result<String> { | ||
match self { | ||
Bound::Commit(commit) => Ok(commit.clone()), | ||
Bound::Date(date) => { | ||
let date_str = date.format(YYYY_MM_DD); | ||
let url = | ||
format!("{NIGHTLY_SERVER}/{date_str}/channel-rust-nightly-git-commit-hash.txt"); | ||
|
||
eprintln!("fetching {url}"); | ||
let client = Client::new(); | ||
let name = format!("nightly manifest {date_str}"); | ||
let mut response = download_progress(&client, &name, &url)?; | ||
let mut commit = String::new(); | ||
response.read_to_string(&mut commit)?; | ||
|
||
eprintln!("converted {date_str} to {commit}"); | ||
|
||
Ok(commit) | ||
} | ||
} | ||
} | ||
} | ||
|
||
/// The starting bisection bounds. | ||
pub enum Bounds { | ||
/// Indicates to search backwards from the given date to find the start | ||
/// date where the regression does not occur. | ||
SearchNightlyBackwards { end: GitDate }, | ||
/// Search between two commits. | ||
/// | ||
/// `start` and `end` must be SHA1 hashes of the commit object. | ||
Commits { start: String, end: String }, | ||
/// Search between two dates. | ||
Dates { start: GitDate, end: GitDate }, | ||
} | ||
|
||
impl Bounds { | ||
pub fn from_args(args: &Opts) -> anyhow::Result<Bounds> { | ||
let (start, end) = translate_tags(&args)?; | ||
let today = today(); | ||
let check_in_future = |which, date: &NaiveDate| -> anyhow::Result<()> { | ||
if date > &today { | ||
bail!( | ||
"{which} date should be on or before current date, \ | ||
got {which} date request: {date} and current date is {today}" | ||
); | ||
} | ||
Ok(()) | ||
}; | ||
let bounds = match (start, end) { | ||
// Neither --start or --end specified. | ||
(None, None) => Bounds::SearchNightlyBackwards { | ||
end: installed_nightly_or_latest()?, | ||
}, | ||
|
||
// --start or --end is a commit | ||
(Some(Bound::Commit(start)), Some(Bound::Commit(end))) => { | ||
Bounds::Commits { start, end } | ||
} | ||
(Some(Bound::Commit(start)), None) => Bounds::Commits { | ||
start, | ||
end: args.access.repo().commit("origin/master")?.sha, | ||
}, | ||
(None, Some(Bound::Commit(end))) => Bounds::Commits { | ||
start: EPOCH_COMMIT.to_string(), | ||
end, | ||
}, | ||
|
||
// --start or --end is a date | ||
(Some(Bound::Date(start)), Some(Bound::Date(end))) => { | ||
check_in_future("start", &start)?; | ||
check_in_future("end", &end)?; | ||
Bounds::Dates { start, end } | ||
} | ||
(Some(Bound::Date(start)), None) => { | ||
check_in_future("start", &start)?; | ||
Bounds::Dates { | ||
start, | ||
end: find_latest_nightly()?, | ||
} | ||
} | ||
(None, Some(Bound::Date(end))) => { | ||
check_in_future("end", &end)?; | ||
if args.by_commit { | ||
bail!("--by-commit with an end date requires --start to be specified"); | ||
} | ||
Bounds::SearchNightlyBackwards { end } | ||
} | ||
|
||
// Mixed not supported. | ||
(Some(Bound::Commit(_)), Some(Bound::Date(_))) | ||
| (Some(Bound::Date(_)), Some(Bound::Commit(_))) => bail!( | ||
"cannot take different types of bounds for start/end, \ | ||
got start: {:?} and end {:?}", | ||
args.start, | ||
args.end | ||
), | ||
}; | ||
if let Bounds::Dates { start, end } = &bounds { | ||
if end < start { | ||
bail!("end should be after start, got start: {start} and end {end}"); | ||
} | ||
if args.by_commit { | ||
eprintln!("finding commit range that corresponds to dates specified"); | ||
let bounds = Bounds::Commits { | ||
start: date_to_sha(&start)?, | ||
end: date_to_sha(&end)?, | ||
}; | ||
return Ok(bounds); | ||
} | ||
} | ||
Ok(bounds) | ||
} | ||
} | ||
|
||
/// Translates a tag-like bound (such as `1.62.0`) to a `Bound::Date` so that | ||
/// bisecting works for versions older than 167 days. | ||
fn translate_tags(args: &Opts) -> anyhow::Result<(Option<Bound>, Option<Bound>)> { | ||
let is_tag = |bound: &Option<Bound>| -> bool { | ||
match bound { | ||
Some(Bound::Commit(commit)) => commit.contains('.'), | ||
None | Some(Bound::Date(_)) => false, | ||
} | ||
}; | ||
let is_datelike = |bound: &Option<Bound>| -> bool { | ||
matches!(bound, None | Some(Bound::Date(_))) || is_tag(bound) | ||
}; | ||
if !(is_datelike(&args.start) && is_datelike(&args.end)) { | ||
// If the user specified an actual commit for one bound, then don't | ||
// even try to convert the other bound to a date. | ||
return Ok((args.start.clone(), args.end.clone())); | ||
} | ||
let fixup = |which: &str, bound: &Option<Bound>| -> anyhow::Result<Option<Bound>> { | ||
if is_tag(bound) { | ||
if let Some(Bound::Commit(tag)) = bound { | ||
let date = args | ||
.access | ||
.repo() | ||
.bound_to_date(Bound::Commit(tag.clone()))?; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels wrong to me. If we're looking up (say) 1.68.0, we presumably need something here to say that we want the stable release as of that date or otherwise go from the stable release to the nightly date range it "points" at? Otherwise we're missing ~6 weeks here, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The That means it does not include any fixes merged to the (This particular code is unchanged from the original, and has been working sufficiently well for me.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I missed that aspect. Sounds good to me. |
||
eprintln!( | ||
"translating --{which}={tag} to {date}", | ||
date = date.format(YYYY_MM_DD) | ||
); | ||
return Ok(Some(Bound::Date(date))); | ||
} | ||
} | ||
Ok(bound.clone()) | ||
}; | ||
Ok((fixup("start", &args.start)?, fixup("end", &args.end)?)) | ||
} | ||
|
||
/// Returns the commit SHA of the nightly associated with the given date. | ||
fn date_to_sha(date: &NaiveDate) -> anyhow::Result<String> { | ||
let date_str = date.format(YYYY_MM_DD); | ||
let url = format!("{NIGHTLY_SERVER}/{date_str}/channel-rust-nightly-git-commit-hash.txt"); | ||
|
||
eprintln!("fetching {url}"); | ||
let client = Client::new(); | ||
let name = format!("nightly manifest {date_str}"); | ||
let mut response = download_progress(&client, &name, &url)?; | ||
let mut commit = String::new(); | ||
response.read_to_string(&mut commit)?; | ||
|
||
eprintln!("converted {date_str} to {commit}"); | ||
|
||
Ok(commit) | ||
} | ||
|
||
/// Returns the date of the nightly toolchain currently installed. If no | ||
/// nightly is found, then it goes to the network to determine the date of the | ||
/// latest nightly. | ||
fn installed_nightly_or_latest() -> anyhow::Result<GitDate> { | ||
if let Some(date) = Toolchain::default_nightly() { | ||
return Ok(date); | ||
} | ||
find_latest_nightly() | ||
} | ||
|
||
/// Returns the date of the latest nightly (fetched from the network). | ||
fn find_latest_nightly() -> anyhow::Result<GitDate> { | ||
let url = format!("{NIGHTLY_SERVER}/channel-rust-nightly-date.txt"); | ||
eprintln!("fetching {url}"); | ||
let client = Client::new(); | ||
let mut response = download_progress(&client, "nightly date", &url)?; | ||
let mut body = String::new(); | ||
response.read_to_string(&mut body)?; | ||
let date = NaiveDate::parse_from_str(&body, "%Y-%m-%d")?; | ||
eprintln!("determined the latest nightly is {date}"); | ||
Ok(date) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, these aren't actually commits -- or at least indirectly so. E.g.,
origin/master
is valid. Maybe we can note that in the comment?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! So there was only ever one case where it wasn't a SHA, and that was the
origin/master
case (when--end
was not specified). This was later converted to a SHA. To keep things simple, I shifted the conversion earlier to when theBounds
is created so that the rest of the code can safely assume both values are a SHA.The SHAs can still be shortened, and that can still be a source of bugs, but for the most part it works. That seems like something that could be further improved in the future to make it more robust.