From f212c67945eafdf929eb4dd7113859ca1c0e2b57 Mon Sep 17 00:00:00 2001 From: Graham Christensen Date: Fri, 22 Mar 2019 15:24:45 -0400 Subject: [PATCH] Move output path based calculation --- ofborg/src/outpathdiff.rs | 6 +- ofborg/src/tagger.rs | 2 +- ofborg/src/tasks/eval/nixpkgs.rs | 237 ++++++++++++++++++++++++++++++- ofborg/src/tasks/evaluate.rs | 201 ++++---------------------- 4 files changed, 262 insertions(+), 184 deletions(-) diff --git a/ofborg/src/outpathdiff.rs b/ofborg/src/outpathdiff.rs index 8ec8703..dd0efa7 100644 --- a/ofborg/src/outpathdiff.rs +++ b/ofborg/src/outpathdiff.rs @@ -81,11 +81,11 @@ impl OutPathDiff { } } - pub fn calculate_rebuild(self) -> Option> { + pub fn calculate_rebuild(&self) -> Option> { let mut rebuild: Vec = vec![]; - if let Some(cur) = self.current { - if let Some(orig) = self.original { + if let Some(ref cur) = self.current { + if let Some(ref orig) = self.original { for key in cur.keys() { trace!("Checking out {:?}", key); if cur.get(key) != orig.get(key) { diff --git a/ofborg/src/tagger.rs b/ofborg/src/tagger.rs index 11535d1..cbb5b72 100644 --- a/ofborg/src/tagger.rs +++ b/ofborg/src/tagger.rs @@ -148,7 +148,7 @@ impl RebuildTagger { Default::default() } - pub fn parse_attrs(&mut self, attrs: Vec) { + pub fn parse_attrs(&mut self, attrs: &Vec) { let mut counter_darwin = 0; let mut counter_linux = 0; diff --git a/ofborg/src/tasks/eval/nixpkgs.rs b/ofborg/src/tasks/eval/nixpkgs.rs index 7e3b23c..6b88423 100644 --- a/ofborg/src/tasks/eval/nixpkgs.rs +++ b/ofborg/src/tasks/eval/nixpkgs.rs @@ -1,25 +1,60 @@ -use hubcaps::issues::IssueRef; +use crate::maintainers; +use crate::maintainers::ImpactedMaintainers; +use hubcaps::gists::Gists; +use hubcaps::issues::{Issue, IssueRef}; +use hubcaps::repositories::Repository; use ofborg::checkout::CachedProjectCo; use ofborg::commitstatus::CommitStatus; use ofborg::evalchecker::EvalChecker; +use ofborg::files::file_to_str; use ofborg::message::buildjob::BuildJob; +use ofborg::message::evaluationjob::EvaluationJob; use ofborg::nix::Nix; -use ofborg::tagger::StdenvTagger; -use ofborg::tasks::eval::{stdenvs::Stdenvs, EvaluationStrategy, StepResult}; +use ofborg::outpathdiff::{OutPathDiff, PackageArch}; +use ofborg::tagger::{MaintainerPRTagger, PathsTagger, RebuildTagger}; +use ofborg::tagger::{PkgsAddedRemovedTagger, StdenvTagger}; +use ofborg::tasks::eval::{stdenvs::Stdenvs, Error, EvaluationStrategy, StepResult}; use ofborg::tasks::evaluate::update_labels; +use std::collections::HashMap; use std::path::Path; +use tasks::evaluate::make_gist; pub struct NixpkgsStrategy<'a> { + job: &'a EvaluationJob, + pull: &'a hubcaps::pulls::PullRequest<'a>, + issue: &'a Issue, issue_ref: &'a IssueRef<'a>, + repo: &'a Repository<'a>, + gists: &'a Gists<'a>, nix: Nix, + tag_paths: &'a HashMap>, stdenv_diff: Option, + outpath_diff: Option, + changed_paths: Option>, } impl<'a> NixpkgsStrategy<'a> { - pub fn new(issue_ref: &'a IssueRef, nix: Nix) -> NixpkgsStrategy<'a> { + pub fn new( + job: &'a EvaluationJob, + pull: &'a hubcaps::pulls::PullRequest, + issue: &'a Issue, + issue_ref: &'a IssueRef, + repo: &'a Repository, + gists: &'a Gists, + nix: Nix, + tag_paths: &'a HashMap>, + ) -> NixpkgsStrategy<'a> { Self { + job, + pull, + issue, issue_ref, + repo, + gists, nix, + tag_paths, stdenv_diff: None, + outpath_diff: None, + changed_paths: None, } } @@ -38,6 +73,22 @@ impl<'a> NixpkgsStrategy<'a> { } } + fn tag_from_paths(&self) { + if let Some(ref changed_paths) = self.changed_paths { + let mut tagger = PathsTagger::new(self.tag_paths.clone()); + + for path in changed_paths { + tagger.path_changed(&path); + } + + update_labels( + &self.issue_ref, + &tagger.tags_to_add(), + &tagger.tags_to_remove(), + ); + } + } + fn check_stdenvs_before(&mut self, dir: &Path) { let mut stdenvs = Stdenvs::new(self.nix.clone(), dir.to_path_buf()); stdenvs.identify_before(); @@ -63,6 +114,139 @@ impl<'a> NixpkgsStrategy<'a> { ); } } + + fn check_outpaths_before(&mut self, dir: &Path) -> StepResult<()> { + let mut rebuildsniff = OutPathDiff::new(self.nix.clone(), dir.to_path_buf()); + + if let Err(mut output) = rebuildsniff.find_before() { + /* + self.events + .notify(Event::TargetBranchFailsEvaluation(target_branch.clone())); + */ + + Err(Error::FailWithGist( + String::from("The branch this PR will merge in to does not evaluate, and so this PR cannot be checked."), + String::from("Output path comparison"), + file_to_str(&mut output), + )) + } else { + self.outpath_diff = Some(rebuildsniff); + Ok(()) + } + } + + fn check_outpaths_after(&mut self) -> StepResult<()> { + if let Some(ref mut rebuildsniff) = self.outpath_diff { + if let Err(mut output) = rebuildsniff.find_after() { + Err(Error::FailWithGist( + String::from("This PR breaks listing of package outputs after merging."), + String::from("Output path comparison"), + file_to_str(&mut output), + )) + } else { + Ok(()) + } + } else { + Err(Error::Fail(String::from( + "Ofborg BUG: No outpath diff! Please report!", + ))) + } + } + + fn update_new_package_labels(&self) { + if let Some(ref rebuildsniff) = self.outpath_diff { + if let Some((removed, added)) = rebuildsniff.package_diff() { + let mut addremovetagger = PkgsAddedRemovedTagger::new(); + addremovetagger.changed(&removed, &added); + update_labels( + &self.issue_ref, + &addremovetagger.tags_to_add(), + &addremovetagger.tags_to_remove(), + ); + } + } + } + + fn update_rebuild_labels(&self, dir: &Path, overall_status: &mut CommitStatus) { + if let Some(ref rebuildsniff) = self.outpath_diff { + let mut rebuild_tags = RebuildTagger::new(); + + if let Some(attrs) = rebuildsniff.calculate_rebuild() { + if !attrs.is_empty() { + overall_status.set_url(self.gist_changed_paths(&attrs)); + self.record_impacted_maintainers(&dir, &attrs); + } + + rebuild_tags.parse_attrs(&attrs); + } + + update_labels( + &self.issue_ref, + &rebuild_tags.tags_to_add(), + &rebuild_tags.tags_to_remove(), + ); + } + } + + fn gist_changed_paths(&self, attrs: &Vec) -> Option { + make_gist( + &self.gists, + "Changed Paths", + Some("".to_owned()), + attrs + .iter() + .map(|attr| format!("{}\t{}", &attr.architecture, &attr.package)) + .collect::>() + .join("\n"), + ) + } + + fn record_impacted_maintainers(&self, dir: &Path, attrs: &Vec) -> () { + let changed_attributes = attrs + .iter() + .map(|attr| attr.package.split('.').collect::>()) + .collect::>>(); + + if let Some(ref changed_paths) = self.changed_paths { + let m = ImpactedMaintainers::calculate( + &self.nix, + &dir.to_path_buf(), + &changed_paths, + &changed_attributes, + ); + + let gist_url = make_gist( + &self.gists, + "Potential Maintainers", + Some("".to_owned()), + match m { + Ok(ref maintainers) => format!("Maintainers:\n{}", maintainers), + Err(ref e) => format!("Ignorable calculation error:\n{:?}", e), + }, + ); + + let mut status = CommitStatus::new( + self.repo.statuses(), + self.job.pr.head_sha.clone(), + String::from("grahamcofborg-eval-check-maintainers"), + String::from("matching changed paths to changed attrs..."), + gist_url, + ); + status.set(hubcaps::statuses::State::Success); + + if let Ok(ref maint) = m { + request_reviews(&maint, &self.pull); + let mut maint_tagger = MaintainerPRTagger::new(); + maint_tagger + .record_maintainer(&self.issue.user.login, &maint.maintainers_by_package()); + update_labels( + &self.issue_ref, + &maint_tagger.tags_to_add(), + &maint_tagger.tags_to_remove(), + ); + } + } + } } impl<'a> EvaluationStrategy for NixpkgsStrategy<'a> { @@ -78,10 +262,22 @@ impl<'a> EvaluationStrategy for NixpkgsStrategy<'a> { ); self.check_stdenvs_before(dir); + status.set_with_description( + "Checking original out paths", + hubcaps::statuses::State::Pending, + ); + self.check_outpaths_before(dir)?; + Ok(()) } - fn after_fetch(&mut self, _co: &CachedProjectCo) -> StepResult<()> { + fn after_fetch(&mut self, co: &CachedProjectCo) -> StepResult<()> { + let changed_paths = co + .files_changed_from_head(&self.job.pr.head_sha) + .unwrap_or_else(|_| vec![]); + self.changed_paths = Some(changed_paths); + self.tag_from_paths(); + Ok(()) } @@ -91,6 +287,9 @@ impl<'a> EvaluationStrategy for NixpkgsStrategy<'a> { status.set_with_description("Checking new stdenvs", hubcaps::statuses::State::Pending); self.check_stdenvs_after(); + status.set_with_description("Checking new out paths", hubcaps::statuses::State::Pending); + self.check_outpaths_after()?; + Ok(()) } @@ -100,11 +299,35 @@ impl<'a> EvaluationStrategy for NixpkgsStrategy<'a> { fn all_evaluations_passed( &mut self, - _co: &Path, - _status: &mut CommitStatus, + dir: &Path, + status: &mut CommitStatus, ) -> StepResult> { self.update_stdenv_labels(); + status.set_with_description( + "Calculating Changed Outputs", + hubcaps::statuses::State::Pending, + ); + + self.update_new_package_labels(); + self.update_rebuild_labels(&dir, status); + Ok(vec![]) } } + +fn request_reviews(maint: &maintainers::ImpactedMaintainers, pull: &hubcaps::pulls::PullRequest) { + if maint.maintainers().len() < 10 { + for maintainer in maint.maintainers() { + if let Err(e) = + pull.review_requests() + .create(&hubcaps::review_requests::ReviewRequestOptions { + reviewers: vec![maintainer.clone()], + team_reviewers: vec![], + }) + { + println!("Failure requesting a review from {}: {:#?}", maintainer, e,); + } + } + } +} diff --git a/ofborg/src/tasks/evaluate.rs b/ofborg/src/tasks/evaluate.rs index da2c151..60fb8bf 100644 --- a/ofborg/src/tasks/evaluate.rs +++ b/ofborg/src/tasks/evaluate.rs @@ -2,9 +2,6 @@ extern crate amqp; extern crate env_logger; extern crate uuid; - -use crate::maintainers; -use crate::maintainers::ImpactedMaintainers; use amqp::protocol::basic::{BasicProperties, Deliver}; use hubcaps; use hubcaps::gists::Gists; @@ -17,11 +14,10 @@ use ofborg::evalchecker::EvalChecker; use ofborg::files::file_to_str; use ofborg::message::{buildjob, evaluationjob}; use ofborg::nix; -use ofborg::outpathdiff::{OutPathDiff, OutPaths}; +use ofborg::outpathdiff::OutPaths; use ofborg::stats; use ofborg::stats::Event; use ofborg::systems; -use ofborg::tagger::{MaintainerPRTagger, PathsTagger, PkgsAddedRemovedTagger, RebuildTagger}; use ofborg::worker; use std::collections::HashMap; use std::path::Path; @@ -66,16 +62,6 @@ impl EvaluationWorker { evaluationjob::Actions {} } - fn tag_from_paths(&self, issue: &hubcaps::issues::IssueRef, paths: &[String]) { - let mut tagger = PathsTagger::new(self.tag_paths.clone()); - - for path in paths { - tagger.path_changed(&path); - } - - update_labels(&issue, &tagger.tags_to_add(), &tagger.tags_to_remove()); - } - fn handle_strategy_err( &self, ret: StepResult<()>, @@ -135,12 +121,6 @@ impl worker::SimpleWorker for EvaluationWorker let issue: Issue; let auto_schedule_build_archs: Vec; - let mut evaluation_strategy: Box = if job.is_nixpkgs() { - Box::new(eval::NixpkgsStrategy::new(&issue_ref, self.nix.clone())) - } else { - Box::new(eval::GenericStrategy::new()) - }; - match issue_ref.get() { Ok(iss) => { if iss.state == "closed" { @@ -169,6 +149,21 @@ impl worker::SimpleWorker for EvaluationWorker } }; + let mut evaluation_strategy: Box = if job.is_nixpkgs() { + Box::new(eval::NixpkgsStrategy::new( + &job, + &pull, + &issue, + &issue_ref, + &repo, + &gists, + self.nix.clone(), + &self.tag_paths, + )) + } else { + Box::new(eval::GenericStrategy::new()) + }; + let mut overall_status = CommitStatus::new( repo.statuses(), job.pr.head_sha.clone(), @@ -220,32 +215,8 @@ impl worker::SimpleWorker for EvaluationWorker return self.actions().skip(&job); } - let mut rebuildsniff = OutPathDiff::new(self.nix.clone(), PathBuf::from(&refpath)); - - overall_status.set_with_description( - "Checking original out paths", - hubcaps::statuses::State::Pending, - ); - let target_branch_rebuild_sniff_start = Instant::now(); - if let Err(mut output) = rebuildsniff.find_before() { - overall_status.set_url(make_gist( - &gists, - "Output path comparison", - Some("".to_owned()), - file_to_str(&mut output), - )); - - self.events - .notify(Event::TargetBranchFailsEvaluation(target_branch.clone())); - overall_status.set_with_description( - format!("Target branch {} doesn't evaluate!", &target_branch).as_ref(), - hubcaps::statuses::State::Failure, - ); - - return self.actions().skip(&job); - } self.events.notify(Event::EvaluationDuration( target_branch.clone(), target_branch_rebuild_sniff_start.elapsed().as_secs(), @@ -265,16 +236,22 @@ impl worker::SimpleWorker for EvaluationWorker return self.actions().skip(&job); } + if self + .handle_strategy_err( + evaluation_strategy.after_fetch(&co), + &gists, + &mut overall_status, + ) + .is_err() + { + return self.actions().skip(&job); + } + let possibly_touched_packages = parse_commit_messages( &co.commit_messages_from_head(&job.pr.head_sha) .unwrap_or_else(|_| vec!["".to_owned()]), ); - let changed_paths = co - .files_changed_from_head(&job.pr.head_sha) - .unwrap_or_else(|_| vec![]); - self.tag_from_paths(&issue_ref, &changed_paths); - overall_status.set_with_description("Merging PR", hubcaps::statuses::State::Pending); if co.merge_commit(job.pr.head_sha.as_ref()).is_err() { @@ -301,27 +278,6 @@ impl worker::SimpleWorker for EvaluationWorker return self.actions().skip(&job); } - overall_status - .set_with_description("Checking new out paths", hubcaps::statuses::State::Pending); - - if let Err(mut output) = rebuildsniff.find_after() { - overall_status.set_url(make_gist( - &gists, - "Output path comparison", - Some("".to_owned()), - file_to_str(&mut output), - )); - overall_status.set_with_description( - format!( - "Failed to enumerate outputs after merging to {}", - &target_branch - ) - .as_ref(), - hubcaps::statuses::State::Failure, - ); - return self.actions().skip(&job); - } - println!("Got path: {:?}, building", refpath); overall_status .set_with_description("Beginning Evaluations", hubcaps::statuses::State::Pending); @@ -532,91 +488,6 @@ impl worker::SimpleWorker for EvaluationWorker } if eval_results { - overall_status.set_with_description( - "Calculating Changed Outputs", - hubcaps::statuses::State::Pending, - ); - - if let Some((removed, added)) = rebuildsniff.package_diff() { - let mut addremovetagger = PkgsAddedRemovedTagger::new(); - addremovetagger.changed(&removed, &added); - update_labels( - &issue_ref, - &addremovetagger.tags_to_add(), - &addremovetagger.tags_to_remove(), - ); - } - - let mut rebuild_tags = RebuildTagger::new(); - if let Some(attrs) = rebuildsniff.calculate_rebuild() { - if !attrs.is_empty() { - let gist_url = make_gist( - &gists, - "Changed Paths", - Some("".to_owned()), - attrs - .iter() - .map(|attr| format!("{}\t{}", &attr.architecture, &attr.package)) - .collect::>() - .join("\n"), - ); - - overall_status.set_url(gist_url); - - let changed_attributes = attrs - .iter() - .map(|attr| attr.package.split('.').collect::>()) - .collect::>>(); - - let m = ImpactedMaintainers::calculate( - &self.nix, - &PathBuf::from(&refpath), - &changed_paths, - &changed_attributes, - ); - - let gist_url = make_gist( - &gists, - "Potential Maintainers", - Some("".to_owned()), - match m { - Ok(ref maintainers) => format!("Maintainers:\n{}", maintainers), - Err(ref e) => format!("Ignorable calculation error:\n{:?}", e), - }, - ); - - if let Ok(ref maint) = m { - request_reviews(&maint, &pull); - let mut maint_tagger = MaintainerPRTagger::new(); - maint_tagger - .record_maintainer(&issue.user.login, &maint.maintainers_by_package()); - update_labels( - &issue_ref, - &maint_tagger.tags_to_add(), - &maint_tagger.tags_to_remove(), - ); - } - - let mut status = CommitStatus::new( - repo.statuses(), - job.pr.head_sha.clone(), - String::from("grahamcofborg-eval-check-maintainers"), - String::from("matching changed paths to changed attrs..."), - gist_url, - ); - - status.set(hubcaps::statuses::State::Success); - } - - rebuild_tags.parse_attrs(attrs); - } - - update_labels( - &issue_ref, - &rebuild_tags.tags_to_add(), - &rebuild_tags.tags_to_remove(), - ); - { let ret = evaluation_strategy .all_evaluations_passed(&Path::new(&refpath), &mut overall_status); @@ -649,7 +520,7 @@ impl worker::SimpleWorker for EvaluationWorker } } -fn make_gist<'a>( +pub fn make_gist<'a>( gists: &hubcaps::gists::Gists<'a>, name: &str, description: Option, @@ -796,19 +667,3 @@ fn indicates_wip(text: &str) -> bool { false } - -fn request_reviews(maint: &maintainers::ImpactedMaintainers, pull: &hubcaps::pulls::PullRequest) { - if maint.maintainers().len() < 10 { - for maintainer in maint.maintainers() { - if let Err(e) = - pull.review_requests() - .create(&hubcaps::review_requests::ReviewRequestOptions { - reviewers: vec![maintainer.clone()], - team_reviewers: vec![], - }) - { - println!("Failure requesting a review from {}: {:#?}", maintainer, e,); - } - } - } -}