-
Notifications
You must be signed in to change notification settings - Fork 290
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
Install GJF hook using a gradle task, rather than a gradlew hack. #298
Conversation
No integration test for this either, but I tried:
And also:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -38,6 +38,7 @@ subprojects { project -> | |||
errorproneJavac deps.build.errorProneJavac | |||
} | |||
project.tasks.withType(JavaCompile) { | |||
dependsOn(installGitHooks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of curiosity, does this task cache correctly? Or just cheap enough to run every time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I see here: https://docs.gradle.org/current/userguide/build_cache.html#sec:task_output_caching_details
Some tasks, like Copy or Jar, usually do not make sense to make cacheable because Gradle is only copying files from one location to another. It also doesn’t make sense to make tasks cacheable that do not produce outputs or have no task actions.
I wonder if there's a friendlier way to handle this, such as using the spotless gradle plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't be cached, but involves only copying a single 358 bytes file, so checking the cache would not be appreciably faster, I think :) (and makes sure the hook gets re-added if the user removes it somehow). Is there a problem with it I am not seeing?
@ZacSweers is your concern around build times? Or is there some other problem with this approach? |
Mostly concern around build times but fair point that this is probably as
expensive as a cache hash check anyway. Fwiw we moved to Spotless in
AutoDispose
…On Mon, Apr 8, 2019 at 11:41 AM Lázaro Clapp ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In build.gradle
<#298 (comment)>:
> @@ -38,6 +38,7 @@ subprojects { project ->
errorproneJavac deps.build.errorProneJavac
}
project.tasks.withType(JavaCompile) {
+ dependsOn(installGitHooks)
This won't be cached, but involves only copying a single 358 bytes file,
so checking the cache would not be any faster, I think :) (and makes sure
the hook gets re-added if the user removes it somehow). Is there a problem
with it I am not seeing?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#298 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABTEvnq5kPHT-SIqm7-fKyneRF6egcz4ks5ve41VgaJpZM4cfOXF>
.
|
Fixes #297