Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include *.kt files in buildSrc in cache key #274

Merged
merged 6 commits into from Aug 10, 2022

Conversation

schuenadel
Copy link
Contributor

Description:
When using Gradle with the Kotlin DSL some people use Kotlin files like Versions.kt or Dependencies.kt in the buildSrc folder to define their dependency versions (see example here and general info about buildSrc here).
At the moment a change in these files would not be covered by the file hash in the cache key calculation, which means an updated dependency would be missed and so not been cached.

This PR adds the files buildSrc/**/*.kt to the cache key calculation (in addition to the existing **/*.gradle*, **/gradle-wrapper.properties)

Related issue:
None

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

Link to test run containing these changes: https://github.com/schuenadel/setup-java/actions/runs/1692869040

@vlsi
Copy link

vlsi commented Jan 15, 2022

Please, no.
Kotlin in buildSrc is used for much more than "versions", and using Versions.kt in buildSrc is an anti-pattern.

If you include *.kt, then you basically break cacheability for everybody 🤷

@schuenadel
Copy link
Contributor Author

Hi @vlsi,

Kotlin in buildSrc is used for much more than "versions"

Yes, I was coming from my "personal Android bubble" where I see it mainly being used for dependencies and if for something else, then rather rarely touched and if so it maybe even rectify a cache miss. But I get your point that this may be different for other, maybe more infrastructure related, projects where the refresh would not be desired.

and using Versions.kt in buildSrc is an anti-pattern.

I wasn't aware of that and also don't know why it is (I'd say invalidating the build cache on changes doesn't really count for clean builds on CI, if that's the reason...). But anyway many people use it and I think it would be nice to enable them to also benefit from a working cache mechanism.

I preferred to not hardcode the file names as that forces you to use these specific names in order to make the cache work, but maybe that's the better compromise to not have unwanted cache invalidations (as you pointed out).
So my suggestion would be to use buildSrc/**/Versions.kt and buildSrc/**/Dependencies.kt instead of buildSrc/**/*.kt

@schuenadel schuenadel requested a review from a team Apr 19, 2022
@schuenadel schuenadel force-pushed the include-buildSrc-in-cache-key branch from 38f0890 to ca584e7 Compare Apr 25, 2022
@schuenadel
Copy link
Contributor Author

Hi @vlsi (and others),
I added my suggestion

So my suggestion would be to use buildSrc//Versions.kt and buildSrc//Dependencies.kt instead of buildSrc/**/*.kt

to restrict the scope of the cache key to the most used(?) files for dependency management. WDYT?

dsame
dsame approved these changes Jun 29, 2022
@schuenadel
Copy link
Contributor Author

Could this be merged, or is something missing? I am lacking rights for merging.

@marko-zivic-93 marko-zivic-93 merged commit ef96bec into actions:main Aug 10, 2022
154 checks passed
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.

None yet

4 participants