Skip to content

Add Java 15 CI #21713

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

Closed
wants to merge 5 commits into from
Closed

Add Java 15 CI #21713

wants to merge 5 commits into from

Conversation

dreis2211
Copy link
Contributor

Hi,

this PR tackles #21604 as discussed. Fortunately, even the automatic JDK upgrade should already work.

Before merging this, #21605 needs to be merged. Apart from that I haven't seen any test failures when I run with the custom buildJavaHome property being set to my local JDK 15 installation.

Cheers,
Christoph

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 5, 2020
@snicoll snicoll self-assigned this Jun 5, 2020
@snicoll snicoll added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 5, 2020
@snicoll snicoll added this to the 2.4.0-M1 milestone Jun 5, 2020
@snicoll
Copy link
Member

snicoll commented Jun 5, 2020

Thanks for the PR @dreis2211

Apart from that I haven't seen any test failures when I run with the custom buildJavaHome property being set to my local JDK 15 installation.

This was required when Gradle does not support the target JDK which does not seem to be case with Java15 at this point. I don't see this PR using that feature either so I am a bit confused.

Switching my local JDK to Java 15ea and building master leads to some failures. Do you have time to look at those?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Jun 5, 2020
@dreis2211
Copy link
Contributor Author

@snicoll You're absolutely right. I was under the impression that I'm getting the same results with the custom home property being set.
Let me try with your approach.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 5, 2020
@dreis2211
Copy link
Contributor Author

@snicoll It would have also worked with the custom java home being set, but my tasks were cached and thus not all executed.

I see one additional failure in SampleAntApplicationIT.runJar + the already mentioned multi-release JAR test that is covered by #21605 .

Does that match your local build?

@snicoll
Copy link
Member

snicoll commented Jun 5, 2020

No, it does not. I have a compilation failure:

> Task :spring-boot-project:spring-boot-tools:spring-boot-loader:compileJava FAILED
/Users/snicoll/workspace/work/spring-boot/master/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/StringSequence.java:75: error: isEmpty() in StringSequence cannot implement isEmpty() in CharSequence
        boolean isEmpty() {

and one test failure before I gave up:

MappingsEndpointReactiveDocumentationTests > mappings() FAILED
    org.springframework.restdocs.payload.FieldTypesDoNotMatchException at MappingsEndpointReactiveDocumentationTests.java:126

This in a branch of your first PR I am about to merge. Maybe something is cached and shouldn't be?

$ java -version
openjdk version "15-ea" 2020-09-15
OpenJDK Runtime Environment (build 15-ea+26-1287)
OpenJDK 64-Bit Server VM (build 15-ea+26-1287, mixed mode, sharing)

@snicoll snicoll added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jun 5, 2020
@dreis2211
Copy link
Contributor Author

openjdk version "15-ea" 2020-09-15
OpenJDK Runtime Environment (build 15-ea+17-717)
OpenJDK 64-Bit Server VM (build 15-ea+17-717, mixed mode, sharing)

I guess the first one can be explained by CharSequence.isEmpty being only available in later builds. I'll try the latest JDK 15 build then.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 5, 2020
@snicoll snicoll added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jun 5, 2020
dreis2211 added 2 commits June 5, 2020 17:20
In JDK 15 the concept of hidden classes was introduced, which also affects
Lambdas in so far that Class.getCanonicalName() will return null for those.
Using Class.getName() works around that behavior change.

See spring-projectsgh-21604
JDK 15 introduces isEmpty() on CharSequence which clashes with the one declared
in StringSequence because it is not public.

See spring-projectsgh-21604
@dreis2211
Copy link
Contributor Author

I've pushed two additional commits that fix the syntax error and the failure in MappingsEndpointReactiveDocumentationTests

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 5, 2020
@dreis2211
Copy link
Contributor Author

dreis2211 commented Jun 5, 2020

I see some intermittent failures for tests that all seem to be related to timing. I wonder if https://bugs.openjdk.java.net/browse/JDK-8242504 is related as this got introduced recently as well.
EDIT: After checking the code, I don't think it is related as it should not affect my Mac.

@dreis2211
Copy link
Contributor Author

I've also seen some flaky tests that are annotated with @WebEndpointTest that prematurely close the connection.

@dreis2211
Copy link
Contributor Author

dreis2211 commented Jun 10, 2020

I just tried with JDK 14 and see similar flaky failures there as well. Particularly, the "performance" tests ConfigurationPropertySourcesTests seem to fail on JDK 14 on my machine, too. So I wonder if we might already be in the position to consider JDK 15 as done and "simply" have to improve flaky tests across the board.

@dreis2211
Copy link
Contributor Author

dreis2211 commented Jun 11, 2020

DevToolsIntegrationTests > createAControllerAndThenAddARequestMapping seems to also fail on JDK 8 occasionally already.

Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

I think we're almost done here but there is one change I'd like us to reconsider. I've added a comment

@@ -29,7 +29,7 @@
private final String className;

HandlerFunctionDescription(HandlerFunction<?> handlerFunction) {
this.className = handlerFunction.getClass().getCanonicalName();
this.className = handlerFunction.getClass().getName();
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should do this. Perhaps as a fallback if getCanonicalName is empty? It's a change of format so it's a breaking change.

Or perhaps we could adapt the test? I haven't looked in details yet so thinking out loud.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote to the core-libs mailing list a week ago, I think, and got this reply.

A hidden class has no canonical name [1] and it can't be nominally referenced [...] For diagnosiability, Class::getName or Class::toString can be used.

As HandlerFunction is often a lambda, it will probably often return null on JDK 15 at least. I don't have a problem with the fallback idea. In reality, there will be only a change for inner classes though. See the following example output from JDK 14 of canonical vs name:

com.example.demo.Example$$Lambda$14/0x0000000800b66c40
com.example.demo.Example$$Lambda$14/0x0000000800b66c40
com.example.demo.AFunction
com.example.demo.AFunction
com.example.demo.Example.InnerFunction
com.example.demo.Example$InnerFunction

Give me a minute to implement the fallback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dreis2211
Copy link
Contributor Author

Trying with 15-ea-27 introduced a change for missing Javadocs: https://bugs.openjdk.java.net/browse/JDK-8242607

That produces quite a lot of warnings like the following:

/Users/christoph.dreis/projects/spring-boot/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/docker/transport/DockerEngineException.java:35: warning: no comment
        private final String reasonPhrase;

It's not breaking the builds, so I guess it's fine for this PR, but probably needs to be tackled in another one.

@snicoll
Copy link
Member

snicoll commented Jun 15, 2020

Ah ah. I was building locally and I've already polished that checkstyle issue here Christoph.

@snicoll snicoll removed the status: feedback-provided Feedback has been provided label Jun 15, 2020
@dreis2211
Copy link
Contributor Author

Sorry - I've noticed this one just now. :/

snicoll pushed a commit that referenced this pull request Jun 15, 2020
In JDK 15 the concept of hidden classes was introduced, which also
affects Lambdas in so far that Class.getCanonicalName() will return null
for those. This commit uses Class.getName() as a fallback when no
canonical name is available.

See gh-21713
snicoll pushed a commit that referenced this pull request Jun 15, 2020
JDK 15 introduces isEmpty() on CharSequence which clashes with the one
declared in StringSequence because it is not public.

See gh-21713
snicoll pushed a commit that referenced this pull request Jun 15, 2020
snicoll added a commit that referenced this pull request Jun 15, 2020
@snicoll snicoll closed this in 75dd965 Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants