Implement target distance metrics #230
Conversation
The new fields in TargetHash and TargetDigest are to track the direct srcs/attrs hash separately from the overall hash. Currently, the directHash is not used at all.
Add e2e test for dump distances TODO: It would be nice if there was a better e2e test here -- Ask Maxwell how he generates the integration workspace data.
|
|
cli/src/main/kotlin/com/bazel_diff/cli/GetImpactedTargetsCommand.kt
Outdated
Show resolved
Hide resolved
cli/src/test/resources/fixture/impacted_targets_distances-1-2.txt
Outdated
Show resolved
Hide resolved
alex-torok
changed the title
alex-torok
changed the title
| Take a look at the following bazelcon talks to learn more about `bazel-diff`: | ||
|
|
||
| * [BazelCon 2023: Improving CI efficiency with Bazel querying and bazel-diff](https://www.youtube.com/watch?v=QYAbmE_1fSo) | ||
| * BazelCon 2024: Not Going the Distance: Filtering Tests by Build Graph Distance: Coming Soon |
| @@ -136,6 +136,14 @@ class GenerateHashesCommand : Callable<Int> { | |||
| ) | |||
| var ignoredRuleHashingAttributes: Set<String> = emptySet() | |||
|
|
|||
| @CommandLine.Option( | |||
| names = ["-d", "--depsFile"], | |||
There was a problem hiding this comment.
I wonder if we should have a longer flag name here that is more descriptive?
There was a problem hiding this comment.
I updated it to --depEdgesFile. Is that sufficient / do you have any other suggestions?
There was a problem hiding this comment.
That sounds good
cli/src/test/resources/fixture/impacted_targets_distances-1-2.txt
Outdated
Show resolved
Hide resolved
1. Output detailed hash data as json 2. Use a different serialization format for targethashes to avoid ambiguity
alex-torok
force-pushed
the
add-direct-hash-to-rule-hasher
branch
from
f121539
to
02efb3e
Compare
|
@tinder-maxwellelliott - I added a new E2E test that better showcases target / package distances with a new test workspace. I don't think there is anything else I need to do here. |
alex-torok
force-pushed
the
add-direct-hash-to-rule-hasher
branch
from
8adcef4
to
e8eef2a
Compare
| mapOf("label" to "//A:gen_two", "targetDistance" to 1.0, "packageDistance" to 0.0), | ||
| mapOf("label" to "//A:two.sh", "targetDistance" to 2.0, "packageDistance" to 0.0), | ||
| mapOf("label" to "//A:two", "targetDistance" to 3.0, "packageDistance" to 0.0), |
There was a problem hiding this comment.
NOTE: GeneratedFiles contribute to targetDistance. I could see the argument for not doing that and instead having the code rewire the dependencies so that //A:two directly depends on //A:gen_two and the GeneratedFile doesn't contribute to targetDistance.
The odd behavior here is due to GeneratedFiles taking the hash of the target that generated them. I.e. - if A:gen_two is directly modified, so is A:two.sh, but if A:one is directly modified, A:two.sh has a target distance of 2.
alex-torok
force-pushed
the
add-direct-hash-to-rule-hasher
branch
from
e8eef2a
to
a60e006
Compare
|
@alex-torok There have been some changes in main for BCR support, once those are resolved I can merge this |
|
@tinder-maxwellelliott Let me know if there is anything else I can do to help land this. |
Looks like there are some test failures |
|
I think I fixed it, but won't be able to run it locally and see until later today. |
Add target distance metrics to measure how far away an impacted target is from a directly impacted target. I tried to keep the commits relatively concise, so reviewing them one-by-one from the beginning may be easier than looking at all of the changes at once.
Fixes #223