Tinder / bazel-diff

Performs Bazel Target Diffing between two revisions in Git, allowing for Test Target Selection and Selective Building
Other
405 stars 60 forks source link

Implement target distance metrics #230

Closed alex-torok closed 1 month ago

alex-torok commented 1 month ago

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

CLAassistant commented 1 month ago

CLA assistant check
All committers have signed the CLA.

alex-torok commented 1 month ago

@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.

tinder-maxwellelliott commented 1 month ago

@alex-torok There have been some changes in main for BCR support, once those are resolved I can merge this

alex-torok commented 1 month ago

@tinder-maxwellelliott Let me know if there is anything else I can do to help land this.

tinder-maxwellelliott commented 1 month ago

@tinder-maxwellelliott Let me know if there is anything else I can do to help land this.

Looks like there are some test failures

alex-torok commented 1 month ago

I think I fixed it, but won't be able to run it locally and see until later today.

alex-torok commented 1 month ago

@tinder-maxwellelliott - should be passing now (at least it is on my machine)

erikkerber commented 1 month ago

I don't know if it was an intentional side-effect, but the format changes mean hashes generated pre-8.0.1 will fail parsing on 8.0.1+. We noticed since we store digests per commit on CI to speed everything up.

It's pretty transient on our part because we fall back to no work avoidance when bazel-diff fails, but others might be a little surprised.

honnix commented 1 month ago

I don't know if it was an intentional side-effect, but the format changes mean hashes generated pre-8.0.1 will fail parsing on 8.0.1+. We noticed since we store digests per commit on CI to speed everything up.

It's pretty transient on our part because we fall back to no work avoidance when bazel-diff fails, but others might be a little surprised.

We are having the same problem. This change is indeed surprising. I'm wondering what could be a migration path.

alex-torok commented 3 weeks ago

This PR changed how hashes are calculated -- you'll get a different hash value for an old commit on the new version of bazel-diff compared to the old version of bazel-diff.

If you version bazel-diff outside of your repo (i.e use a new version of bazel-diff for calculate-hashes on older commits), then you'd hit cache consistency issues. Versioning it in the repo would just mean that when you bump it, you'd create an 'all targets changed' commit.

If you want the backwards compatibility, you'd need to update the TargetHash parsing to work when reading old hash values - https://github.com/alex-torok/bazel-diff/blob/50b1609cb7ec9db2050bad00e518d6abad3b82dd/cli/src/main/kotlin/com/bazel_diff/hash/TargetHash.kt#L37-L40

We discussed this, but I think I just missed getting it in with a testcase.