-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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] Use maven profile to target Java 8 bytecode / SDK #6764
Conversation
I think this is "the way" to ensure bytecode and SDK compatibility. However, I'm not sure how I can truly verify without a new release. |
and what part of this requires a 2.0.3 given that there are no code changes? |
@aardappel the problem is that 2.0.2 is not fully compatible with Java 8 due to the compilation settings in the maven pom in combination with compilation by a newer Java. By using "release" instead of "source" and "target" it should be. A bit deeper explained in jetty/jetty.project#3244. |
It is hard to tell if this will be enough if we don't write a reproducible unit test that will fail in 2.0.2 that will pass with this MR. It looks ok otherwise, but we released minor versions before to address this issue and it didn't solve it. |
@paulovap agreed but not straight forward how to put together such a unit test, but I suppose it should be possible using a special purpose dockerfile and github actions - I'll see what I can do. |
@paulovap while I can run JavaTest.sh locally and I've also been able to run it containerized using the Dockerfile.testing.java.openjdk_11_0_1, I realize now it doesn't help much as it's not using maven to compile or run the tests. |
@paulovap I've managed to reproduce the issue locally and verify that this fix works. I did this by modding the JavaTest.java to be a JUnit test, add stuff to pom.xml to be able to execute it with a specific JRE and point it to Java 8 on my system. However this feels entirely hacky and ruins JavaTest.sh so I'm not sure what more I can do at this point. |
I think if you've verified it locally, then that is acceptable at this point. |
If it works, we can merge it and see what happens. Are we also merging #6729? |
Ok, both merged, so I am pushing out a 2.0.3 now? |
With this change you only include the bundle plugin (and others) if it is java 9 or later. This causes the maven-dependency plugin to fail under java 8, because the "bundle" isn't known unless the plugin is defined. One solution is to move the bundle plugin out of the java 9 profile, and directly into the build section. How is this working for anyone using java 8 for flat buffers 2.0.3 forward, or was that the point of this. But reading through this thread, it looks like the point was to regain java 8 compatibility. Also no matter what I do to try and add the plugin, from my own project, I still get the unknown bundle error: Failed to execute goal org.apache.maven.plugins:maven-dependency-plugin:3.1.2:copy-dependencies (copy-dependencies) on project icore-release: Some problems were encountered while processing the POMs: [ERROR] [ERROR] Unknown packaging: bundle @ line 7, column 14 |
Aims to resolve #6763