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

Gradle cache #155

Merged
merged 5 commits into from
Apr 6, 2020
Merged

Gradle cache #155

merged 5 commits into from
Apr 6, 2020

Conversation

mkmaier
Copy link
Contributor

@mkmaier mkmaier commented Jan 9, 2020

Apart from the https/rat stuff, this is the minimal needed change to support gradle build caching properly.
It is still necessary to enable the cache explicitly with outputs.cacheIf { true }. This is intentional, because caching will cause no warnings to be logged if there is a cache hit, which might confuse users. This is not a problem with ignoreFailures = false, since failed tasks should not be cached.

@uschindler
Copy link
Member

Hi,
this would fix #146, but the other issue mentions that a "cacheable" task also needs to explicitly marked as "cacheable": https://docs.gradle.org/current/userguide/build_cache.html#sec:task_output_caching_details

@uschindler
Copy link
Member

We can use cacheIf to enable caching only if ignoreFailures == false. That's easy to add to the plugin-init.groovy.

@mkmaier
Copy link
Contributor Author

mkmaier commented Jan 9, 2020

this would fix #146, but the other issue mentions that a "cacheable" task also needs to explicitly marked as "cacheable": https://docs.gradle.org/current/userguide/build_cache.html#sec:task_output_caching_details

One of @CachableTask or outputs.cacheIf { true } is enough.

We can use cacheIf to enable caching only if ignoreFailures == false. That's easy to add to the plugin-init.groovy.

My change fixes the UPTO-DATE check for the task, so now it can actually be UPTO-DATE.
Combined with your trick of declaring @OutputDirectories public FileCollection getClassesDirs(), this means that the task will always be UPTO-DATE if the respective compile task input/output is unchanged (even if that just came from cache). So what I wrote above about having to enable the cache is not correct.
Most of the time, this will have the same effect as using the cache for this task, but it's not perfect:

  1. If caching is enabled manually, the compile output will probably be cached twice. This should be fixed by declaring a real output file in Allow to produce a text/xml/json/... report of the failures for better CI integration #134, anything else will be a workaround that is not much better than the current situation.
  2. If failures are ignored, the check will still not run every time (even if caching is not enabled for the task), which might confuse users, but is really a matter of taste. If you want this to be fixed, then outputs.upToDateWhen { false } should be used in that case, but I'm not sure this is really worth the trouble, and it would also be better to fix this by outputting the warnings to a file.

What's your opinion on this? Do you want me to change anything?

@uschindler uschindler self-assigned this Jan 9, 2020
@uschindler
Copy link
Member

If failures are ignored, the check will still not run every time (even if caching is not enabled for the task), which might confuse users, but is really a matter of taste.

This was also the case before. Actually the uptodate check was working most of the time. Maybe it changed in one later Gradle version when @classpath was introduced.

Nevertheless I like your change. I just have to think when to include this.

One opportunity would be to release the Java 14 update in March as version 3.0, so I can also remove the old deprecated stuff, too. In that case I will add some more commits afterwards to change minimum java version and remove *-1.6 signatures files (merge them into the 1.7 ones).

Do you want me to change anything?

All fine as first step.

@uschindler uschindler added this to the 3.0 milestone Apr 6, 2020
@uschindler
Copy link
Member

I am working on #160, which will update to Java 7 and forbiddenapis 3.0. After this is merged, I will apply your PR.

Thanks anyways and sorry for the delay!

@uschindler uschindler merged commit f8c8fa5 into policeman-tools:master Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants