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

Add initial recipes for Micronaut 4. #50

Merged
merged 26 commits into from
Jun 7, 2023
Merged

Conversation

jeremyg484
Copy link
Contributor

What's changed?

An initial set of recipes have been added for upgrading to Micronaut 4. See issue #49

This PR is based on the branch from the pending PR #48 for upgrading to OpenRewrite 8.0, thus the intial code developed
against OpenRewrite 7.x at https://github.com/jeremyg484/rewrite-micronaut4 has been merged and refactored to
accomodate for the OpenRewrite 8.0 changes.

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've added the license header to any new files through ./gradlew licenseFormat
  • I've used the IntelliJ auto-formatter on affected files

@jeremyg484
Copy link
Contributor Author

I have updated the PR after merging with the latest from main.

There is one test that is now failing due to a classpath issue, and I could use some suggestions on how to resolve it. The problematic test is FixDeprecatedExceptionHandlerConstructorsTest#updatesErrorProcessorConstructor. This is one of the prior existing tests from the Micronaut 2-to-3 recipes. I believe it fails because the "before" version of the Java code being tested is using the default constructor of ConversionErrorHandler which was previously deprecated but has now been removed for Micronaut 4. We are pulling the Micronaut 4 dependencies into the test runtime classpath.

Is there a suggested strategy for handling such a case where two different versions of a dependency would be needed on the classpath for the "before" and "after" version of a class being modified?

I would also like to specifically ask for a review of AddSnakeYamlDependencyIfNeeded to see if the approach I'm taking is completely appropriate and expected with the changes in the 8.0 APIs. The general idea is that we need a set of recipes to run to add the snakeyaml dependency to the classpath, but only if we first can verify that the Micronaut app in question is using yaml for configuration by checking for the existence of certain yaml files. When I was basing against the 7.x version, this was done as a declarative recipe with an applicability test. The version here in the PR is done completely imperatively instead, but I'm not convinced that what I've had to do to make the other recipes run conditionally is totally appropriate.

@timtebeek
Copy link
Contributor

Thanks a lot for this initial work, and keeping it up to date with our 8.0 developments! We're nearing a release there, which is taking up quite some of our time. In some cases we're still working out the final details of what intended use in 8.0 should look like, which might affect this pull request as well. So know that work is ongoing, and we hope to come back here once we finalize the intended targets.

As for

Is there a suggested strategy for handling such a case where two different versions of a dependency would be needed on the classpath for the "before" and "after" version of a class being modified?

Had you already read our guide on using multiple versions? https://docs.openrewrite.org/authoring-recipes/multiple-versions
That's what we do (to an extreme extend) in rewrite-spring to facilitate such removed method migrations, which might otherwise lead to mismatches in the type system. Happy to help if you can't get that to work immediately, although it might be a day or two before the dust from 8.0 settles.

@jeremyg484
Copy link
Contributor Author

Had you already read our guide on using multiple versions? https://docs.openrewrite.org/authoring-recipes/multiple-versions That's what we do (to an extreme extend) in rewrite-spring to facilitate such removed method migrations, which might otherwise lead to mismatches in the type system. Happy to help if you can't get that to work immediately, although it might be a day or two before the dust from 8.0 settles.

Thanks for the links, I somehow had missed that one...looks like exactly what is needed, and I'll take a peek at the way rewrite-spring is doing things as well for deeper examples.

@timtebeek timtebeek added the recipe Recipe requested label Jun 1, 2023
@jeremyg484
Copy link
Contributor Author

@timtebeek I've refactored all of the tests to use recipeDependencies. It all passes locally, but the dependencies seem to be failing to get resolved in CI. Any suggestions? I thought maybe I had some Gradle caching that was making things work locally, but was able to execute everything from a clean cache, etc.

@timtebeek
Copy link
Contributor

I got things to work locally by first running ./gradlew downloadRecipeDependencies, which put the jars in src/main/resources/META-INF/rewrite/classpath. I've not committed those yet, as I had expected we download those in CI as well. 🤔 I'll ask internally what we expect to happen and do there, as it's perhaps not ideal to have them inside the repository.

@timtebeek
Copy link
Contributor

@sambsnyd What's the expected flow here with regards to parserClasspath? Do we add the jars to the repository? Or should downloadRecipeDependencies already be tied into the Ci pipeline somehow? It looks like it's failing on that.

@jeremyg484
Copy link
Contributor Author

I got things to work locally by first running ./gradlew downloadRecipeDependencies, which put the jars in src/main/resources/META-INF/rewrite/classpath. I've not committed those yet, as I had expected we download those in CI as well. 🤔 I'll ask internally what we expect to happen and do there, as it's perhaps not ideal to have them inside the repository.

Right, my hope was that the recipe-library plugin would do that already as part of a full build so that we don't have to commit these dependencies to the repository. I was using downloadRecipeDependencies at first during development, but then I cleaned my build directory and wiped that src/main/resources/META-INF/rewrite/classpath directory and it still worked, leading me to think the plugin must be doing some magic. :) But now as I dig deeper, I think there is definitely some Gradle caching going on that makes it only appear to work without running downloadRecipeDependencies first. Adding a new dependency without running that task definitely leads to the same failure.

So then I supposed the question just becomes, what is the best way to get downloadRecipeDependencies running by default with the build?

@sambsnyd
Copy link
Member

sambsnyd commented Jun 5, 2023

Hi @jeremyg484 as of right now the plugin does no magic and does not automatically update included jars. If things worked for you with that directory deleted, it was indeed a false-positive due to a cache hit. For now the intention is indeed that the jars are checked in to source control.

@jeremyg484
Copy link
Contributor Author

Hi @jeremyg484 as of right now the plugin does no magic and does not automatically update included jars. If things worked for you with that directory deleted, it was indeed a false-positive due to a cache hit. For now the intention is indeed that the jars are checked in to source control.

Ok, thanks for the clarification @sambsnyd, I'll go ahead and add them to the repo then.

Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Overall I'd say this looks very good already; I've added some comments throughout where I think we can do better looking forward. Feel free to tackle those as you see fit, or let me know if you need help.

src/main/resources/META-INF/rewrite/micronaut3-to-4.yml Outdated Show resolved Hide resolved
src/main/resources/META-INF/rewrite/micronaut3-to-4.yml Outdated Show resolved Hide resolved
@Language("groovy")
private final String buildGradleInitial = """
plugins {
id("io.micronaut.application") version "4.0.0-M2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now I see a lot of explicit version numbers in the tests, which might not work well if we move over to xrange versions, or merely switch from 4.0.0-M2 to 4.0.0. We have alternatives that allow you to assert without pinning a particular version, which might be interesting to apply here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, we should definitely use that assertion style going forward, but I think keep it as-is until we're closer to the GA release and can fully move to Xrange versions.

Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

A last couple optional suggestions, but otherwise I think we're good to merge! Nice work on this in how quickly you were able to produce the recipes and resolve the review feedback.

We might be able to do some touch ups after the merge with some automated recipes, for instance remove public visibility modifiers from tests, formatting the text blocks and more; no need to make that part of this PR; we have the platform to perform those changes easily.

Then once you're closer to a release we could update the milestones again to be sure, and then finally after the release quickly update the version xrange patterns, such that it will all just work for developers looking to migrate automatically to the release by then.

When are you roughly planning to release 4.0? And would we need to coordinate to get a timely release of this module out at the same time?

@jeremyg484
Copy link
Contributor Author

@timtebeek

Then once you're closer to a release we could update the milestones again to be sure, and then finally after the release quickly update the version xrange patterns, such that it will all just work for developers looking to migrate automatically to the release by then.

When are you roughly planning to release 4.0? And would we need to coordinate to get a timely release of this module out at the same time?

M3 of the full Micronaut platform should hopefully be released today, and I will create a separate PR after this gets merged to update and test the recipe against M3 ASAP. The plan is to then have one more milestone release, and then potentially two RC releases before going final. The goal is to get these all released within a couple weeks of each other. If we manage to stick to that, then we're hopefully looking at a July final release. Prior to that, I need to go through all of the Micronaut modules and look for additional breaking changes that might be able to be addressed with the recipe.

Yes, ideally the release of the OpenRewrite module will be coordinated closely with the Micronaut 4 GA release so that our users will be able to immediately use it. I'd like to have a section in the Micronaut docs (as we currently do for v3 https://docs.micronaut.io/latest/guide/#upgrading) that explains using this recipe.

I'd also like to go ahead and get this into the hands of our users during the milestone/RC phases, though it doesn't need to be quite as tightly coordinated. What's the best way to do that? Do you think we could get a proper release of this out for them to try out in that time period as well, or should we just be pointing them to snapshots? I'd like to perhaps do a blog post with instructions sometime after M3 is out and after this recipe is working with it to motivate users to give it a try.

@jeremyg484
Copy link
Contributor Author

@timtebeek Getting random test failures now (as seen in CI) with

Caused by:
java.lang.OutOfMemoryError: Java heap space

I believe it's happening only with tests that use RewriteTest.fromRuntimeClasspath. I wonder if something changed in there recently to cause the memory error?

@timtebeek
Copy link
Contributor

@timtebeek Getting random test failures now (as seen in CI) with

Caused by:
java.lang.OutOfMemoryError: Java heap space

I believe it's happening only with tests that use RewriteTest.fromRuntimeClasspath. I wonder if something changed in there recently to cause the memory error?

I didn't even know about that method; looks like we've only ever used it twice before. 😅 Guess we have some debugging to do there, or look for alternatives.

timtebeek added 2 commits June 7, 2023 20:08
Also only scan once instead of twice
@timtebeek
Copy link
Contributor

timtebeek commented Jun 7, 2023

I've just pushed dd26d98 ; waiting to see if that helps, although GitHub appears to have issues starting a pipeline.

Here's a manually started pipeline: https://github.com/openrewrite/rewrite-micronaut/actions/runs/5203215794/jobs/9385766787 🤞🏻

@jeremyg484
Copy link
Contributor Author

Looks like that did the trick. I'd say we're good to merge now.

@timtebeek timtebeek merged commit f777d3d into main Jun 7, 2023
@timtebeek timtebeek deleted the Micronaut3to4_OpenRewrite8 branch June 7, 2023 18:47
@timtebeek
Copy link
Contributor

Great to hear! Thanks again for your work on this.

@jeremyg484
Copy link
Contributor Author

Great to hear! Thanks again for your work on this.

No problem, thanks for the help!

@timtebeek
Copy link
Contributor

M3 of the full Micronaut platform should hopefully be released today, and I will create a separate PR after this gets merged to update and test the recipe against M3 ASAP. The plan is to then have one more milestone release, and then potentially two RC releases before going final. The goal is to get these all released within a couple weeks of each other. If we manage to stick to that, then we're hopefully looking at a July final release. Prior to that, I need to go through all of the Micronaut modules and look for additional breaking changes that might be able to be addressed with the recipe.

Sounds good! We're running a little late with 8.0, but this timeline should be fine for us to be ready for a release before your 4.0 .

Yes, ideally the release of the OpenRewrite module will be coordinated closely with the Micronaut 4 GA release so that our users will be able to immediately use it. I'd like to have a section in the Micronaut docs (as we currently do for v3 https://docs.micronaut.io/latest/guide/#upgrading) that explains using this recipe.

Good to know! We'll be sure to keep the repository in a patch-releasable state then!

I'd also like to go ahead and get this into the hands of our users during the milestone/RC phases, though it doesn't need to be quite as tightly coordinated. What's the best way to do that? Do you think we could get a proper release of this out for them to try out in that time period as well, or should we just be pointing them to snapshots? I'd like to perhaps do a blog post with instructions sometime after M3 is out and after this recipe is working with it to motivate users to give it a try.

It'll be easiest once we get 8.0 out the door; then at least the versions of rewrite, our boms & plugins should all align without any issues. We hope to have all of that sorted within this week, if no further surprises come up. Happy to help on instructions for people to try out M3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Recipe requested
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants