Issue #227: Incompatible with the configuration cache

jbduncan commented 1 year ago

What's changed?

This is a draft PR for issue https://github.com/openrewrite/rewrite-gradle-plugin/issues/227, showing my work as I go along.

What's your motivation?

To fix issue https://github.com/openrewrite/rewrite-gradle-plugin/issues/227.

Anything in particular you'd like reviewers to focus on?

I'm trying to figure out how to avoid Gradle-specific things like Project, Configuration, Task and Task.project to allow rewrite-gradle-plugin to be configuration-cache-compatible. Any insights would be appreciated.

Any additional context

I'll mention in https://github.com/gradle/gradle/issues/13490 that this plugin is configuration-cache-incompatible. Also I'm considering hopping on the Gradle Slack or forums to get some help.

Checklist

[x] I've added unit tests to cover both positive and negative cases
[x] https://github.com/openrewrite/rewrite-gradle-plugin/issues/227#issuecomment-1705710548 is no longer a blocker.
[x] I've added the license header to any new files through ./gradlew licenseFormat
[ ] I've used the IntelliJ IDEA auto-formatter on affected files
[ ] I've updated the documentation (if applicable)

jbduncan commented 1 year ago

Blocked until I've received the go ahead to bump the min Gradle version to one that better supports the configuration cache, as per https://github.com/openrewrite/rewrite-gradle-plugin/issues/227#issuecomment-1705710548.

timtebeek commented 1 year ago

Thanks for getting this started! As mentioned in https://github.com/openrewrite/rewrite-gradle-plugin/issues/227#issuecomment-1707455588 we might want to wait until we hear if requiring Gradle 4.3 is an option, and what the upsides versus downsides of that are. I'm not sure the upsides have been discussed much in #227 up to now.

In general I can say we're in the business of (among other things) migrating legacy applications; as such we would like to support going as far back as possible, until such a time that we no longer expect or wish to support migrating from ancient versions. That does have downsides in particular for us as developers on OpenRewrite, but raising the minimum required version before folks can get started with automated migrations is also not ideal.

Let's wait for feedback in the issue; potentially expand a bit on the upsides of versions bump versions, and see from there how to continue.

jbduncan commented 1 year ago

@timtebeek Okey dokey! It's not the end of world if we can't go forward in this direction, as there's the option of turning on a flag in each and every task to indicate they're incompatible with the configuration cache, which solves the problem but without any speed gains.

jbduncan commented 1 year ago

That being said, this flag may only be available in later Gradle versions, so if we need to stick with a config-cache-unaware minimum version of Gradle for the foreseeable future, then so be it. 😄

jbduncan commented 8 months ago

I have many failing tests that I'm not sure how to fix, so when I next have time, I'll copy changes bit by bit into a new local branch.

But even if I fix everything, I've discovered that rewrite-gradle-plugin needs the Gradle root project and subprojects to parse everything, which may be incompatible with the configuration cache. So the simplest change may be to just use notCompatibleWithConfigurationCache to tell Gradle that rewrite-gradle-plugin is incompatible. Stay tuned.

Note for self: use these resources:

knutwannheden commented 8 months ago

Can't a Gradle plugin query the Gradle version at runtime and then conditionally use new features? Then we could use the config cache when available.

jbduncan commented 8 months ago

@knutwannheden True. I'm struggling to see how that would help, though, because it looks like rewrite-gradle-plugin needs to scan the entire Gradle project to work; root project, subprojects and all. My fear is that the config cache needs plugins to work on one subproject at a time, not the whole project, but I could be wrong. Worst case scenario, notCompatibleWithConfigurationCache will do the trick.

WDYT? :)

jbduncan commented 8 months ago

@knutwannheden Please let me know if I've misunderstood you!

knutwannheden commented 8 months ago

My Gradle skills are rather limited, unfortunately. Possibly @shanman190 night have a suggestion.

shanman190 commented 8 months ago

So @jbduncan is correct in the assessment that for configuration cache support the Gradle plugin would have to undergo a pretty drastic overhaul due to the strict requirements set forth by the configuration cache feature. (You can see more here: https://docs.gradle.org/current/userguide/configuration_cache.html#config_cache:requirements)

To illustrate a few of these that would have impact, the DefaultProjectParser would have to have the Micrometer JVM metrics module removed because it accesses JVM state and would have to be restructured such that it accepts details about the project without actually working with the org.gradle.api.Project type and some of its siblings.

Overall, right now, I think that it's in the realm of being achievable, but it would require a bit of familiarity with the inner workings of that class in specific.

jbduncan commented 8 months ago

Overall, right now, I think that it's in the realm of being achievable, but it would require a bit of familiarity with the inner workings of that class in specific.

Indeed, so it will take me some time to fix it.

I'm hoping notCompatibleWithConfigurationCache will prove to be a good tactical solution in the meantime. I'll report back when I have more info.

jbduncan commented 8 months ago

So, the configuration cache was introduced in Gradle 6.6, but my testing shows that notCompatibleWithConfigurationCache was introduced in Gradle 7.4.

Therefore, I'll update this PR to conditionally use notCompatibleWithConfigurationCache if the current Gradle is 7.4+.

I'll make another PR in the future to see if I can make everything config-cache-compatible (following @shanman190's advice) as far back as Gradle 6.6 and remove the notCompatibleWithConfigurationCache call, hopefully making the plugin faster in the process.

In summary, this PR will make rewrite-gradle-plugin config-cache-compatible for Gradle 7.4+.

jbduncan commented 8 months ago

Team, I'm ready for a review. Hit me. :fist_left:

jbduncan commented 7 months ago

Sorry for the delay, this PR fell off my radar! I believe I've dealt with @timtebeek's feedback, so I'm ready for another round of reviews.

jbduncan commented 3 months ago

@timtebeek @shanman190 I found the time to revisit this today and I fixed a merge conflict, so I'm ready again for another round of reviews and I wanted to ask if there's anything I can do to speed things along?

jbduncan commented 1 month ago

I've found the time to revisit this again, and I'm wondering if there's anything I can do to help move things forward?

timtebeek commented 1 month ago

Apologies @jbduncan ! Looks like I'd missed your previous ping. I just got home from a conference that day and it must have slipped by me. I've resolved the conflicts with the main branch; those were benign: a copyright header change on files removed here. IDEA did mess up the commit message, but we'll squash merge anyway.

Thanks again for the continued work on this! I must admit I know less about developing Gradle plugins than you do, so it's kind of hard to review this beyond going over the changes I can see. Would you mind sharing if and what tests you've ran through to verify these changes?

In terms of verification I'm thinking we could do a cross table of this new plugin against various versions of Gradle x various scenarios like single module, multi module, and init-gradle usage. And would we have to run twice to check the configuration cache is used?

shanman190 commented 1 month ago

@timtebeek, so that was one of the changes here was to mark all of the tasks as not compatible which fixes the issue in the newer Gradle versions.

Beyond that 3 line code change found in AbstractRewriteTask's constructor. The rest of the changes were from early attempts to become configuration cache compatible. In order to become configuration cache compatible would require some pretty drastic refactoring due to the requirements of configuration cache and how the OpenRewrite Gradle plugin is written today.

I kept meaning to come back to this, but like you it got away from me. The thought going through my head is to do we just clean things up to that minimal 3 line code change or take on these additional dependency resolution updates as well?

jbduncan commented 1 month ago

@shanman190 @timtebeek Thank you very much both for reviewing this and merging this in!

I've lost a bit of context now, but @shanman190 is right indeed that the main change is to the AbstractRewriteTask constructor, which ensures that the plugin tells Gradle 7.4+ that it's incompatible with the configuration cache, disabling any warnings that may appear.

It's better than nothing because older versions of Gradle simply have no way of disabling such warnings, and the OpenRewrite plugin would need extensive refactoring to use the cache properly.

openrewrite / rewrite-gradle-plugin