diff --git a/site/src/github.rs b/site/src/github.rs index 0a03f2a28..68ffc798c 100644 --- a/site/src/github.rs +++ b/site/src/github.rs @@ -1,40 +1,19 @@ pub mod client; +pub mod comparison_summary; use crate::api::github::{Commit, Issue}; -use crate::comparison::{ - deserves_attention_icount, write_summary_table, write_summary_table_footer, ArtifactComparison, - ArtifactComparisonSummary, Direction, Metric, -}; use crate::load::{SiteCtxt, TryCommit}; -use anyhow::Context as _; -use database::{ArtifactId, QueuedCommit}; use serde::Deserialize; -use std::collections::HashSet; - -use std::{fmt::Write, sync::Arc, time::Duration}; - type BoxedError = Box; -pub async fn get_authorized_users() -> Result, String> { - let url = format!("{}/permissions/perf.json", ::rust_team_data::v1::BASE_URL); - let client = reqwest::Client::new(); - client - .get(&url) - .send() - .await - .map_err(|err| format!("failed to fetch authorized users: {}", err))? - .error_for_status() - .map_err(|err| format!("failed to fetch authorized users: {}", err))? - .json::() - .await - .map_err(|err| format!("failed to fetch authorized users: {}", err)) - .map(|perms| perms.github_ids) -} +pub use comparison_summary::post_finished; /// Enqueues try builds on the try-perf branch for every rollup merge in `rollup_merges`. /// Returns a mapping between the rollup merge commit and the try build sha. +/// +/// `rollup_merges` must only include actual rollup merge commits. pub async fn enqueue_unrolled_try_builds<'a>( client: client::Client, rollup_merges: impl Iterator, @@ -90,7 +69,10 @@ lazy_static::lazy_static! { } // Gets the pr number for the associated rollup PR message. Returns None if this is not a rollup PR -pub async fn rollup_pr(client: &client::Client, message: &str) -> Result, String> { +pub async fn rollup_pr_number( + client: &client::Client, + message: &str, +) -> Result, String> { if !message.starts_with("Auto merge of") { return Ok(None); } @@ -119,169 +101,6 @@ pub async fn rollup_pr(client: &client::Client, message: &str) -> Result, - repository_url: &str, - rollup_merge_sha: &str, - origin_url: &str, -) -> anyhow::Result { - let client = client::Client::from_ctxt(&ctxt, repository_url.to_owned()); - log::trace!( - "creating PR for {:?} {:?}", - repository_url, - rollup_merge_sha - ); - let branch = branch_for_rollup(&ctxt, repository_url, rollup_merge_sha).await?; - - let pr = client - .create_pr( - &format!( - "[DO NOT MERGE] perf-test for #{}", - branch.rolled_up_pr_number - ), - &format!("rust-timer:{}", branch.name), - "master", - &format!( - "This is an automatically generated pull request (from [here]({})) to \ - run perf tests for #{} which merged in a rollup. - -r? @ghost", - origin_url, branch.rolled_up_pr_number - ), - true, - ) - .await - .context("Created PR")?; - - let pr_number = pr.number; - let rollup_merge_sha = rollup_merge_sha.to_owned(); - tokio::task::spawn(async move { - // Give github time to create the merge commit reference - tokio::time::sleep(Duration::from_secs(30)).await; - // This provides the master SHA so that we can check that we only queue - // an appropriate try build. If there's ever a race condition, i.e., - // master was pushed while this command was running, the user will have to - // take manual action to detect it. - // - // Eventually we'll want to handle this automatically, but that's a ways - // off: we'd need to store the state in the database and handle the try - // build starting and generally that's a lot of work for not too much gain. - client - .post_comment( - pr.number, - &format!( - "@bors try @rust-timer queue - -The try commit's (master) parent should be {master}. If it isn't, \ -then please: - - * Stop this try build (`try-`). - * Run `@rust-timer update-pr-for {merge}`. - * Rerun `bors try`. - -You do not need to reinvoke the queue command as long as the perf \ -build hasn't yet started.", - master = branch.master_base_sha, - merge = rollup_merge_sha, - ), - ) - .await; - }); - - Ok(pr_number) -} - -pub struct RollupBranch { - pub master_base_sha: String, - pub rolled_up_pr_number: u32, - pub name: String, -} - -pub async fn branch_for_rollup( - ctxt: &SiteCtxt, - repository_url: &str, - rollup_merge_sha: &str, -) -> anyhow::Result { - let client = client::Client::from_ctxt(ctxt, repository_url.to_owned()); - let timer = "https://api.github.com/repos/rust-timer/rust"; - let timer_client = client::Client::from_ctxt(ctxt, timer.to_owned()); - let rollup_merge = client - .get_commit(rollup_merge_sha) - .await - .context("got rollup merge")?; - - let mut current = rollup_merge.clone(); - loop { - log::trace!("searching for auto branch, at {:?}", current.sha); - if current.commit.message.starts_with("Auto merge") { - break; - } - assert_eq!(current.parents.len(), 2); - current = client - .get_commit(¤t.parents[0].sha) - .await - .context("success master get")?; - } - let old_master_commit = current; - - let current_master_commit = client - .get_commit("master") - .await - .context("success master get")?; - - let revert_sha = timer_client - .create_commit( - &format!("Revert to {}", old_master_commit.sha), - &old_master_commit.commit.tree.sha, - &[¤t_master_commit.sha], - ) - .await - .context("create revert")?; - - let merge_sha = timer_client - .create_commit( - &format!( - "rust-timer simulated merge of {}\n\nOriginal message:\n{}", - rollup_merge.sha, rollup_merge.commit.message - ), - &rollup_merge.commit.tree.sha, - &[&revert_sha], - ) - .await - .context("create merge commit")?; - - let rolled_up_pr_number = if let Some(stripped) = rollup_merge - .commit - .message - .strip_prefix("Rollup merge of #") - { - stripped - .split_whitespace() - .next() - .unwrap() - .parse::() - .unwrap() - } else { - anyhow::bail!( - "not a rollup merge commit: {:?}", - rollup_merge.commit.message - ) - }; - - let branch = format!("try-for-{}", rolled_up_pr_number); - timer_client - .create_ref(&format!("refs/heads/{}", branch), &merge_sha) - .await - .context("created branch")?; - - Ok(RollupBranch { - rolled_up_pr_number, - master_base_sha: current_master_commit.sha, - name: branch, - }) -} - pub async fn enqueue_sha(issue: Issue, ctxt: &SiteCtxt, commit: String) -> Result<(), String> { let client = client::Client::from_ctxt(ctxt, issue.repository_url.clone()); let commit_response = client @@ -354,344 +173,22 @@ pub async fn parse_homu_comment(comment_body: &str) -> Option { Some(sha) } -/// Post messages to GitHub for all queued commits that have -/// not yet been marked as completed. -pub async fn post_finished(ctxt: &SiteCtxt) { - // If the github token is not configured, do not run this -- we don't want - // to mark things as complete without posting the comment. - if ctxt.config.keys.github_api_token.is_none() { - return; - } - let conn = ctxt.conn().await; - let index = ctxt.index.load(); - let mut known_commits = index - .commits() - .into_iter() - .map(|c| c.sha.to_string()) - .collect::>(); - let (master_commits, queued_pr_commits, in_progress_artifacts) = futures::join!( - collector::master_commits(), - conn.queued_commits(), - conn.in_progress_artifacts() - ); - let master_commits = match master_commits { - Ok(mcs) => mcs.into_iter().map(|c| c.sha).collect::>(), - Err(e) => { - log::error!("posting finished did not load master commits: {:?}", e); - // If we can't fetch master commits, return. - // We'll eventually try again later - return; - } - }; - - for aid in in_progress_artifacts { - match aid { - ArtifactId::Commit(c) => { - known_commits.remove(&c.sha); - } - ArtifactId::Tag(_) => { - // do nothing, for now, though eventually we'll want an artifact queue - } - } - } - for queued_commit in queued_pr_commits - .into_iter() - .filter(|c| known_commits.contains(&c.sha)) - { - if let Some(completed) = conn.mark_complete(&queued_commit.sha).await { - assert_eq!(completed, queued_commit); - - let is_master_commit = master_commits.contains(&queued_commit.sha); - post_comparison_comment(ctxt, queued_commit, is_master_commit).await; - } - } -} - -/// Posts a comment to GitHub summarizing the comparison of the queued commit with its parent -/// -/// `is_master_commit` is used to differentiate messages for try runs and post-merge runs. -async fn post_comparison_comment(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit: bool) { - let client = client::Client::from_ctxt( - ctxt, - "https://api.github.com/repos/rust-lang/rust".to_owned(), - ); - let pr = commit.pr; - let body = match summarize_run(ctxt, commit, is_master_commit).await { - Ok(message) => message, - Err(error) => error, - }; - - client.post_comment(pr, body).await; -} - -fn make_comparison_url(commit: &QueuedCommit, stat: Metric) -> String { - format!( - "https://perf.rust-lang.org/compare.html?start={}&end={}&stat={}", - commit.parent_sha, - commit.sha, - stat.as_str() - ) -} - -async fn calculate_metric_comparison( - ctxt: &SiteCtxt, - commit: &QueuedCommit, - metric: Metric, -) -> Result { - match crate::comparison::compare( - collector::Bound::Commit(commit.parent_sha.clone()), - collector::Bound::Commit(commit.sha.clone()), - metric, - ctxt, - ) - .await - { - Ok(Some(c)) => Ok(c), - _ => Err("ERROR categorizing benchmark run!".to_owned()), - } -} - -async fn summarize_run( - ctxt: &SiteCtxt, - commit: QueuedCommit, - is_master_commit: bool, -) -> Result { - let benchmark_map = ctxt.get_benchmark_category_map().await; - - let mut message = format!( - "Finished benchmarking commit ({sha}): [comparison url]({comparison_url}).\n\n", - sha = commit.sha, - comparison_url = make_comparison_url(&commit, Metric::InstructionsUser) - ); - - let inst_comparison = - calculate_metric_comparison(ctxt, &commit, Metric::InstructionsUser).await?; - - let errors = if !inst_comparison.newly_failed_benchmarks.is_empty() { - let benchmarks = inst_comparison - .newly_failed_benchmarks - .iter() - .map(|(benchmark, _)| format!("- {benchmark}")) - .collect::>() - .join("\n"); - format!("\n**Warning ⚠**: The following benchmark(s) failed to build:\n{benchmarks}\n") - } else { - String::new() - }; - let (inst_primary, inst_secondary) = inst_comparison - .clone() - .summarize_by_category(&benchmark_map); - - let mut table_written = false; - let metrics = vec![ - ( - "Instruction count", - Metric::InstructionsUser, - false, - inst_comparison, - ), - ( - "Max RSS (memory usage)", - Metric::MaxRSS, - true, - calculate_metric_comparison(ctxt, &commit, Metric::MaxRSS).await?, - ), - ( - "Cycles", - Metric::CyclesUser, - true, - calculate_metric_comparison(ctxt, &commit, Metric::CyclesUser).await?, - ), - ]; - - for (title, metric, hidden, comparison) in metrics { - message.push_str(&format!( - "\n### [{title}]({})\n", - make_comparison_url(&commit, metric) - )); - - let (primary, secondary) = comparison.summarize_by_category(&benchmark_map); - table_written |= write_metric_summary(primary, secondary, hidden, &mut message).await; - } - - if table_written { - write_summary_table_footer(&mut message); - } - - const DISAGREEMENT: &str = "If you disagree with this performance assessment, \ - please file an issue in [rust-lang/rustc-perf](https://github.com/rust-lang/rustc-perf/issues/new)."; - let footer = format!("{DISAGREEMENT}{errors}"); - - let direction = inst_primary.direction().or(inst_secondary.direction()); - let next_steps = next_steps(inst_primary, inst_secondary, direction, is_master_commit); - - write!(&mut message, "\n{footer}\n{next_steps}").unwrap(); - - Ok(message) -} - -/// Returns true if a summary table was written to `message`. -async fn write_metric_summary( - primary: ArtifactComparisonSummary, - secondary: ArtifactComparisonSummary, - hidden: bool, - message: &mut String, -) -> bool { - if !primary.is_relevant() && !secondary.is_relevant() { - message - .push_str("This benchmark run did not return any relevant results for this metric.\n"); - false - } else { - let primary_short_summary = generate_short_summary(&primary); - let secondary_short_summary = generate_short_summary(&secondary); - - if hidden { - message.push_str("
\nResults\n\n"); - } - - write!( - message, - r#" -- Primary benchmarks: {primary_short_summary} -- Secondary benchmarks: {secondary_short_summary} - -"# - ) - .unwrap(); - - write_summary_table(&primary, &secondary, true, message); - - if hidden { - message.push_str("
\n"); - } - - true - } -} - -fn next_steps( - primary: ArtifactComparisonSummary, - secondary: ArtifactComparisonSummary, - direction: Option, - is_master_commit: bool, -) -> String { - let deserves_attention = deserves_attention_icount(&primary, &secondary); - let (is_regression, label) = match (deserves_attention, direction) { - (true, Some(Direction::Regression | Direction::Mixed)) => (true, "+perf-regression"), - _ => (false, "-perf-regression"), - }; - - if is_master_commit { - master_run_body(is_regression) - } else { - try_run_body(label) - } -} - -fn master_run_body(is_regression: bool) -> String { - if is_regression { - " -**Next Steps**: If you can justify the \ -regressions found in this perf run, please indicate this with \ -`@rustbot label: +perf-regression-triaged` along with \ -sufficient written justification. If you cannot justify the regressions \ -please open an issue or create a new PR that fixes the regressions, \ -add a comment linking to the newly created issue or PR, \ -and then add the `perf-regression-triaged` label to this PR. - -@rustbot label: +perf-regression -cc @rust-lang/wg-compiler-performance -" - } else { - " -@rustbot label: -perf-regression -" - } - .to_string() -} - -fn try_run_body(label: &str) -> String { - let next_steps = if label.starts_with("+") { - "\n\n**Next Steps**: If you can justify the regressions found in \ - this try perf run, please indicate this with \ - `@rustbot label: +perf-regression-triaged` along with \ - sufficient written justification. If you cannot justify the regressions \ - please fix the regressions and do another perf run. If the next run \ - shows neutral or positive results, the label will be automatically removed." - } else { - "" - }; - - format!( - " -Benchmarking this pull request likely means that it is \ -perf-sensitive, so we're automatically marking it as not fit \ -for rolling up. While you can manually mark this PR as fit \ -for rollup, we strongly recommend not doing so since this PR may lead to changes in \ -compiler perf.{next_steps} - -@bors rollup=never -@rustbot label: +S-waiting-on-review -S-waiting-on-perf {label}", - ) -} - -fn generate_short_summary(summary: &ArtifactComparisonSummary) -> String { - // Add an "s" to a word unless there's only one. - fn ending(word: &'static str, count: usize) -> std::borrow::Cow<'static, str> { - if count == 1 { - return word.into(); - } - format!("{}s", word).into() - } - - let num_improvements = summary.number_of_improvements(); - let num_regressions = summary.number_of_regressions(); - - match summary.direction() { - Some(Direction::Improvement) => format!( - "🎉 relevant {} found", - ending("improvement", num_improvements) - ), - Some(Direction::Regression) => format!( - "😿 relevant {} found", - ending("regression", num_regressions) - ), - Some(Direction::Mixed) => "mixed results".to_string(), - None => "no relevant changes found".to_string(), - } -} - +#[derive(serde::Deserialize)] pub(crate) struct PullRequest { pub number: u64, pub title: String, } +#[derive(serde::Deserialize)] +struct PullRequestResponse { + items: Vec, +} + /// Fetch all merged PRs that are labeled with `perf-regression` and not `perf-regression-triaged` pub(crate) async fn untriaged_perf_regressions() -> Result, BoxedError> { let url = "https://api.github.com/search/issues?q=repo:rust-lang/rust+label:perf-regression+-label:perf-regression-triaged+is:merged".to_owned(); let request = github_request(&url); - let body = send_request(request).await?; - Ok(body - .get("items") - .ok_or_else(malformed_json_error)? - .as_array() - .ok_or_else(malformed_json_error)? - .iter() - .map(|v| { - let title = v - .get("title") - .ok_or_else(malformed_json_error)? - .as_str() - .ok_or_else(malformed_json_error)? - .to_owned(); - let number = v - .get("number") - .ok_or_else(malformed_json_error)? - .as_u64() - .ok_or_else(malformed_json_error)?; - Ok(PullRequest { title, number }) - }) - .collect::>()?) + Ok(send_request::(request).await?.items) } /// Get the title of a PR with the given number @@ -700,7 +197,7 @@ pub(crate) async fn pr_title(pr: u32) -> String { let request = github_request(&url); async fn send(request: reqwest::RequestBuilder) -> Result { - let body = send_request(request).await?; + let body = send_request::(request).await?; Ok(body .get("title") .ok_or_else(malformed_json_error)? @@ -729,12 +226,14 @@ fn github_request(url: &str) -> reqwest::RequestBuilder { request } -async fn send_request(request: reqwest::RequestBuilder) -> Result { +async fn send_request( + request: reqwest::RequestBuilder, +) -> Result { Ok(request .send() .await? .error_for_status()? - .json::() + .json::() .await?) } diff --git a/site/src/github/client.rs b/site/src/github/client.rs index dd9fd68b8..c18235196 100644 --- a/site/src/github/client.rs +++ b/site/src/github/client.rs @@ -187,7 +187,7 @@ impl Client { .map_err(|e| anyhow::anyhow!("cannot deserialize commit: {:?}", e)) } - pub async fn post_comment(&self, pr: u32, body: B) + pub async fn post_comment(&self, pr_number: u32, body: B) where B: Into, { @@ -198,7 +198,10 @@ impl Client { let body = body.into(); let req = self .inner - .post(&format!("{}/issues/{}/comments", self.repository_url, pr)) + .post(&format!( + "{}/issues/{}/comments", + self.repository_url, pr_number + )) .json(&PostComment { body: body.to_owned(), }); diff --git a/site/src/github/comparison_summary.rs b/site/src/github/comparison_summary.rs new file mode 100644 index 000000000..e7b25deb8 --- /dev/null +++ b/site/src/github/comparison_summary.rs @@ -0,0 +1,317 @@ +use crate::comparison::{ + deserves_attention_icount, write_summary_table, write_summary_table_footer, ArtifactComparison, + ArtifactComparisonSummary, Direction, Metric, +}; +use crate::load::SiteCtxt; + +use database::{ArtifactId, QueuedCommit}; + +use std::collections::HashSet; +use std::fmt::Write; + +/// Post messages to GitHub for all queued commits that have +/// not yet been marked as completed. +pub async fn post_finished(ctxt: &SiteCtxt) { + // If the github token is not configured, do not run this -- we don't want + // to mark things as complete without posting the comment. + if ctxt.config.keys.github_api_token.is_none() { + return; + } + let conn = ctxt.conn().await; + let index = ctxt.index.load(); + let mut known_commits = index + .commits() + .into_iter() + .map(|c| c.sha.to_string()) + .collect::>(); + let (master_commits, queued_pr_commits, in_progress_artifacts) = futures::join!( + collector::master_commits(), + conn.queued_commits(), + conn.in_progress_artifacts() + ); + let master_commits = match master_commits { + Ok(mcs) => mcs.into_iter().map(|c| c.sha).collect::>(), + Err(e) => { + log::error!("posting finished did not load master commits: {:?}", e); + // If we can't fetch master commits, return. + // We'll eventually try again later + return; + } + }; + + for aid in in_progress_artifacts { + match aid { + ArtifactId::Commit(c) => { + known_commits.remove(&c.sha); + } + ArtifactId::Tag(_) => { + // do nothing, for now, though eventually we'll want an artifact queue + } + } + } + for queued_commit in queued_pr_commits + .into_iter() + .filter(|c| known_commits.contains(&c.sha)) + { + if let Some(completed) = conn.mark_complete(&queued_commit.sha).await { + assert_eq!(completed, queued_commit); + + let is_master_commit = master_commits.contains(&queued_commit.sha); + post_comparison_comment(ctxt, queued_commit, is_master_commit).await; + } + } +} + +/// Posts a comment to GitHub summarizing the comparison of the queued commit with its parent +/// +/// `is_master_commit` is used to differentiate messages for try runs and post-merge runs. +async fn post_comparison_comment(ctxt: &SiteCtxt, commit: QueuedCommit, is_master_commit: bool) { + let client = super::client::Client::from_ctxt( + ctxt, + "https://api.github.com/repos/rust-lang/rust".to_owned(), + ); + let pr = commit.pr; + let body = match summarize_run(ctxt, commit, is_master_commit).await { + Ok(message) => message, + Err(error) => error, + }; + + client.post_comment(pr, body).await; +} + +fn make_comparison_url(commit: &QueuedCommit, stat: Metric) -> String { + format!( + "https://perf.rust-lang.org/compare.html?start={}&end={}&stat={}", + commit.parent_sha, + commit.sha, + stat.as_str() + ) +} + +async fn calculate_metric_comparison( + ctxt: &SiteCtxt, + commit: &QueuedCommit, + metric: Metric, +) -> Result { + match crate::comparison::compare( + collector::Bound::Commit(commit.parent_sha.clone()), + collector::Bound::Commit(commit.sha.clone()), + metric, + ctxt, + ) + .await + { + Ok(Some(c)) => Ok(c), + _ => Err("ERROR categorizing benchmark run!".to_owned()), + } +} + +async fn summarize_run( + ctxt: &SiteCtxt, + commit: QueuedCommit, + is_master_commit: bool, +) -> Result { + let benchmark_map = ctxt.get_benchmark_category_map().await; + + let mut message = format!( + "Finished benchmarking commit ({sha}): [comparison url]({comparison_url}).\n\n", + sha = commit.sha, + comparison_url = make_comparison_url(&commit, Metric::InstructionsUser) + ); + + let inst_comparison = + calculate_metric_comparison(ctxt, &commit, Metric::InstructionsUser).await?; + + let errors = if !inst_comparison.newly_failed_benchmarks.is_empty() { + let benchmarks = inst_comparison + .newly_failed_benchmarks + .iter() + .map(|(benchmark, _)| format!("- {benchmark}")) + .collect::>() + .join("\n"); + format!("\n**Warning ⚠**: The following benchmark(s) failed to build:\n{benchmarks}\n") + } else { + String::new() + }; + let (inst_primary, inst_secondary) = inst_comparison + .clone() + .summarize_by_category(&benchmark_map); + + let mut table_written = false; + let metrics = vec![ + ( + "Instruction count", + Metric::InstructionsUser, + false, + inst_comparison, + ), + ( + "Max RSS (memory usage)", + Metric::MaxRSS, + true, + calculate_metric_comparison(ctxt, &commit, Metric::MaxRSS).await?, + ), + ( + "Cycles", + Metric::CyclesUser, + true, + calculate_metric_comparison(ctxt, &commit, Metric::CyclesUser).await?, + ), + ]; + + for (title, metric, hidden, comparison) in metrics { + message.push_str(&format!( + "\n### [{title}]({})\n", + make_comparison_url(&commit, metric) + )); + + let (primary, secondary) = comparison.summarize_by_category(&benchmark_map); + table_written |= write_metric_summary(primary, secondary, hidden, &mut message).await; + } + + if table_written { + write_summary_table_footer(&mut message); + } + + const DISAGREEMENT: &str = "If you disagree with this performance assessment, \ + please file an issue in [rust-lang/rustc-perf](https://github.com/rust-lang/rustc-perf/issues/new)."; + let footer = format!("{DISAGREEMENT}{errors}"); + + let direction = inst_primary.direction().or(inst_secondary.direction()); + let next_steps = next_steps(inst_primary, inst_secondary, direction, is_master_commit); + + write!(&mut message, "\n{footer}\n{next_steps}").unwrap(); + + Ok(message) +} + +/// Returns true if a summary table was written to `message`. +async fn write_metric_summary( + primary: ArtifactComparisonSummary, + secondary: ArtifactComparisonSummary, + hidden: bool, + message: &mut String, +) -> bool { + if !primary.is_relevant() && !secondary.is_relevant() { + message + .push_str("This benchmark run did not return any relevant results for this metric.\n"); + false + } else { + let primary_short_summary = generate_short_summary(&primary); + let secondary_short_summary = generate_short_summary(&secondary); + + if hidden { + message.push_str("
\nResults\n\n"); + } + + write!( + message, + r#" +- Primary benchmarks: {primary_short_summary} +- Secondary benchmarks: {secondary_short_summary} + +"# + ) + .unwrap(); + + write_summary_table(&primary, &secondary, true, message); + + if hidden { + message.push_str("
\n"); + } + + true + } +} + +fn next_steps( + primary: ArtifactComparisonSummary, + secondary: ArtifactComparisonSummary, + direction: Option, + is_master_commit: bool, +) -> String { + let deserves_attention = deserves_attention_icount(&primary, &secondary); + let (is_regression, label) = match (deserves_attention, direction) { + (true, Some(Direction::Regression | Direction::Mixed)) => (true, "+perf-regression"), + _ => (false, "-perf-regression"), + }; + + if is_master_commit { + master_run_body(is_regression) + } else { + try_run_body(label) + } +} + +fn master_run_body(is_regression: bool) -> String { + if is_regression { + " +**Next Steps**: If you can justify the \ +regressions found in this perf run, please indicate this with \ +`@rustbot label: +perf-regression-triaged` along with \ +sufficient written justification. If you cannot justify the regressions \ +please open an issue or create a new PR that fixes the regressions, \ +add a comment linking to the newly created issue or PR, \ +and then add the `perf-regression-triaged` label to this PR. + +@rustbot label: +perf-regression +cc @rust-lang/wg-compiler-performance +" + } else { + " +@rustbot label: -perf-regression +" + } + .to_string() +} + +fn try_run_body(label: &str) -> String { + let next_steps = if label.starts_with("+") { + "\n\n**Next Steps**: If you can justify the regressions found in \ + this try perf run, please indicate this with \ + `@rustbot label: +perf-regression-triaged` along with \ + sufficient written justification. If you cannot justify the regressions \ + please fix the regressions and do another perf run. If the next run \ + shows neutral or positive results, the label will be automatically removed." + } else { + "" + }; + + format!( + " +Benchmarking this pull request likely means that it is \ +perf-sensitive, so we're automatically marking it as not fit \ +for rolling up. While you can manually mark this PR as fit \ +for rollup, we strongly recommend not doing so since this PR may lead to changes in \ +compiler perf.{next_steps} + +@bors rollup=never +@rustbot label: +S-waiting-on-review -S-waiting-on-perf {label}", + ) +} + +fn generate_short_summary(summary: &ArtifactComparisonSummary) -> String { + // Add an "s" to a word unless there's only one. + fn ending(word: &'static str, count: usize) -> std::borrow::Cow<'static, str> { + if count == 1 { + return word.into(); + } + format!("{}s", word).into() + } + + let num_improvements = summary.number_of_improvements(); + let num_regressions = summary.number_of_regressions(); + + match summary.direction() { + Some(Direction::Improvement) => format!( + "🎉 relevant {} found", + ending("improvement", num_improvements) + ), + Some(Direction::Regression) => format!( + "😿 relevant {} found", + ending("regression", num_regressions) + ), + Some(Direction::Mixed) => "mixed results".to_string(), + None => "no relevant changes found".to_string(), + } +} diff --git a/site/src/request_handlers/github.rs b/site/src/request_handlers/github.rs index 3ae8cf1c7..e03a0e461 100644 --- a/site/src/request_handlers/github.rs +++ b/site/src/request_handlers/github.rs @@ -1,27 +1,22 @@ use crate::api::{github, ServerResult}; use crate::github::{ - branch_for_rollup, client, enqueue_sha, enqueue_unrolled_try_builds, get_authorized_users, - parse_homu_comment, pr_and_try_for_rollup, rollup_pr, + client, enqueue_sha, enqueue_unrolled_try_builds, parse_homu_comment, rollup_pr_number, }; use crate::load::SiteCtxt; use std::sync::Arc; -use regex::{Captures, Regex}; +use regex::Regex; lazy_static::lazy_static! { static ref ROLLUP_PR_NUMBER: Regex = Regex::new(r#"^Auto merge of #(\d+)"#).unwrap(); - static ref ROLLUPED_PR_NUMBER: Regex = + static ref ROLLEDUP_PR_NUMBER: Regex = Regex::new(r#"^Rollup merge of #(\d+)"#).unwrap(); static ref BODY_TRY_COMMIT: Regex = Regex::new(r#"(?:\W|^)@rust-timer\s+build\s+(\w+)(?:\W|$)(?:include=(\S+))?\s*(?:exclude=(\S+))?\s*(?:runs=(\d+))?"#).unwrap(); static ref BODY_QUEUE: Regex = Regex::new(r#"(?:\W|^)@rust-timer\s+queue(?:\W|$)(?:include=(\S+))?\s*(?:exclude=(\S+))?\s*(?:runs=(\d+))?"#).unwrap(); - static ref BODY_MAKE_PR_FOR: Regex = - Regex::new(r#"(?:\W|^)@rust-timer\s+make-pr-for\s+([\w:/\.\-]+)(?:\W|$)"#).unwrap(); - static ref BODY_UDPATE_PR_FOR: Regex = - Regex::new(r#"(?:\W|^)@rust-timer\s+update-branch-for\s+([\w:/\.\-]+)(?:\W|$)"#).unwrap(); } pub async fn handle_github( @@ -47,26 +42,44 @@ async fn handle_push(ctxt: Arc, push: github::Push) -> ServerResult pr, - None => return Ok(github::Response), - }; + let rollup_pr_number = + match rollup_pr_number(&main_repo_client, &push.head_commit.message).await? { + Some(pr) => pr, + None => return Ok(github::Response), + }; let previous_master = &push.before; - let rollup_merges = push - .commits + let commits = push.commits; + + handle_rollup_merge( + ci_client, + main_repo_client, + commits, + previous_master, + rollup_pr_number, + ) + .await?; + Ok(github::Response) +} + +/// Handler for when a rollup has been merged +async fn handle_rollup_merge( + ci_client: client::Client, + main_repo_client: client::Client, + commits: Vec, + previous_master: &str, + rollup_pr_number: u32, +) -> Result<(), String> { + let rollup_merges = commits .iter() .rev() .skip(1) // skip the head commit .take_while(|c| c.message.starts_with("Rollup merge of ")); - let mapping = enqueue_unrolled_try_builds(ci_client, rollup_merges, previous_master).await?; - let mapping = mapping .into_iter() .map(|(rollup_merge, sha)| { - ROLLUPED_PR_NUMBER + ROLLEDUP_PR_NUMBER .captures(&rollup_merge.message) .and_then(|c| c.get(1)) .map(|m| (m.as_str(), sha)) @@ -86,8 +99,8 @@ async fn handle_push(ctxt: Arc, push: github::Push) -> ServerResult impl Iterator + '_ { - BODY_MAKE_PR_FOR - .captures_iter(body) - .filter_map(|c| extract_rollup_merge(c)) -} - -fn extract_update_pr_for(body: &str) -> impl Iterator + '_ { - BODY_UDPATE_PR_FOR - .captures_iter(body) - .filter_map(|c| extract_rollup_merge(c)) -} - -fn extract_rollup_merge(capture: Captures) -> Option<&str> { - capture.get(1).map(|c| { - c.as_str() - .trim_start_matches("https://github.com/rust-lang/rust/commit/") - }) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn captures_the_right_sha() { - let message = r#"This is a message. - - @rust-timer make-pr-for https://github.com/rust-lang/rust/commit/857afc75e6ca69cc7dcae36a6fac8c093ee6fa31 - @rust-timer make-pr-for https://github.com/rust-lang/rust/commit/857afc75e6ca69cc7dcae36a6fac8c093ee6fa31 - "#; - - let mut iter = extract_make_pr_for(message); - assert_eq!( - iter.next().unwrap(), - "857afc75e6ca69cc7dcae36a6fac8c093ee6fa31", - "sha did not match" - ); - assert_eq!( - iter.next().unwrap(), - "857afc75e6ca69cc7dcae36a6fac8c093ee6fa31", - "sha did not match" - ); - assert!(iter.next().is_none(), "there were more rollup merges"); - let message = r#"This is a message. - - @rust-timer update-branch-for https://github.com/rust-lang/rust/commit/857afc75e6ca69cc7dcae36a6fac8c093ee6fa31"#; - - let mut iter = extract_update_pr_for(message); - let sha = iter.next().unwrap(); - println!("{sha}"); - assert_eq!( - sha, "857afc75e6ca69cc7dcae36a6fac8c093ee6fa31", - "sha did not match" - ); - assert!(iter.next().is_none(), "there were more rollup merges"); - } +pub async fn get_authorized_users() -> Result, String> { + let url = format!("{}/permissions/perf.json", ::rust_team_data::v1::BASE_URL); + let client = reqwest::Client::new(); + client + .get(&url) + .send() + .await + .map_err(|err| format!("failed to fetch authorized users: {}", err))? + .error_for_status() + .map_err(|err| format!("failed to fetch authorized users: {}", err))? + .json::() + .await + .map_err(|err| format!("failed to fetch authorized users: {}", err)) + .map(|perms| perms.github_ids) }