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

maven.compiler.release set in parent is not recognized in child module #523

Closed
pzygielo opened this issue Aug 1, 2024 · 6 comments · Fixed by #549
Closed

maven.compiler.release set in parent is not recognized in child module #523

pzygielo opened this issue Aug 1, 2024 · 6 comments · Fixed by #549
Labels
bug Something isn't working

Comments

@pzygielo
Copy link

pzygielo commented Aug 1, 2024

maven.compiler.release which is defined in parent is not recognized in child module and patch for explicit overriding property is generated by UpgradeToJava17 recipe.

What version of OpenRewrite are you using?

I am using

  • rewrite-migrate-java 2.21.0
  • Maven plugin 5.37.1

How are you running OpenRewrite?

I am using the Maven plugin, and my project is a multi-module project.

What is the smallest, simplest way to reproduce the problem?

https://github.com/pzrep/openrewrite-migrate-compiler-release-in-child/

What did you expect to see?

No patch generated by recipe as property inherited from parent is good enough.

What did you see instead?

Patch generated by recipe (step: Show applied changes): https://github.com/pzrep/openrewrite-migrate-compiler-release-in-child/actions/runs/10196871940/job/28208511361?pr=2

diff --git a/child/pom.xml b/child/pom.xml
index 96186f3..2570020 100644
--- a/child/pom.xml
+++ b/child/pom.xml
@@ -8,5 +8,8 @@
         <version>1-SNAPSHOT</version>
     </parent>
     <artifactId>openrewrite-migrate-compiler-release-in-child.child</artifactId>
+    <properties>
+        <maven.compiler.release>17</maven.compiler.release>
+    </properties>

8d15935#commitcomment-144892557

@pzygielo pzygielo added the bug Something isn't working label Aug 1, 2024
@timtebeek timtebeek moved this to Backlog in OpenRewrite Aug 1, 2024
pzygielo referenced this issue Aug 1, 2024
…upgrade.

Before these changes if no property or maven compiler plugin configured java version no change would be made.

Fixes #514
@pzygielo
Copy link
Author

pzygielo commented Aug 1, 2024

Additional info: I observe, that m.c.r property is added even to pom.xml in aggregator project (of <packaging>pom</packaging>), where no compilation happens.

@timtebeek
Copy link
Contributor

Thanks for the reproducer @pzygielo ! Next steps would be to turn this into a runnable unit test and see that it's resolved.

For the aggregator poms I'm not sure how you'd handle those separately, as we would like to set the property in parent poms, not each submodule. How would you recognize those cases?

/cc @sambsnyd for visibility

@pzygielo
Copy link
Author

pzygielo commented Aug 2, 2024

For the aggregator poms I'm not sure how you'd handle those separately, as we would like to set the property in parent poms, not each submodule.

That's what I'd like to see as well. My reference to modified aggregator in comment above was probably unnecessary, as in my case this aggregator is also a parent (which does not need to be the case in general - but I can't tell about plain aggregator yet).

@DidierLoiseau
Copy link
Contributor

I must say this is quite annoying because it adds it everywhere when your microservices & libraries inherit the spring-boot-starter-parent pom and don’t redeclare it.

I haven’t checked, but wouldn’t it also override a higher java version specified in a parent?

Would you think it is some kind of regression or has it always been like this?

@pzygielo
Copy link
Author

Would you think it is some kind of regression or has it always been like this?

I have not seen this in 2.20.0.

@DidierLoiseau
Copy link
Contributor

DidierLoiseau commented Sep 11, 2024

I confirm it was introduced in 2.21.0 – maybe a regression from commit 8d15935?

I also confirm it downgrades java if a higher version is specified in the parent pom.

Edit: created PR #549 adding a test case to reproduce this issue.

DidierLoiseau pushed a commit to DidierLoiseau/rewrite-migrate-java that referenced this issue Sep 11, 2024
DidierLoiseau added a commit to DidierLoiseau/rewrite-migrate-java that referenced this issue Sep 11, 2024
DidierLoiseau added a commit to DidierLoiseau/rewrite-migrate-java that referenced this issue Sep 17, 2024
DidierLoiseau added a commit to DidierLoiseau/rewrite-migrate-java that referenced this issue Sep 18, 2024
DidierLoiseau added a commit to DidierLoiseau/rewrite-migrate-java that referenced this issue Sep 18, 2024
…arent, but smartly

- only override numeric values
- if any property is detected, don't add maven.compiler.release
DidierLoiseau added a commit to DidierLoiseau/rewrite-migrate-java that referenced this issue Sep 18, 2024
…arent, but smartly

- only override numeric values
- if any property is detected, don't add maven.compiler.release
DidierLoiseau added a commit to DidierLoiseau/rewrite-migrate-java that referenced this issue Sep 20, 2024
DidierLoiseau added a commit to DidierLoiseau/rewrite-migrate-java that referenced this issue Sep 20, 2024
…arent, but smartly

- only override numeric values
- if any property is detected, don't add maven.compiler.release
@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenRewrite Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants