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

Java 1.11 Compatibility #29

Closed
drdrwhite opened this issue Jun 18, 2018 · 9 comments
Closed

Java 1.11 Compatibility #29

drdrwhite opened this issue Jun 18, 2018 · 9 comments
Assignees

Comments

@drdrwhite
Copy link
Collaborator

A few people have reported problems with future versions of the JDK and Gin. This could be a regression, but we should do some proper evaluation and work out what's going on.

@sandybrownlee sandybrownlee self-assigned this May 22, 2020
@sandybrownlee
Copy link
Collaborator

This might be of help. https://blog.codefx.org/java/java-9-migration-guide/

@sandybrownlee
Copy link
Collaborator

Actually, better to target Java 11 now...

@sandybrownlee sandybrownlee changed the title Java 1.10 Compatibility Java 1.11 Compatibility May 22, 2020
sandybrownlee added a commit that referenced this issue May 28, 2020
Also some minor fixes for other compiler warnings.
@sandybrownlee
Copy link
Collaborator

A lot of the problem seems to be down to the use of reflection and new security settings. It's not clear that there is a way to avoid reflection in this case (that major culprit is adding timeouts to junit tests - this needs access to java.lang.reflect.Executable in JUnitBridge.annotateTestWithTimeout())

You can see the warning if you build gin and run this:
java -cp build/gin.jar gin.util.DeleteEnumerator -d examples/unittests/ -m ./examples/unittests/mypackage/profiler_results.csv -c examples/unittests/

A workaround is using the JVM option --add-opens java.base/java.lang.reflect=ALL-UNNAMED; this works great as part of the gradle unit tests, and if the project is run using gradle run. Alas, we cannot build the jar so this option is always invoked. So for now, we will need to instruct users to add the above option when running Gin on any Java >9.

Like this:
java --add-opens java.base/java.lang.reflect=ALL-UNNAMED -cp build/gin.jar gin.util.DeleteEnumerator -d examples/unittests/ -m ./examples/unittests/mypackage/profiler_results.csv -c examples/unittests/

I've implemented some fixes to kill the warnings for the unit tests. I would appreciate if someone can try the origin/java11-debug branch and report two things:

  • does gin build successfully under java 11?
  • can you run the DeleteEnumerator command above and say whether it runs successfully (with and without --add-opens)

@justynapt justynapt self-assigned this Sep 1, 2020
@justynapt
Copy link
Collaborator

before upgrade release stable version with Java 8 and create a branch for future bug fixing with Java 8

@justynapt
Copy link
Collaborator

justynapt commented Jan 22, 2021

The plan is to upgrade to Java 14 at this point. Working version ready - the profiler - hprof no longer supportedm, so needs an upgrade. https://docs.oracle.com/en/java/javase/14/troubleshoot/diagnostic-tools.html#GUID-CBC97A20-7379-4762-BA17-FB1A560D02E4
Considering FligtRecorder

@justynapt
Copy link
Collaborator

let's stick to LTS releases

@GiovaniGuizzo
Copy link
Collaborator

I just pushed a new branch (https://github.com/gintool/gin/tree/no-reflection-junit5).

With this new code, the need for reflection is removed by simply adding a configuration parameter with a global timeout.

However, the new requirement is that test cases MUST be written with JUnit 5's annotations. The timeout configuration parameter only works with the JUnit 5's engine, not with JUnit 4's. JUnit 4's test cases must still be changed with reflection to include timeouts.

Another detail: when a test case has a @timeout annotation, it overwrites the configuration parameter set by me. It kind of makes sense, since the author of the test case might want to set a timeout for a slow test anyway.

@sandybrownlee
Copy link
Collaborator

PR #91 has updated the profiler to support Java Flight Recorder for Java versions >8.

Test timeout / reflection issue seems to be the only remaining upgrade needed for newer Java.

@sandybrownlee
Copy link
Collaborator

Woo hoo! Support for 17 added by 16b2b89

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

No branches or pull requests

4 participants