Skip to content

Conversation

tgeng
Copy link
Contributor

@tgeng tgeng commented Apr 8, 2023

Hi friends at Tinder! First thank you for making this awesome project! I work for Pinterest and we are evaluating adopting this over our in-house solution.

One issue I am seeing is that bazel-diff seems to treat the entire external repo as one opaque blob. For example, if one uses this with maven_install, any single third party dependency change ends up trigger a global rebuild.

This PR adds a flag that makes bazel-diff generate fine-grained hash for the explicitly mentioned external repos. This way, bazel-diff is smarter when handling external repos like @maven in a monorepo setting.

Usage:
pass --fineGrainedHashExternalRepos maven1,maven2 to generate-hashes command and fine-grained hashes are generated for targets in the listed external repos.

Note: This is still a draft and hence I have not added any tests to this yet. I would like to get some input on this approach before that. I have tested this with a mono repo with >4M lines of Java code and it seems to be working as expected.

This flag makes bazel-diff generate fine-grained hash for the explicitly
mentioned external repos. This way, bazel-diff is smarter when handling
external repos like `@maven` in a monorepo setting.
@CLAassistant
Copy link

CLAassistant commented Apr 8, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@tinder-maxwellelliott tinder-maxwellelliott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really like the approach! Minor comments

@tgeng tgeng marked this pull request as ready for review April 10, 2023 18:37
@tinder-maxwellelliott
Copy link
Collaborator

@tgeng I am sorry, I am now realizing that fineGrainedHashExternalRepos matches the existing arg style. Can you go back to that?

@tgeng
Copy link
Contributor Author

tgeng commented Apr 10, 2023

@tgeng I am sorry, I am now realizing that fineGrainedHashExternalRepos matches the existing arg style. Can you go back to that?

Np! Pushed another patch.

@tinder-maxwellelliott tinder-maxwellelliott merged commit 74f1114 into Tinder:master Apr 10, 2023
@tinder-maxwellelliott
Copy link
Collaborator

@tgeng Thank you!

@tgeng
Copy link
Contributor Author

tgeng commented Apr 10, 2023

Thanks for the quick response! When can I expect another release? (I can build our own, but we still want to make it clean by using the official build eventually 😉 )

@tinder-maxwellelliott
Copy link
Collaborator

Thanks for the quick response! When can I expect another release? (I can build our own, but we still want to make it clean by using the official build eventually 😉 )

I will upload it now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants