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

Update Maven Plugins #111

Merged
merged 3 commits into from
May 9, 2024
Merged

Update Maven Plugins #111

merged 3 commits into from
May 9, 2024

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented May 8, 2024

Using all the latest Maven plugins (base for 3.6.3).

Left out spotless and bnd 7.0.0 (raises requirement for 3.8.1)

Using all the latest Maven plugins (base for 3.6.3).

Left out spotless and bnd 7.0.0 (raises requirement for
3.8.1)
@cstamas cstamas requested review from mcculls and kwin May 8, 2024 09:17
@cstamas cstamas self-assigned this May 8, 2024
@@ -117,8 +117,8 @@
<properties>
<maven.compiler.release>8</maven.compiler.release>
<!-- Set to same version as release target for consistency -->
<maven.compiler.source>1.${maven.compiler.release}</maven.compiler.source>
<maven.compiler.target>1.${maven.compiler.release}</maven.compiler.target>
<maven.compiler.source>${maven.compiler.release}</maven.compiler.source>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can fully remove those properties, as we build with Java11+ only

Copy link
Member Author

@cstamas cstamas May 8, 2024

Choose a reason for hiding this comment

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

They were originally added as some tools and IDEs (IDEA) did not pick up maven.compiler.release, so just to "fully advertise" to all tools out there that project is 8. We can remove them, but there is no harm in them either.

@cstamas
Copy link
Member Author

cstamas commented May 8, 2024

The --add-opens made build green, but my problem is WHY it suddenly became needed?

@mbien
Copy link

mbien commented May 8, 2024

The --add-opens made build green, but my problem is WHY it suddenly became needed?

the dependency updates likely cause different code paths now. A recent build on master was green so we probably can exclude the environment as cause (e.g JDK update which changed method modifiers somewhere or moved functionality to a different module).

btw cglib is no longer maintained!

IMPORTANT NOTE: cglib is unmaintained and does not work well (or possibly at all?) in newer JDKs, particularly JDK17+. If you need to support newer JDKs, we will accept well-tested well-thought-out patches... but you'll probably have better luck migrating to something like ByteBuddy.

-> https://github.com/cglib/cglib

looking at the maven downloads from the CI log:

[INFO] --- dependency:3.6.1:copy (guice3) @ org.eclipse.sisu.inject ---
...
 [INFO] Downloading from central: https://repo.maven.apache.org/maven2/com/google/inject/guice/3.0/guice-3.0.jar
...

this is getting quite dusty. cglib was likely used by guice at some point.

https://github.com/eclipse/sisu.inject/blob/e52a773bf41307d9e253555bee1eab587adef8a3/org.eclipse.sisu.inject/src/main/java/org/eclipse/sisu/inject/Guice4.java#L24-L27

@cstamas
Copy link
Member Author

cstamas commented May 9, 2024

@mbien but the point is that there are no dependency updates, only plugin updates... This is what confuse me.
Edit: there IS one update: junit 5.10.0 to 5.10.2... but again WHY is suddenly add-open needed?

@cstamas
Copy link
Member Author

cstamas commented May 9, 2024

One suspect: surefire 3.2.2 (update went from 3.0.0 to 3.2.5) got this change:
apache/maven-surefire#682 update of plexus-java from 1.1.x to 1.2.x, that on other hand MAY cause that a dependency end up on module path instead of classpath...

@cstamas cstamas merged commit 23eec03 into master May 9, 2024
12 checks passed
@cstamas cstamas deleted the update-maven-plugins branch May 9, 2024 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants