From 2bc3dd5f7671d57c59528bfeaadd7ae417c9943a Mon Sep 17 00:00:00 2001 From: Jake Bailey Date: Tue, 5 Mar 2024 11:02:49 -0800 Subject: [PATCH] Track the reason why an immediate change was detected Summary: Add an `ImpactReason` enum indicating the path we took in `immediate_target_changes` to mark a target as affected, then propagate that reason through rdeps in `recursive_target_changes`. Carry the reason all the way through to BTD's output so that we can inspect it in Skycastle. Also summarize the number of affected targets with each reason in the BTD_SUCCESS event. Reviewed By: ndmitchell Differential Revision: D54436861 fbshipit-source-id: 800607b2796cd94f4f6965c0555e143cb468c9a2 --- btd/Cargo.toml | 1 + btd/src/check.rs | 15 ++-- btd/src/diff.rs | 162 ++++++++++++++++++++++++++++++------------ btd/src/glean.rs | 16 +++-- btd/src/graph_size.rs | 11 +-- btd/src/lib.rs | 18 +++-- btd/src/output.rs | 24 +++++-- btd/test/test_e2e.py | 3 + 8 files changed, 178 insertions(+), 72 deletions(-) diff --git a/btd/Cargo.toml b/btd/Cargo.toml index a331d4d..4d89382 100644 --- a/btd/Cargo.toml +++ b/btd/Cargo.toml @@ -15,6 +15,7 @@ derive_more = "0.99.3" fbinit = { workspace = true } glob = "0.3.0" itertools = "0.10.5" +parse-display = "0.8.2" serde = {version = "1.0", features = ["derive"]} serde_json = "1.0.66" tempfile = "3.1.0" diff --git a/btd/src/check.rs b/btd/src/check.rs index 026311c..ddfede8 100644 --- a/btd/src/check.rs +++ b/btd/src/check.rs @@ -23,6 +23,7 @@ use crate::buck::types::Package; use crate::buck::types::TargetLabel; use crate::buck::types::TargetPattern; use crate::changes::Changes; +use crate::diff::ImpactReason; #[derive(Debug, Error, Serialize)] pub enum ValidationError { @@ -141,7 +142,7 @@ pub fn check_errors(base: &Targets, diff: &Targets, changes: &Changes) -> Vec Vec { let exists_after = diff.targets_by_label_key(); @@ -149,7 +150,7 @@ pub fn check_dangling( let mut errors = Vec::new(); // Lets check if dangling edges were introduced. - for target in immediate_changes.iter() { + for (target, _) in immediate_changes.iter() { for dep in target.deps.iter() { let key = dep.key(); // Only check newly introduced dangling dependencies that are @@ -357,7 +358,7 @@ mod tests { target_entry("aaa", &[]), target_entry("bbb", &["aaa", "ccc"]) ]), - &[&modified_target], + &[(&modified_target, ImpactReason::Inputs)], &[TargetPattern::new("foo//...")], ) .len(), @@ -376,7 +377,7 @@ mod tests { target_entry("bbb", &["aaa"]), target_entry("ccc", &["ddd"]) ]), - &[&modified_target], + &[(&modified_target, ImpactReason::Inputs)], &[TargetPattern::new("foo//...")], ) .len(), @@ -395,7 +396,7 @@ mod tests { target_entry("aaa", &["ccc"]), target_entry("bbb", &["aaa"]) ]), - &[&modified_target], + &[(&modified_target, ImpactReason::Inputs)], &[TargetPattern::new("foo//...")], ) .len(), @@ -413,7 +414,7 @@ mod tests { target_entry("aaa", &["ccc"]), target_entry("bbb", &["aaa"]) ]), - &[&modified_target], + &[(&modified_target, ImpactReason::Inputs)], &[TargetPattern::new("foo//...")], ) .len(), @@ -431,7 +432,7 @@ mod tests { target_entry("aaa", &[]), target_entry("bbb", &["aaa"]) ]), - &[&modified_target], + &[(&modified_target, ImpactReason::Inputs)], &[TargetPattern::new("foo//...")], ) .len(), diff --git a/btd/src/diff.rs b/btd/src/diff.rs index b054023..b444b1e 100644 --- a/btd/src/diff.rs +++ b/btd/src/diff.rs @@ -86,10 +86,10 @@ fn is_changed_ci_srcs(file_deps: &[Glob], changes: &Changes) -> bool { pub struct GraphImpact<'a> { /// Targets which changed, and whose change is expected to impact /// things that depend on them (they changed recursively). - recursive: Vec<&'a BuckTarget>, + recursive: Vec<(&'a BuckTarget, ImpactReason)>, /// Targets which changed in a way that won't impact things recursively. /// Currently only package value changes. - non_recursive: Vec<&'a BuckTarget>, + non_recursive: Vec<(&'a BuckTarget, ImpactReason)>, } impl<'a> GraphImpact<'a> { @@ -97,7 +97,7 @@ impl<'a> GraphImpact<'a> { self.recursive.len() + self.non_recursive.len() } - pub fn iter(&'a self) -> impl Iterator { + pub fn iter(&'a self) -> impl Iterator { self.recursive .iter() .chain(self.non_recursive.iter()) @@ -105,6 +105,29 @@ impl<'a> GraphImpact<'a> { } } +/// Categorization of the kind of immediate target change which caused BTD to +/// report a target as impacted. These reasons are propagated down through +/// rdeps, so they indicate that a target *or one of its dependencies* changed +/// in the indicated way. +#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd)] +#[derive(serde::Serialize, serde::Deserialize, parse_display::Display)] +#[serde(rename_all = "snake_case")] +#[display(style = "snake_case")] +pub enum ImpactReason { + /// This target was impacted because a target's package changed. + Package, + /// The hash of a target changed. + Hash, + /// The sources a target points at changed. + Inputs, + /// The `ci_srcs` of a target (used as additional triggers) changed. + CiSrcs, + /// The Buck rule used to define a target changed. + Rule, + /// The `buck.package_values` of a target changed. + PackageValues, +} + pub fn immediate_target_changes<'a>( base: &Targets, diff: &'a Targets, @@ -117,40 +140,75 @@ pub fn immediate_target_changes<'a>( // Find those .bzl files that have changed, including transitive changes let bzl_change = changed_bzl_files(diff, changes, track_prelude_changes); + // Track the reason we determined a target to have changed + let some_if = |reason, changed| if changed { Some(reason) } else { None }; + let mut res = GraphImpact::default(); for target in diff.targets() { // "hidden feature" that allows using btd to find rdeps of a "package" (directory) // by including directory paths in the changes input - let change_package: bool = changes.contains_package(&target.package); + let change_package = some_if( + ImpactReason::Package, + changes.contains_package(&target.package), + ); let old_target = old.get(&target.label_key()); // Did the hash of the target change - let change_hash = || match old_target { - None => true, - Some(x) => x.hash != target.hash, + let change_hash = || { + some_if( + ImpactReason::Hash, + match old_target { + None => true, + Some(x) => x.hash != target.hash, + }, + ) }; // Did the package values change - let change_package_values = || match old_target { - None => true, - Some(x) => x.package_values != target.package_values, + let change_package_values = || { + some_if( + ImpactReason::PackageValues, + match old_target { + None => true, + Some(x) => x.package_values != target.package_values, + }, + ) }; // Did any of the sources we point at change - let change_inputs = || target.inputs.iter().any(|x| changes.contains_cell_path(x)); - let change_ci_srcs = || is_changed_ci_srcs(&target.ci_srcs, changes); + let change_inputs = || { + some_if( + ImpactReason::Inputs, + target.inputs.iter().any(|x| changes.contains_cell_path(x)), + ) + }; + let change_ci_srcs = || { + some_if( + ImpactReason::CiSrcs, + is_changed_ci_srcs(&target.ci_srcs, changes), + ) + }; // Did the rule we point at change - let change_rule = - || !bzl_change.is_empty() && bzl_change.contains(&target.rule_type.file()); + let change_rule = || { + some_if( + ImpactReason::Rule, + !bzl_change.is_empty() && bzl_change.contains(&target.rule_type.file()), + ) + }; - if change_package || change_hash() || change_inputs() || change_ci_srcs() || change_rule() { - res.recursive.push(target) - } else if change_package_values() { - res.non_recursive.push(target); + if let Some(reason) = change_package + .or_else(change_hash) + .or_else(change_inputs) + .or_else(change_ci_srcs) + .or_else(change_rule) + { + res.recursive.push((target, reason)); + } else if let Some(reason) = change_package_values() { + res.non_recursive.push((target, reason)); } } // Sort to ensure deterministic output - res.recursive.sort_by_key(|t| t.label_key()); - res.non_recursive.sort_by_key(|t| t.label_key()); + res.recursive.sort_by_key(|(t, _)| t.label_key()); + res.non_recursive.sort_by_key(|(t, _)| t.label_key()); res } @@ -168,7 +226,7 @@ pub fn recursive_target_changes<'a>( changes: &GraphImpact<'a>, depth: Option, follow_rule_type: impl Fn(&RuleType) -> bool, -) -> Vec> { +) -> Vec> { // Just an optimisation, but saves building the reverse mapping if changes.recursive.is_empty() { let mut res = if changes.non_recursive.is_empty() { @@ -238,8 +296,13 @@ pub fn recursive_target_changes<'a>( let mut done: HashMap = changes .recursive .iter() - .map(|x| (x.label_key(), true)) - .chain(changes.non_recursive.iter().map(|x| (x.label_key(), false))) + .map(|(x, _)| (x.label_key(), true)) + .chain( + changes + .non_recursive + .iter() + .map(|(x, _)| (x.label_key(), false)), + ) .collect(); let mut result = Vec::new(); @@ -247,9 +310,12 @@ pub fn recursive_target_changes<'a>( let mut todo_silent = Vec::new(); let mut next_silent = Vec::new(); - fn add_result<'a>(results: &mut Vec>, mut items: Vec<&'a BuckTarget>) { + fn add_result<'a>( + results: &mut Vec>, + mut items: Vec<(&'a BuckTarget, ImpactReason)>, + ) { // Sort to ensure deterministic output - items.sort_by_key(|x| x.label_key()); + items.sort_by_key(|(x, _)| x.label_key()); results.push(items); } @@ -263,17 +329,17 @@ pub fn recursive_target_changes<'a>( let mut next = Vec::new(); - for lbl in todo.iter().chain(todo_silent.iter()) { + for (lbl, reason) in todo.iter().chain(todo_silent.iter()) { if follow_rule_type(&lbl.rule_type) { for rdep in rdeps.get(&lbl.label()) { match done.entry(rdep.label_key()) { Entry::Vacant(e) => { - next.push(*rdep); + next.push((*rdep, *reason)); e.insert(true); } Entry::Occupied(mut e) => { if !e.get() { - next_silent.push(*rdep); + next_silent.push((*rdep, *reason)); e.insert(true); } } @@ -395,8 +461,8 @@ mod tests { ]), false, ); - let recursive = res.recursive.map(|x| x.label().to_string()); - let non_recursive = res.non_recursive.map(|x| x.label().to_string()); + let recursive = res.recursive.map(|(x, _)| x.label().to_string()); + let non_recursive = res.non_recursive.map(|(x, _)| x.label().to_string()); assert_eq!( recursive.map(|x| x.as_str()), &[ @@ -435,7 +501,7 @@ mod tests { &Changes::testing(&[Status::Modified(package)]), false, ); - let mut res = res.recursive.map(|x| x.label().to_string()); + let mut res = res.recursive.map(|(x, _)| x.label().to_string()); res.sort(); let res = res.map(|x| x.as_str()); assert_eq!(&res, &["foo//bar:aaa", "foo//bar:bbb",]); @@ -459,11 +525,11 @@ mod tests { let changes = GraphImpact { recursive: Vec::new(), - non_recursive: vec![diff.targets().next().unwrap()], + non_recursive: vec![(diff.targets().next().unwrap(), ImpactReason::Inputs)], }; let res = recursive_target_changes(&diff, &changes, Some(2), |_| true); let res = res.map(|xs| { - let mut xs = xs.map(|x| x.name.as_str()); + let mut xs = xs.map(|(x, _)| x.name.as_str()); xs.sort(); xs }); @@ -500,12 +566,12 @@ mod tests { ]); let changes = GraphImpact { - recursive: vec![diff.targets().next().unwrap()], - non_recursive: vec![diff.targets().nth(1).unwrap()], + recursive: vec![(diff.targets().next().unwrap(), ImpactReason::Inputs)], + non_recursive: vec![(diff.targets().nth(1).unwrap(), ImpactReason::Inputs)], }; let res = recursive_target_changes(&diff, &changes, Some(2), |_| true); let res = res.map(|xs| { - let mut xs = xs.map(|x| x.name.as_str()); + let mut xs = xs.map(|(x, _)| x.name.as_str()); xs.sort(); xs }); @@ -536,12 +602,12 @@ mod tests { ]); let changes = GraphImpact { - recursive: vec![diff.targets().next().unwrap()], + recursive: vec![(diff.targets().next().unwrap(), ImpactReason::Inputs)], non_recursive: Vec::new(), }; let res = recursive_target_changes(&diff, &changes, Some(3), |_| true); let res = res.map(|xs| { - let mut xs = xs.map(|x| x.name.as_str()); + let mut xs = xs.map(|(x, _)| x.name.as_str()); xs.sort(); xs }); @@ -573,12 +639,12 @@ mod tests { let change_target = BuckTarget::testing("dep", "code//foo", "prelude//rules.bzl:cxx_library"); let changes = GraphImpact { - recursive: vec![&change_target], + recursive: vec![(&change_target, ImpactReason::Inputs)], non_recursive: Vec::new(), }; let res = recursive_target_changes(&diff, &changes, Some(1), |_| true); let res = res.map(|xs| { - let mut xs = xs.map(|x| x.name.as_str()); + let mut xs = xs.map(|(x, _)| x.name.as_str()); xs.sort(); xs }); @@ -603,11 +669,15 @@ mod tests { ]); let changes = GraphImpact { - recursive: diff.targets().take(2).collect(), + recursive: diff + .targets() + .take(2) + .map(|x| (x, ImpactReason::Inputs)) + .collect(), non_recursive: Vec::new(), }; let res = recursive_target_changes(&diff, &changes, None, |_| true); - let res = res.map(|xs| xs.map(|x| x.name.as_str())); + let res = res.map(|xs| xs.map(|(x, _)| x.name.as_str())); assert_eq!(res, vec![vec!["a", "b"], vec!["c", "d"], vec![]]); } @@ -785,7 +855,9 @@ mod tests { .count(), 2 ); - impact.recursive.push(targets.targets().nth(1).unwrap()); + impact + .recursive + .push((targets.targets().nth(1).unwrap(), ImpactReason::Inputs)); assert_eq!( recursive_target_changes(&targets, &impact, None, |_| true) .iter() @@ -812,13 +884,13 @@ mod tests { ]); let changes = GraphImpact { - recursive: vec![diff.targets().next().unwrap()], + recursive: vec![(diff.targets().next().unwrap(), ImpactReason::Inputs)], non_recursive: Vec::new(), }; let res = recursive_target_changes(&diff, &changes, Some(3), |_| true); assert_eq!(res[0].len(), 1); assert_eq!(res[1].len(), 1); - assert_eq!(res[1][0].name, TargetName::new("baz")); + assert_eq!(res[1][0].0.name, TargetName::new("baz")); assert_eq!(res.iter().flatten().count(), 2); } } diff --git a/btd/src/glean.rs b/btd/src/glean.rs index d38bc97..1fa9b9f 100644 --- a/btd/src/glean.rs +++ b/btd/src/glean.rs @@ -18,6 +18,7 @@ use crate::buck::types::TargetLabelKeyRef; use crate::changes::Changes; use crate::diff::immediate_target_changes; use crate::diff::recursive_target_changes; +use crate::diff::ImpactReason; fn cxx_rule_type(typ: &RuleType) -> bool { let short = typ.short(); @@ -34,7 +35,7 @@ pub fn glean_changes<'a>( diff: &'a Targets, changes: &Changes, depth: Option, -) -> Vec> { +) -> Vec> { let header = immediate_target_changes( base, diff, @@ -47,20 +48,23 @@ pub fn glean_changes<'a>( merge(header_rec, other_rec) } -fn merge<'a>(a: Vec>, b: Vec>) -> Vec> { +fn merge<'a>( + a: Vec>, + b: Vec>, +) -> Vec> { let mut seen: HashSet = HashSet::new(); let mut res = Vec::new(); for layer in a.into_iter().zip_longest(b) { let mut res1 = Vec::new(); let (left, right) = layer.or_default(); - for item in left.into_iter().chain(right) { + for (item, reason) in left.into_iter().chain(right) { if seen.insert(item.label_key()) && cxx_rule_type(&item.rule_type) { - res1.push(item) + res1.push((item, reason)) } } if !res1.is_empty() { - res1.sort_by_key(|x| x.label_key()); + res1.sort_by_key(|(x, _)| x.label_key()); res.push(res1) } } @@ -122,7 +126,7 @@ mod tests { ]), None, ); - let mut res = res.concat().map(|x| x.label()); + let mut res = res.concat().map(|(x, _)| x.label()); res.sort(); let want = vec![ TargetLabel::new("root//:bin1"), diff --git a/btd/src/graph_size.rs b/btd/src/graph_size.rs index 88a5ebc..d8e8d22 100644 --- a/btd/src/graph_size.rs +++ b/btd/src/graph_size.rs @@ -20,6 +20,7 @@ use crate::buck::targets::BuckTarget; use crate::buck::targets::Targets; use crate::buck::types::TargetLabel; use crate::buck::types::TargetLabelKeyRef; +use crate::diff::ImpactReason; use crate::output::Output; use crate::output::OutputFormat; @@ -79,7 +80,7 @@ impl GraphSize { pub fn print_recursive_changes( &mut self, - changes: &[Vec<&BuckTarget>], + changes: &[Vec<(&BuckTarget, ImpactReason)>], sudos: &HashSet, output: OutputFormat, ) { @@ -88,12 +89,12 @@ impl GraphSize { .enumerate() .flat_map(|(depth, xs)| { xs.iter() - .map(move |x| (depth, *x, sudos.contains(&x.label_key()))) + .map(move |&(x, r)| (depth, x, sudos.contains(&x.label_key()), r)) }) .collect::>() - .par_iter() - .map(|(depth, x, uses_sudo)| OutputWithSize { - output: Output::from_target(x, *depth as u64, *uses_sudo), + .into_par_iter() + .map(|(depth, x, uses_sudo, reason)| OutputWithSize { + output: Output::from_target(x, depth as u64, uses_sudo, reason), before_size: self.base.get(&x.label()), after_size: self.diff.get(&x.label()), }) diff --git a/btd/src/lib.rs b/btd/src/lib.rs index d1457ea..c9e3fe2 100644 --- a/btd/src/lib.rs +++ b/btd/src/lib.rs @@ -26,6 +26,7 @@ pub mod rerun; pub mod sapling; pub mod sudo; +use std::collections::BTreeMap; use std::collections::HashSet; use std::fs::File; use std::io::stdout; @@ -54,6 +55,7 @@ use crate::buck::types::TargetLabelKeyRef; use crate::buck::types::TargetPattern; use crate::changes::Changes; use crate::check::ValidationError; +use crate::diff::ImpactReason; use crate::graph_size::GraphSize; use crate::output::Output; use crate::output::OutputFormat; @@ -265,12 +267,18 @@ pub fn main(args: Args) -> anyhow::Result<()> { step(&format!( "finish with {immediate_changes} immediate changes, {total_changes} total changes" )); + // BTreeMap so that reasons are consistently ordered in logs + let mut reason_counts: BTreeMap = BTreeMap::new(); + for &(_, reason) in recursive.iter().flatten() { + *reason_counts.entry(reason).or_default() += 1; + } td_util::scuba!( event: BTD_SUCCESS, duration_ms: t.elapsed().as_millis(), data: json!({ "immediate_changes": immediate_changes, "total_changes": total_changes, + "reason_counts": reason_counts, }) ); Ok(()) @@ -382,7 +390,7 @@ impl OutputFormat { } fn print_recursive_changes<'a, T: Serialize + 'a>( - changes: &[Vec<&'a BuckTarget>], + changes: &[Vec<(&'a BuckTarget, ImpactReason)>], sudos: &HashSet, output: OutputFormat, mut augment: impl FnMut(&'a BuckTarget, Output<'a>) -> T, @@ -390,7 +398,7 @@ fn print_recursive_changes<'a, T: Serialize + 'a>( if output == OutputFormat::Text { for (depth, xs) in changes.iter().enumerate() { println!("Level {}", depth); - for x in xs { + for (x, _) in xs { println!(" {}", x.label()); } } @@ -400,10 +408,10 @@ fn print_recursive_changes<'a, T: Serialize + 'a>( .enumerate() .flat_map(|(depth, xs)| { xs.iter() - .map(move |x| (depth, *x, sudos.contains(&x.label_key()))) + .map(move |&(x, r)| (depth, x, sudos.contains(&x.label_key()), r)) }) - .map(|(depth, x, uses_sudo)| { - augment(x, Output::from_target(x, depth as u64, uses_sudo)) + .map(|(depth, x, uses_sudo, reason)| { + augment(x, Output::from_target(x, depth as u64, uses_sudo, reason)) }); let out = stdout().lock(); diff --git a/btd/src/output.rs b/btd/src/output.rs index a8a37be..af777c3 100644 --- a/btd/src/output.rs +++ b/btd/src/output.rs @@ -16,6 +16,7 @@ use crate::buck::labels::Labels; use crate::buck::targets::BuckTarget; use crate::buck::types::Oncall; use crate::buck::types::TargetLabel; +use crate::diff::ImpactReason; #[derive(Debug, Serialize)] pub struct Output<'a> { @@ -25,10 +26,16 @@ pub struct Output<'a> { oncall: &'a Option, depth: u64, labels: Labels, + reason: ImpactReason, } impl<'a> Output<'a> { - pub fn from_target(x: &'a BuckTarget, depth: u64, uses_sudo: bool) -> Self { + pub fn from_target( + x: &'a BuckTarget, + depth: u64, + uses_sudo: bool, + reason: ImpactReason, + ) -> Self { let additional_labels = if uses_sudo && !x.labels.contains("uses_sudo") { Labels::new(&["uses_sudo"]) } else { @@ -44,6 +51,7 @@ impl<'a> Output<'a> { .package_values .labels .merge3(&x.labels, &additional_labels), + reason, } } } @@ -80,6 +88,7 @@ mod tests { "depth": 3, "labels": ["my_label", "another_label"], "oncall": "my_team", + "reason": "inputs", } ); @@ -94,7 +103,7 @@ mod tests { oncall: Some(Oncall::new("my_team")), ..BuckTarget::testing("test", "fbcode//me", "prelude//rules.bzl:python_library") }; - let output = Output::from_target(&target, 3, false); + let output = Output::from_target(&target, 3, false, ImpactReason::Inputs); assert_eq!(serde_json::to_value(&output).unwrap(), json); assert_eq!( serde_json::from_str::(&output.to_string()).unwrap(), @@ -113,10 +122,17 @@ mod tests { "depth": 3, "labels": ["my_label", "another_label"], "oncall": Value::Null, + "reason": ImpactReason::Inputs, } ); assert_eq!( - serde_json::to_value(Output::from_target(&target_no_oncall, 3, false)).unwrap(), + serde_json::to_value(Output::from_target( + &target_no_oncall, + 3, + false, + ImpactReason::Inputs + )) + .unwrap(), json_no_oncall ); } @@ -128,7 +144,7 @@ mod tests { package_values: PackageValues::new(&["must-come-first"], serde_json::Value::Null), ..BuckTarget::testing("test", "fbcode//me", "prelude//rules.bzl:python_library") }; - let output = Output::from_target(&target, 3, false); + let output = Output::from_target(&target, 3, false, ImpactReason::Inputs); assert_eq!( output.labels, Labels::new(&["must-come-first", "target_label"]) diff --git a/btd/test/test_e2e.py b/btd/test/test_e2e.py index accede6..e219ef3 100644 --- a/btd/test/test_e2e.py +++ b/btd/test/test_e2e.py @@ -189,6 +189,7 @@ def check_properties(patch, rdeps): "target": "root//inner:baz", "type": "my_rule", "oncall": None, + "reason": "inputs", } in rdeps assert len(rdeps) == 2 elif patch == "rename_inner": @@ -198,6 +199,7 @@ def check_properties(patch, rdeps): "target": "root//inner:baz", "type": "my_rule", "oncall": None, + "reason": "hash", } in rdeps assert len(rdeps) == 2 elif patch == "buckconfig": @@ -210,6 +212,7 @@ def check_properties(patch, rdeps): "target": "root//inner:baz", "type": "my_rule", "oncall": None, + "reason": "package_values", } ] else: