Skip to content

Commit

Permalink
btd: Tag terminal targets in the rdeps traversal
Browse files Browse the repository at this point in the history
Summary: Mark the terminal targets in the rdeps traversal.

Reviewed By: rjbailey

Differential Revision: D60877321

fbshipit-source-id: e60558f23361a73b895014db8c456f1464f99bea
  • Loading branch information
Aniket Mathur authored and facebook-github-bot committed Aug 7, 2024
1 parent 270ae15 commit 3e022b9
Show file tree
Hide file tree
Showing 5 changed files with 102 additions and 3 deletions.
12 changes: 12 additions & 0 deletions btd/src/buck/target_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,18 @@ impl<T> TargetMap<T> {
.chain(non_recursive_patterns)
.chain(recursive_patterns)
}

pub fn is_terminal_node(&self, key: &TargetLabel) -> bool {
// If the corresponding entry for the given TargetLabel doesn't
// contain any values, where each value is an edge between two nodes,
// then we can conclude that we have a terminal node.
// In a normal dependency graph, this would be the leaf targets.
// In a reverse dependency graph, this would be the root targets.
match self.literal.get(key) {
Some(values) => values.is_empty(),
None => true,
}
}
}

#[cfg(test)]
Expand Down
73 changes: 73 additions & 0 deletions btd/src/diff.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,8 @@ pub struct ImpactTraceData {
/// The target name of the dependency which actually changed,
/// and the type of change that we detected in it.
pub root_cause: (String, RootImpactKind), // root_target_name, reason
/// Whether the node is a root in the dependency graph.
pub is_terminal: bool,
}

impl ImpactTraceData {
Expand All @@ -150,6 +152,7 @@ impl ImpactTraceData {
format!("{}:{}", target.package.as_str(), target.name.as_str()),
kind,
),
is_terminal: false,
}
}

Expand All @@ -158,6 +161,7 @@ impl ImpactTraceData {
ImpactTraceData {
affected_dep: "cell//foo:bar".to_owned(),
root_cause: ("cell//baz:qux".to_owned(), RootImpactKind::Inputs),
is_terminal: false,
}
}
}
Expand Down Expand Up @@ -425,6 +429,7 @@ pub fn recursive_target_changes<'a>(
let updated_reason = ImpactTraceData {
affected_dep: lbl.label().to_string(),
root_cause: reason.root_cause.clone(),
is_terminal: false,
};
for rdep in rdeps.get(&lbl.label()) {
match done.entry(rdep.label_key()) {
Expand Down Expand Up @@ -458,9 +463,25 @@ pub fn recursive_target_changes<'a>(
// an empty todo list might be added to the result here, indicating to
// the user (in Text output mode) that there are no additional levels
add_result(&mut result, todo);
annotate_terminal_nodes(&mut result, &rdeps);
result
}

/// For all nodes that are affected, mark the ones which are terminal in the
/// target graph. We do not mark nodes cut off by depth as terminal.
fn annotate_terminal_nodes(
result: &mut [Vec<(&BuckTarget, ImpactTraceData)>],
rdeps: &TargetMap<&BuckTarget>,
) {
for level in result.iter_mut() {
for (target, trace) in level.iter_mut() {
if rdeps.is_terminal_node(&target.label()) {
trace.is_terminal = true;
}
}
}
}

#[cfg(test)]
mod tests {
use td_util::prelude::*;
Expand Down Expand Up @@ -1159,4 +1180,56 @@ mod tests {
assert_eq!(res[1][0].0.name, TargetName::new("baz"));
assert_eq!(res.iter().flatten().count(), 2);
}

#[test]
fn test_terminal_nodes() {
// Create A -> B -> C -> D -> E
// And then assert that C and E are terminal nodes.
fn target(name: &str, deps: &[&str]) -> TargetsEntry {
let pkg = Package::new("foo//");
TargetsEntry::Target(BuckTarget {
deps: deps.iter().map(|x| pkg.join(&TargetName::new(x))).collect(),
..BuckTarget::testing(name, pkg.as_str(), "prelude//rules.bzl:cxx_library")
})
}
let entries = vec![
target("a", &["b"]),
target("b", &["c", "d"]),
target("c", &["e", "f"]),
target("d", &["e"]),
target("e", &["g"]),
target("f", &[]),
target("g", &[]),
target("x", &["c"]),
target("y", &["d"]),
target("z", &["g"]),
];
let diff = Targets::new(entries);

let check = |changes: &GraphImpact, depth: usize, expected: &[&str]| {
let res = recursive_target_changes(&diff, changes, Some(depth), |_| true);
let mut terminal = res
.iter()
.flatten()
.filter(|(_, impact)| impact.is_terminal)
.map(|(t, _)| t.name.as_str())
.collect::<Vec<_>>();
terminal.sort();
assert_eq!(&terminal, expected);
};

let changes = GraphImpact::from_recursive(vec![(
diff.targets().find(|t| t.name.as_str() == "g").unwrap(),
ImpactTraceData::sample(),
)]);
check(&changes, 5, &["a", "x", "y", "z"]);
check(&changes, 1, &["z"]);

let changes = GraphImpact::from_recursive(vec![(
diff.targets().find(|t| t.name.as_str() == "c").unwrap(),
ImpactTraceData::sample(),
)]);
check(&changes, 5, &["a", "x"]);
check(&changes, 1, &["x"]);
}
}
1 change: 1 addition & 0 deletions btd/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,7 @@ pub fn main(args: Args) -> anyhow::Result<()> {
"input_parse_errors": base.errors().count(),
"diff_targets": diff.targets().count(),
"diff_parse_errors": diff.errors().count(),
"terminal_node_changes": recursive.iter().flatten().filter(|(_, r)| r.is_terminal).count(),
})
);
Ok(())
Expand Down
6 changes: 5 additions & 1 deletion btd/src/output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,11 @@ mod tests {
"depth": 3,
"labels": ["my_label", "another_label"],
"oncall": "my_team",
"reason": {"affected_dep": "cell//foo:bar", "root_cause": ["fbcode//me:test", "inputs"]},
"reason": {
"affected_dep": "cell//foo:bar",
"is_terminal": false,
"root_cause": ["fbcode//me:test", "inputs"],
}
}
);

Expand Down
13 changes: 11 additions & 2 deletions btd/test/test_e2e.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,11 @@ def check_properties(patch, rdeps):
"target": "root//inner:baz",
"type": "my_rule",
"oncall": None,
"reason": {"affected_dep": "", "root_cause": ["root//inner:baz", "inputs"]},
"reason": {
"affected_dep": "",
"root_cause": ["root//inner:baz", "inputs"],
"is_terminal": False,
},
} in rdeps
assert len(rdeps) == 2
elif patch == "rename_inner":
Expand All @@ -217,7 +221,11 @@ def check_properties(patch, rdeps):
"target": "root//inner:baz",
"type": "my_rule",
"oncall": None,
"reason": {"affected_dep": "", "root_cause": ["root//inner:baz", "hash"]},
"reason": {
"affected_dep": "",
"root_cause": ["root//inner:baz", "hash"],
"is_terminal": False,
},
} in rdeps
assert len(rdeps) == 2
elif patch == "buckconfig":
Expand All @@ -233,6 +241,7 @@ def check_properties(patch, rdeps):
"reason": {
"affected_dep": "",
"root_cause": ["root//inner:baz", "package_values"],
"is_terminal": False,
},
}
]
Expand Down

0 comments on commit 3e022b9

Please sign in to comment.