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

Upgrade cf-java-client to support Spring Boot 3.1.x and 3.2.x with Java 17 #1198

Closed
wants to merge 0 commits into from

Conversation

pacphi
Copy link
Contributor

@pacphi pacphi commented Oct 3, 2023

It's been 7 months since the 5.10.0.RELEASE. The current release documentation states that that release is compatible with Spring Boot release versions up to 2.6.x. Meanwhile, Spring Boot 3.1.x is the current release, with Spring Boot 3.2.x nearing GA.

There's not much that needed to change to prepare a version of the cf-java-client to be compatible with Spring Boot 3.1.x. I did, however, set the baseline for Java 17. (A decision that could of course be revisited). That required a change to the maven-compiler-plugin compileArgs configuration to facilitate source code generation and data-binding when using Immutables. I also took the test dependencies forward, so that in the future, the tests could be refactored to leverage JUnit 5 APIs. The rest of the changes are pretty straight-forward, just upgrades to dependencies and plugins to the latest available as of the date of this pull request.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 3, 2023

CLA Not Signed

@pacphi
Copy link
Contributor Author

pacphi commented Oct 4, 2023

Built with

❯ mvn clean install -DskipTests -Dgpg.skip

Then

❯ mvn test

...
[ERROR] Tests run: 636, Failures: 36, Errors: 0, Skipped: 2
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Cloud Foundry Java Client Parent 6.0.0.BUILD-SNAPSHOT:
[INFO] 
[INFO] Cloud Foundry Java Client Parent ................... SUCCESS [  0.309 s]
[INFO] Cloud Foundry Java Client .......................... SUCCESS [ 23.485 s]
[INFO] Cloud Foundry Java Utilities ....................... SUCCESS [ 15.961 s]
[INFO] Cloud Foundry Java Client - Reactor Implementation . SUCCESS [ 19.845 s]
[INFO] Cloud Foundry Java Operations ...................... FAILURE [ 16.091 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:15 min
[INFO] Finished at: 2023-10-03T19:04:35-07:00
[INFO] ------------------------------------------------------------------------

Then

❯ cd cloudfoundry-operations
❯ mvn test

See output, here:

test-build.log

Need to figure out why ~5% of the total number of tests are failing.

@anthonydahanne
Copy link
Contributor

anthonydahanne commented Oct 5, 2023

Hello @pacphi !
Thanks for your PR!
There's something that I wonder though: why do we need the upgrade to Spring Boot 3 / Java 17?
java-cf-client does not depend on spring libraries... I mean sure cloudfoundry-client-reactor depends on some utilities from spring, but it does not depend on Spring APIs.
Which leads me to think that: sure we can upgrade the maven wrapper, the unit tests to use Junit5 - but mandating users to run on Java 17 when they could simply run on Java 8; for no tangibles advantages...
I'd prefer this PR to just upgrade all the dependencies (including the Spring ones that will be supported until 2027, see https://spring.io/projects/spring-framework#support)


Update 1hour later
Do you have an example of a Spring Boot 3 application for example that cannot include java-cf-client ? If so could you just upgrade some dependencies to make them compatible with Java 17?
Thanks!

@pacphi
Copy link
Contributor Author

pacphi commented Oct 6, 2023

@anthonydahanne First, thanks for considering inclusion of this PR. To answer your questions and concerns.

If you continue to employ Spring Framework dependencies in the cf-java-client libraries, then when you upgrade dependencies to 6.0, that version's source code baselines support on Java 17. See What's New in Spring Framework 6.0.

And Spring Boot 3.1.x depends on Spring Framework 6, see https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-3.1-Release-Notes#dependency-upgrades.

So, yeah. We could strip away Spring Boot 3.1, and then you'd want to strip the convenient dependencies management which manages version for all transitives.

I went diving on dependencies, and I see spring-core and spring-web as dependencies in client, client-reactor, and util libs. And deeper I saw that the only two classes employed from Spring are:

  • org.springframework.web.util.UriComponents
  • org.springframework.web.util.UriComponentsBuilder

Why not remove Spring dependencies entirely? You're just using it for convenience.


Update: I realize my last statement is easier said than done.


Update 2: To your point on no tangible advantages. We're addressing CVEs in transitives.

@pacphi
Copy link
Contributor Author

pacphi commented Oct 6, 2023

Is it of utmost importance to maintain backward target compatibility with Java 8?

I realize extended support is available for that release until 2030. (Java 8 was first released in March 2014). But before that future, on current LTS release cadence, Java 25 will be out in September 2025.

@anthonydahanne
Copy link
Contributor

@pacphi

I went diving on dependencies, and I see spring-core and spring-web as dependencies in client, client-reactor, and util libs. And deeper I saw that the only two classes employed from Spring are:

org.springframework.web.util.UriComponents
org.springframework.web.util.UriComponentsBuilder

Ah, I had a very quick look and came to the same conclusion!so weird to import 2 big libraries just for 2 classes; but I guess there was history.

Why not remove Spring dependencies entirely? You're just using it for convenience.

Indeed!

Update 2: To your point on no tangible advantages. We're addressing CVEs in transitives.

That's true; maybe it's possible to keep Spring Framework 5 BOM without importing any Spring Libs?

Is it of utmost importance to maintain backward target compatibility with Java 8?
I realize extended support is available for that release until 2030. (Java 8 was first released in March 2014). But before that future, on current LTS release cadence, Java 25 will be out in September 2025.

Personally, I always prefer to use Java latest; but being new to this codebase (and to its users) I don't feel comfortable to impose an upgrade from Java 8 to Java 17 (9 versions); I imagine there are still some people on Java 8 out there.

Then we could branch, like your major upgrade suggested; but maintaining 2 branches is something we'd prefer to avoid for now.

Thanks for your understanding; let me know what you think!

@pacphi
Copy link
Contributor Author

pacphi commented Oct 6, 2023

I don't feel comfortable to impose an upgrade from Java 8 to Java 17 (9 versions)

It's really only 2 LTS jumps. But I get it... to allow for the widest possible adoption and consumption we could stay pinned to Java 8.

Then we could branch, like your major upgrade suggested; but maintaining 2 branches is something we'd prefer to avoid for now.

If you don't like maintaining branches, perhaps prep building and packaging thru Maven profiles (and properties) to support Java 8 and Java17+ variants of the cf-java-client libraries? For the latter, just activate a Maven profile during build, that overrides properties, and produces artifact ids appended with -jdk17 or -jdk21? When publishing and releasing, you could then decide what versions of the JDK/JRE to support.

I have completed a proof-of-concept of this recommendation. See later commits. And you can take a surface-level look at the recently added Github Action workflow runs for each version of Java here.

My fork still suffers from some test failures in versions built with Java 17+. But for Java 8 and Java 11, tests do pass. (I could adapt workflow to run unit tests for each variant).

@pacphi
Copy link
Contributor Author

pacphi commented Oct 6, 2023

Adapted workflow run (that executes tests for each child module) results here.

@pacphi
Copy link
Contributor Author

pacphi commented Oct 6, 2023

I believe we are facing this issue when employing Mockito with JDK 17+. Not sure how to proceed.

pom.xml Outdated
Copy link

Choose a reason for hiding this comment

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

Please update wire-runtime.

Copy link
Contributor Author

@pacphi pacphi Oct 8, 2023

Choose a reason for hiding this comment

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

Please update wire-runtime.

I have tried upgrading to later versions of wire-maven-plugin, wire-runtime, and/or wire-runtime-jvm with Java 8. My attempts have resulted in compilation or test failures. Leaving versions "as-was", compilation and tests pass.

That said, still trying to get tests with Java 17+ to pass.

Copy link
Contributor Author

@pacphi pacphi Oct 8, 2023

Choose a reason for hiding this comment

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

I successfully upgraded wire-runtime and wire-maven-plugin to 3.7.1 and 3.0.2 respectively for Java 8. Unfortunately, adding any 4.x version fails with and/without the addition of wire-runtime-jvm.

@christopherclark
Copy link
Member

@pacphi If you're having issues with EasyCLA, you can open a ticket with LF IT at the link on the check above, and someone should get you sorted out within a day or so. You could also try other link, select VMware, Inc. from the list of companies, and then retrigger the CLA check by commenting "/easycla" in this PR.

However, It looks like you're already approved under the VMware CLA, so I'm guessing those commits might be associated with an alternative email address? That happens sometimes. GH docs on that here: https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-email-preferences/setting-your-commit-email-address

Regardless, opening a ticket should get this fixed quick.

@DerrickLFX
Copy link

/easycla

@pacphi pacphi closed this Oct 13, 2023
@pacphi
Copy link
Contributor Author

pacphi commented Oct 13, 2023

My Git-fu is not strong enough.

What I'm going to do...

  • Keep local copy of my source with updates
  • Destroy the fork on Github
  • Re-fork this project
  • Get updates in on a branch off main
  • Then file a new PR

I'm going to close this PR.

pacphi added a commit to pacphi/cf-java-client that referenced this pull request Oct 13, 2023
It's been 7 months since the 5.10.0.RELEASE. The current release documentation states that that release is compatible with Spring Boot release versions up to 2.6.x. Meanwhile, Spring Boot 3.1.x is the current release, with Spring Boot 3.2.x nearing GA.

There's not much that needed to change to prepare a version of the cf-java-client to be compatible with Spring Boot 3.1.x, but it does require setting target to Java 17. That required a change to the maven-compiler-plugin compileArgs configuration to facilitate source code generation and data-binding when using Immutables. I also took the test dependencies forward, so that in the future, the tests could be refactored to leverage JUnit 5 APIs. The rest of the changes are pretty straight-forward, just upgrades to dependencies and plugins to the latest available as of the date of this pull request.

Default build with Maven 3.9.4 targets Java 8 and upgrades Spring Boot to 2.7.16.

Maven profiles get activated based on the installed version of the JDK detected.  These profiles update the dependencies and plugins to support compilation and testing on Java 17 and 21 with Spring Boot 3.1.4 dependency management.

A Github Action workflow was added to provide signal on "good builds".

Currently, tests fail when targeting either Java 17 or 21.  See this open issue for more details: immutables/immutables#1339.

> This PR was based upon earlier work in cloudfoundry#1198.
@anthonydahanne
Copy link
Contributor

replaced with: #1201

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.

5 participants