openrewrite / rewrite

Automated mass refactoring of source code.
https://docs.openrewrite.org
Apache License 2.0
2.15k stars 321 forks source link

Problem with maybeUpdateModel #1582

Closed gsmet closed 2 years ago

gsmet commented 2 years ago

Hi there,

So after all the PRs you merged (thanks!), I can nearly use a vanilla version of OpenRewrite for our ongoing migration of Quarkus to Jakarta 9/10. There is one remaining issue though that is a bit hairy and that I was not able to solve all by myself.

A bit of context:

Now... here comes the problem: whenever there is an update of the model (the one triggered by maybeUpdateModel();), it breaks my transformation .

This manifests in two forms:

As for the latter, there are more issues: the AddDependency has more required parameters than I would like and it's doing clever things by default, whereas I just want to add a dependency with no version (it's managed at a higher level) and specifically in the POM I target without checking anything because I know what I'm doing. I understand being clever is a good thing but in this case, I really want to add my dependency the way I want it.

As for the update model thing, I see it's trying to download POM files for my local projects currently rewritten (it maybe tries to get them from the Maven cache first but they are not around), it just prints a WARN. I don't see any error. I find it weird that it tries to reload the POM files whereas I'm actually in a Maven reactor building them. Can't it somehow reload them from the project itself in this case? Not sure it's related to my problem though.

Funnily enough, if I apply mvn rewrite:run in the submodule specifically - after the first unsuccessful run, it applies the recipe and I get some changes. But no luck when run sequentially from the root project.

I understand my case is maybe a bit specific and complex but I'm wondering if we could make progress solving this together as using a Vanilla version would be far better than maintaining my own fork to get things to work for us.

Not sure how we can make progress on this, if having a call explaining our situation could help, if you see something obvious that can't work or if you have pointers on how I could debug this thing and maybe provide more insights.

I'll try to make some progress on my side to understand why ChangeDependencyGroupIdAndArtifactId is suddenly broken by the addition of the model update.

gsmet commented 2 years ago

Part of the problem is due to: https://github.com/openrewrite/rewrite/pull/1585 .

Once this gets in, I will probably be able to make some progress on making AddDependency more suitable for us.

jkschneider commented 2 years ago

1585 merged

gsmet commented 2 years ago

@jkschneider thanks! I'll revisit my AddDependency issues now that this initial issue is out of the way. Probably next week.

tkvangorder commented 2 years ago

@gsmet If you want to schedule some time next week, we can get on zoom to discuss the issues you are having with the update model.

tkvangorder commented 2 years ago

Yeah, the existing AddDependency is meant to operate when more than one maven pom exists in the list of source files that are being operated on. The provenance information is used to pair up a pom file with a list of its associated Java files (and then we check to see if the type is in use.) Without something like this, we would end up adding the dependency to ALL pom files in a multi-module maven project.

Since you are running recipes, one maven project at a time, your approach totally makes sense.

Not sure if you will see this comment, so linking here:

https://github.com/gsmet/rewrite/commit/09cb534746ce331faea2ed57adb27bb450f3a24f#r71009299

gsmet commented 2 years ago

Hey @tkvangorder ,

Yeah I wasn't sure what would be needed to make it work so it's really very dirty as I didn't plan to keep it.

My plan is to try generalizing AddDependency and get back to you if I can make things work.

tkvangorder commented 2 years ago

I am going to close out this issue, if there is something actionable, we can create a new issue/pr.

Thanks for you input!