Skip to content

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

Merged
merged 4 commits into from
Jun 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
224 changes: 224 additions & 0 deletions src/bounds.rs
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 },
Copy link
Member

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?

Copy link
Collaborator Author

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 the Bounds 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.

/// 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()))?;
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The RustRepositoryAccessor::commit function returns the merge base with the master branch. So it essentially looks for the point where the given tag branched off the master branch. It does not return the commit associated with that tag.

That means it does not include any fixes merged to the beta branch, but should provide a close approximation of what the equivalent nightly was.

(This particular code is unchanged from the original, and has been working sufficiently well for me.)

Copy link
Member

Choose a reason for hiding this comment

The 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)
}
9 changes: 9 additions & 0 deletions src/github.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,15 @@ impl CommitsQuery<'_> {
.url();

let response: Response = client.get(&url).send()?;
let status = response.status();
if !status.is_success() {
bail!(
"error: url <{}> response {}: {}",
url,
status,
response.text().unwrap_or_else(|_| format!("<empty>"))
);
}

let action = parse_paged_elems(response, |elem: GithubCommitElem| {
let found_last = elem.sha == self.earliest_sha;
Expand Down
Loading