Skip to content

Better error on absent next commit #1120

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.

Sign up for GitHub

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 1 commit into from
Dec 14, 2021
Merged

Better error on absent next commit #1120

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 additions & 1 deletion site/src/comparison.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,13 @@ pub async fn handle_triage(
let start_artifact = ctxt
.artifact_id_for_bound(start.clone(), true)
.ok_or(format!("could not find start commit for bound {:?}", start))?;
// This gives a better error, but is still not great -- the common case here
// is that we've had a 422 error and as such had a fork. It's possible we
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain this bit "422 error and as such had a fork" more?

Copy link
Member Author

Choose a reason for hiding this comment

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

When bors fails to push with a 422 error (e.g., rust-lang/rust#91279 (comment)), which really shouldn't happen but isn't fixed yet -- see rust-lang/homu#75 for the issue tracking that bug -- we end up in a state where triagebot + rustc-perf effectively believe a commit has landed on master, but that commit hasn't actually landed. This is because triagebot looks for comments like this one rust-lang/rust#91279 (comment) when discovering new rustc master commits, not actually a push to the master branch on rust-lang/rust. (Plausible that this could get fixed alongside rust-lang/triagebot#1483).

The net effect is that we end up in a situation where this not-actually-on-master commit is believed to be, which means that when the next commit does land to rust-lang/rust master, we have a fork in the otherwise linear history of bors merges from triagebot's perspective, which means that next_commit stops working correctly on the rustc-perf side (there's two next commits, probably we choose a semi-random one?).

// could diagnose that and give a nicer error here telling the user which
// commit to use.
let mut next = next_commit(&start_artifact, &master_commits)
.map(|c| Bound::Commit(c.sha.clone()))
.unwrap(); // TODO: handle no next commit
.ok_or(format!("no next commit for {:?}", start_artifact))?;

let mut report = HashMap::new();
let mut before = start.clone();
Expand Down