openrewrite / rewrite-gradle-plugin

OpenRewrite's Gradle plugin.
Apache License 2.0
64 stars 40 forks source link

Add update hint - recipes are not found any more when updating from 5.40.6 to 6.0.0 #203

Closed koppor closed 1 year ago

koppor commented 1 year ago

When updating from 5.40.6 to 6.0.0, I get following output:

Recipe validation error in org.jabref.config.rewrite.cleanup.recipeList[34] (in file:/C:/git-repositories/jabref/rewrite.yml): recipe 'org.openrewrite.java.cleanup.RemoveExtraSemicolons' does not exist.
Recipe validation error in org.jabref.config.rewrite.cleanup.recipeList[35] (in file:/C:/git-repositories/jabref/rewrite.yml): recipe 'org.openrewrite.java.cleanup.RemoveJavaDocAuthorTag' does not exist.
Recipe validation error in org.jabref.config.rewrite.cleanup.recipeList[36] (in file:/C:/git-repositories/jabref/rewrite.yml): recipe 'org.openrewrite.java.cleanup.RemoveRedundantTypeCast' does not exist.
Recipe validation error in org.jabref.config.rewrite.cleanup.recipeList[37] (in file:/C:/git-repositories/jabref/rewrite.yml): recipe 'org.openrewrite.java.cleanup.RemoveUnneededAssertion' does not exist.

I checked https://docs.openrewrite.org/running-recipes/getting-started, but that still lists 5.40.6.

I checked the diff https://github.com/openrewrite/rewrite-gradle-plugin/compare/v5.40.5...v6.0.0, but could not see why this happens.

Full project setup at https://github.com/JabRef/jabref/blob/76c35f0afb6f9c700ee35a5475bbe9ff612fa75c/build.gradle#L487

koppor commented 1 year ago

Update: I get following output:

Unable to configure org.openrewrite.java.logging.SystemPrintToLogging
org.openrewrite.config.RecipeIntrospectionException: Unable to call primary constructor for Recipe class org.openrewrite.java.logging.SystemPrintToLogging
...
Caused by: java.lang.NoSuchMethodError: 'org.openrewrite.Recipe org.openrewrite.java.logging.SystemPrintToLogging.doNext(org.openrewrite.Recipe)'
        at org.openrewrite.java.logging.SystemPrintToLogging.<init>(SystemPrintToLogging.java:76)
        at java.base/jdk.internal.reflect.DirectConstructorHandleAccessor.newInstance(DirectConstructorHandleAccessor.java:67)

Which I didn't get at 5.40.6.

I disabled all logging checks, the error of not found recipes persists.

timtebeek commented 1 year ago

Hi @koppor ; we're still working on our 8.0 release, and as a part of that have released some components already, but have not yet completed the full release, and as a result of that have not updated the documentation yet either.

Once we have fully released 8.0, there will be updated documentation and resources on how to upgrade, including some automated migration recipes.

In your specific case the associated recipes have been moved to a separate repository: https://github.com/openrewrite/rewrite-static-analysis/tree/main/src/main/java/org/openrewrite/staticanalysis This way it's easier for new contributors to get started there, as it's a common source of beginner friendly recipes. We have already released a first 1.0.0 of rewrite-static-analysis, so you can already adopt that if you want to get ahead.

Just know that things are slightly in flux while we're working through this release; we hope to stabilize & document all of this quickly!

koppor commented 1 year ago

Thank you for the quick answer. Seems, I need to be patient. A simple update of the recipes's names did not help

recipe 'org.openrewrite.staticanalysis.cleanup.RemoveUnneededAssertion' does not exist.

The compileOnly dependency to rewrite-java is also not helping.

compileOnly 'org.openrewrite:rewrite-java:8.0.0'

Ah, I need to use org.openrewrite.recipe:rewrite-static-analysis

compileOnly 'org.openrewrite.recipe:rewrite-static-analysis:1.0.0'

OK, I am giving up and waiting for a full release.

koppor commented 1 year ago

One more:

rewrite(platform("org.openrewrite.recipe:rewrite-recipe-bom:2.0.0"))

also does not help

Link to mvnrepository: https://mvnrepository.com/artifact/org.openrewrite.recipe/rewrite-recipe-bom/2.0.0

timtebeek commented 1 year ago

We're glad to have you verify our work early; to reply quickly to the above: the new class is at: org.openrewrite.staticanalysis.RemoveUnneededAssertion in rewrite-static-analysis, with indeed the new GAV dependency you've discovered. Again: work in progress, but it should mostly work already. Let us know if you find any issues with the above!

koppor commented 1 year ago

I had a .cleanup too much in the rule names.

Still not sure how to add the dependency.

I have no success with

-    rewrite(platform("org.openrewrite.recipe:rewrite-recipe-bom:1.19.4"))
+    rewrite(platform("org.openrewrite.recipe:rewrite-recipe-bom:2.0.0"))
+    compileOnly 'org.openrewrite.recipe:rewrite-static-analysis:1.0.0'

and no success with

-    rewrite(platform("org.openrewrite.recipe:rewrite-recipe-bom:1.19.4"))
+    rewrite(platform("org.openrewrite.recipe:rewrite-recipe-bom:2.0.0"))
+    rewrite(platform("org.openrewrite.recipe:rewrite-static-analysis:1.0.0"))
timtebeek commented 1 year ago

I've had a quick look; right now you're using: https://github.com/JabRef/jabref/blob/76c35f0afb6f9c700ee35a5475bbe9ff612fa75c/build.gradle#L224-L225

    rewrite(platform("org.openrewrite.recipe:rewrite-recipe-bom:1.19.3"))
    rewrite("org.openrewrite.recipe:rewrite-logging-frameworks")

I'd expect things to work when using:

    rewrite(platform("org.openrewrite.recipe:rewrite-recipe-bom:2.0.0"))
    rewrite("org.openrewrite.recipe:rewrite-logging-frameworks")
    rewrite("org.openrewrite.recipe:rewrite-static-analysis")

Given that rewrite-recipe-bom-2.0.0.pom manages the version of rewrite-static-analysis-1.0.0.pom

Does that work on your end?

koppor commented 1 year ago

Thank you for the patience and quick look. I indeed did it locally only - and did not push anything.

Included your changes and comments (https://github.com/JabRef/jabref/pull/9975/commits/7f6f080672840dac9162b0bff6b877ace05ca013) and removed recipies no longer being available (https://github.com/JabRef/jabref/pull/9975/commits/618996c05a9bf241b92636fa9367366bee96f4b0)

Is there a chance that SimplifyBooleanReturn (applied at https://github.com/JabRef/jabref/pull/9885) and SimplifyBooleanExpression are coming back?

I think, I won't miss MethodParamPad, PadEmptyForLoopComponents, UnnecessaryParentheses that much. UnnecessaryParentheses caused internal discussions (https://github.com/JabRef/jabref/pull/9876#issuecomment-1543359637) ^^.

timtebeek commented 1 year ago

Good to see those updates already; the SimplifyBooleanExpression never moved; that's still available by default in the original package.

koppor commented 1 year ago

Good to see those updates already; the SimplifyBooleanExpression never moved; that's still available by default in the original package.

One more additional dependency for me then? ^^ - Shouldn't it move, too?

timtebeek commented 1 year ago

One more additional dependency for me then? ^^ - Shouldn't it move, too?

You don't need a dependency for the recipes (still) in the original cleanup package. We've kept the recipes that we call directly from other recipes in openrewrite/rewrite. The others we have since moved to rewrite-static-analysis, as you've found.

We're still in the midst of this release and documenting it all, so I appreciate you trying it out and finding where we should provide guidance in the migration.

koppor commented 1 year ago

We're still in the midst of this release and documenting it all, so I appreciate you trying it out and finding where we should provide guidance in the migration.

I summarize what we discussed, maybe it helps:

Migration guide

  1. Replace id("org.openrewrite.rewrite") version("5.40.6") by id("org.openrewrite.rewrite") version("6.0.0")
  2. Replace rewrite(platform("org.openrewrite.recipe:rewrite-recipe-bom:1.19.4")) by following three lines:

    rewrite(platform("org.openrewrite.recipe:rewrite-recipe-bom:2.0.0"))
    rewrite("org.openrewrite.recipe:rewrite-logging-frameworks")
    rewrite("org.openrewrite.recipe:rewrite-static-analysis")
  3. Remove following rules
    • org.openrewrite.java.cleanup.MethodParamPad
    • org.openrewrite.java.cleanup.PadEmptyForLoopComponents
  4. Replace org.openrewrite.java.cleanup by org.openrewrite.staticanalysis in the recipe references (most rules have been migrated to staticanalysis namespace)
  5. Replace back the names of following recipes (they still reside in the java.cleanup namespace)
    • SimplifyBooleanExpression: org.openrewrite.java.cleanup.SimplifyBooleanExpression
    • SimplifyBooleanReturn: org.openrewrite.java.cleanup.SimplifyBooleanReturn
    • UnnecessaryParentheses: org.openrewrite.java.cleanup.UnnecessaryParentheses
timtebeek commented 1 year ago

@mike-solomon see the above comment for when it comes time to document and announce the new release. we might even be able to automate some of those moved recipe changes in code and yaml files. Also captured in https://github.com/openrewrite/rewrite/issues/3234

koppor commented 1 year ago

I think, this thing is really in flux, therefore, I just comment here instead of rasing a new issue.

Bumping org.openrewrite.rewrite from 6.1.4 to 6.1.6 (https://github.com/JabRef/jabref/pull/10037) leads to following error:

> java.lang.RuntimeException: Error while visiting src/jmh/java/org/jabref/benchmarks/Benchmarks.java: java.lang.NoSuchMethodError: 'boolean org.openrewrite.java.MethodMatcher.matches(org.openrewrite.java.tree.J$MethodInvocation)'

I was hoping that an increase of a patch version does not lead to errors during usage. The solution is unfortunately not to bump org.openrewrite.recipe:rewrite-recipe-bom from 2.0.1 to 2.0.2 at the same time (tried at https://github.com/JabRef/jabref/pull/10040). Of only that is bumped, it fails too.

timtebeek commented 1 year ago

There has unfortunately been a bit of a botched change recently, which was subsequently reverted, that changed the signature of the MethodMatcher. It should all work again with the latest versions, so it's curious to hear you're having these issues. In general we recommend using all the latest version, but if there's a combination that's not working yet we should maybe bring out an additional patch release to get things going.

timtebeek commented 1 year ago

We're squashing every last instance of that NoSuchMethodError with patch releases as needed; We'll follow up with a rewrite-recipe-bom release once confirmed fixed, such that it's not necessary to pin individual modules anymore.

We've also documented the changes needed to pick up OpenRewrite 8.x over on https://docs.openrewrite.org/changelog/8-1-2-release That should hopefully resolve this issue, as we've incorporated the much of the suggestions above into either automated or documented migration steps.

Would you agree with closing the issues as done @koppor ? Or is there anything you'd want to still tackle as part of this issue, or in a subsequent issue?

koppor commented 1 year ago

For me, the upgrade worked with your help. Thank you again. - The linked update doc looks good.

In case, I miss something at subsequent updates, I'll raise my voice.

koppor commented 1 year ago

Think, I need to drop recipies at the update.

-    id 'org.openrewrite.rewrite' version '6.1.4'
+    id 'org.openrewrite.rewrite' version '6.1.7'

-    rewrite(platform("org.openrewrite.recipe:rewrite-recipe-bom:2.0.1"))
+    rewrite(platform("org.openrewrite.recipe:rewrite-recipe-bom:2.0.2"))

Would be nice if the default error output would include the failed recipe, because manually disabling recipeies one by one is difficult. And there is no good "gid bisect" equivalent for the rewrite.yml file ^^.

* What went wrong:
Execution failed for task ':rewriteDryRun'.
> java.lang.RuntimeException: Error while visiting src\jmh\java\org\jabref\benchmarks\Benchmarks.java: java.lang.NoSuchMethodError: 'boolean org.openrewrite.java.MethodMatcher.matches(org.openrewrite.java.tree.J$MethodInvocation)'
    org.openrewrite.staticanalysis.EqualsAvoidsNullVisitor.visitMethodInvocation(EqualsAvoidsNullVisitor.java:48)
    org.openrewrite.staticanalysis.EqualsAvoidsNullVisitor.visitMethodInvocation(EqualsAvoidsNullVisitor.java:32)
    org.openrewrite.java.tree.J$MethodInvocation.acceptJava(J.java:3706)
    org.openrewrite.java.tree.J.accept(J.java:59)
    org.openrewrite.TreeVisitor.visit(TreeVisitor.java:278)
    org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:361)
    org.openrewrite.java.JavaVisitor.visitLeftPadded(JavaVisitor.java:1312)
    org.openrewrite.java.JavaVisitor.visitAssignment(JavaVisitor.java:310)
    ...
timtebeek commented 1 year ago

The rewrite-recipe-bom still needs a patch release; will go out as soon as I hear the issues with rewrite-static-analysis and rewrite-migrate-java have been resolved with their latest patch releases.