-
Notifications
You must be signed in to change notification settings - Fork 41.2k
Allow the project to be built with Java 16 #25171
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
Conversation
@@ -31,10 +31,6 @@ rootProject.name="spring-boot-build" | |||
settings.gradle.projectsLoaded { | |||
gradleEnterprise { | |||
buildScan { | |||
if (settings.gradle.rootProject.hasProperty('buildJavaHome')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea/hope is that the build scans show the actual toolchain, but we'll see. Couldn't test this really.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a scan against 2.4.x
with -PtoolchainVersion=16
: https://ge.spring.io/s/yadj75gujofi6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why 2.4.x is targeted here? I guess we would need Groovy 3 as well in the 2.4.x mainline to make this work. See #24946
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Support for Java 16 starts with Spring Framework 5.3 which is what Spring Boot 2.4+ uses. We need a GA release of Spring Boot that folks using Java 15 could rely on once 16 is out (given that 15 will be effectively EOL at that time).
As for Groovy 3, perhaps we could upgrade to it on the Java 16 build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would need a little help on getting this to work on 2.4.x - I currently don't see a good way to do this sort of conditional dependency upgrade
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this would only happen for the actual pipelines e.g. via -x :spring-boot-project:spring-boot-cli:test
. I wouldn't know what I should update in this PR - cluttering every CLI test with @DisabledForJreRange
seems like an overkill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather find a way to disable tests for the CLI module altogether with Java 16. When we had a Maven build we used profiles to disable Surefire. I guess Gradle should have a similar feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you know one that is also aware of the toolchain version, that would be great. We could also use that for the gradle plugin then. I just don't know how we could exclude a whole module based on the toolchain version - apart from coming up with our own way. Let me know if I should experiment around that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll have to come up with our own way. Alternatively, we could use @DisabledForJreRange
all over the place. One advantage of that is the tests are disabled no matter how they're launched.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm currently writing a custom plugin that lets you specify the maximum compatible java version inside the build.gradle for that particular submodule. The plan is that this information is consumed in order to dynamically disable the things that run with the optional toolchain.
Thanks very much, @dreis2211. Here's a build scan for the proposed changes: https://ge.spring.io/s/ovgtv4h7lapkc. The failures in the CLI appear to be due to Groovy not supported Java 16 byte code. The
Support for Java 16 has landed in ASM but it has not yet been released. Framework is avoiding this problem by patching its own version of ASM to pull in the latest changes prior to them being released. The ASM release with Java 16 is expected to be "at the same time the JDK 16 will be released" so we may have to live with this for a while by skipping those tests on Java 16. |
@wilkinsona Thanks. I'm wondering why the test you mentioned didn't fail on my machine. But seeing no indication of the toolchain, I think I will bring back a custom value. |
Things seem to have happened more quickly on the ASM side than the ticket linked to above suggested. According to https://asm.ow2.io/versions.html, ASM 9.0 – that includes Java 16 support – was released in September. |
I couldn't see any indication either. I raised it with the Gradle team and they confirmed it's not yet surfaced in the builds scans. They suggested using custom values for now. |
Me too. Turns out there's a bug in |
@wilkinsona Thanks for checking that. I indeed did some I added the custom toolchain value to the scans. Let me know if you miss anything on this PR still. |
@philwebb I saw you changed the title, but I'm personally not comfortable with saying "Support Java 16" just yet. The issues over at spring-projects/spring-ldap#570 and the Gradle Plugin not being supported at all maybe gives the wrong impression to users that read the changelog and see that. In the end, it's more "Support Java 16 builds" and one step further into supporting Java 16. |
We're happy with stating that Spring Boot itself supports Java 16, even if that's not true across the board. For example, our Java 9 support excluded Cassandra for several months. If necessary, we can do the same with Spring LDAP and Gradle. The latter's really not too bad as users can still use Java 16 at runtime as long as they run Gradle itself on an earlier JVM. |
Fair enough... |
@wilkinsona @snicoll I've pushed a revised version that extracts the toolchain configuration into its own plugin/extension. That felt cleaner overall, but let me know what you think of it. The custom plugin + extension has the possibility to set a maximum compatible java version and disables the toolchain based tasks if it doesn't match (see the build.gradle of spring-boot-cli for an example) The positive side-effect of this is that there is no need for custom task exclusions via |
This is great. Thanks very much, @dreis2211. I particularly like the new approach to configuring the toolchain stuff via the plugin and extension. |
Here is a scan for a 2.4.x build using a Java 16 toolchain. |
Hi,
this PR gets us one step further into supporting JDK 16 (see #24402).
I'm at a stage where I have random failures on my machine that seem related to network errors & timeouts, so I'm both confident and desperate enough to share my first draft.
Essentially, the PR replaces the functionality of
buildJavaHome
with a new propertytoolchainVersion
that controls whether or not an optional toolchain should be configured for compilation, javadoc and test runs.I should note that the
spring-boot-gradle-plugin
has multiple problems. None of the current versions support JDK 16 and thus a lot of tests fail. Unfortunately, you can't provide an empty stream inGradleCompatibilityExtension.provideTestTemplateInvocationContexts
. I had some hacky code that would workaround that but I figured that a simple exclude would suffice for the time being. E.g. I used the following:As you can see, I excluded the
validatePlugins
task as well, which is failing due to gradle/gradle#15538.The big issue is something else though. With JDK 16 and more specifically JEP-396 we have strong encapsulation enabled by default. That breaks a lot of tests and libraries. Just to name a few:
AbstractServletWebServerFactoryTests
due to theURL.factory
field being reset.CliTester
for a similar reasonJsonbTesterTests
due to a bug in JohnzonI decided to add
--illegal-access=warn
so we get warned, but at the same time have access allowed for the time being.Feel free to test around with this PR. Again - I didn't have a single test run today that was green, but all test failures were random and successful when run in isolation.
Next steps
If this PR is merged eventually, I would tackle the actual pipeline. I thought this might be a bit much for this PR, given that I need to add a secondary JDK to the CI images. (I can hopefully reuse some logic though that I build in the past for Boot already)
Let me know what you think.
Christoph