-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[GEOT-6964] Java 17 compatibility #3612
Conversation
The two remaining test case failures are very strange. They involve the function Home grown code attempts to find the date pattern of Change 30b8fa1 works for me locally but doesn't feel like a reliable fix. |
Not sure why I didn't see the test failure for module locally but now I do and the failure is:
Added commit to attempt upgrading Mockito. |
Upgrading Mockito seems effective but now a couple of test failures pop up in Cartographic CSS parser.. strange. |
Green Linux JDK 8, 11 and 17 GitHub CI 🎉 |
Hi @bjornharrtell . |
Yep, otherwise we'll get dates formatted in the default locale of the system... |
...-multidim/netcdf/src/main/java/org/geotools/imageio/netcdf/utilities/NetCDFCRSUtilities.java
Show resolved
Hide resolved
The usual issue is with the time zone, never seen a Locale issue in our date handling code before. Does the locale affect the TZ at all? |
@@ -1046,7 +1046,7 @@ public static Format getAxisFormat(final AxisType type, final String prototype) | |||
// TODO: Improve me: | |||
// Handle timeZone |
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.
this line is what would worry me!
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.
sure but that's been there forever
Seems to be working with Locale.ENGLISH but I'm a bit scared over potential regressions with stuff we are not covering with tests. |
The Downstream integration build fails due to that GeoServer also have a test using reflection to override a private final static field. Pretty nasty in my opinion and should use the new public API I introduce here. But not sure how to make that happen unless merging this and adapt GeoServer ASAP. |
@bjornharrtell just prepare twin PRs on the two projects, that build fine on your machine. Someone will confirm they do and then we can merge them together. Btw, this is one example of things I was expecting to break solid in Java 17... and they don't? |
Agree, that is suspect.. could it be that we somehow do not cover it in tests? |
@bjornharrtell maybe it's because we're telling the compiler to target Java 8? |
@aaime some more thought on this... it's reflection so it make sense that it doesn't cause compilation issues, however I would expect runtime issues covered by tests when run with the Java 17 JRE. Perhaps this will become more clear when I can invest som more time into the corresponding GeoServer issue with some more real life runtime experience. |
Maybe I can add some details here... At runtime you will get Warnings such as reported in GEOT-6967 (in this case with java 11 runtime started with https://github.com/locationtech/udig-platform) for ImageWorker. In our scenario we havent't seen any other issues but as @aaime stated: Only code thats loaded at runtime logs warnings. And we definitely do not use everything from GeoTools. So to get feedback for the intergation tests, its required to compile and test with at least maven.target.level=11 |
Ah, found it, Maven was still using java 11 because I overrode the PATH, but not JAVA_HOME.
We need to get that writer spi instance, just allowing free SPI usage is to be avoided, However, it should be possible to get to the writer in other ways, for example, looping over the results of |
@aaime ok still find it strange that it is not seen with Java 17 in JAVA_HOME but source/target left as is. |
@aaime revisiting this, I still don't understand why it is/would be needed to compile to target Java 11 to get the runtime problems. And in either case we want to target Java 8 to be able to do this in not too far future. |
@bjornharrtell I'm just as clueless as you are... internet searches are giving me no hints either. Maybe ask on stackoverflow? |
@aaime I think I found that it's |
I suppose |
For reference, the obsoletion of |
Finished for ImageWriter for 24862d9. But then there are test failures for the other way round in imagemosaic. And I find that imagemosaic properties point directly to internal Spi classes.. that seems a bit unfortunate (fx Line 13 in 06d379a
|
Found that I could use |
A blocker to complete this might be that we need to wait for a newer release of easymock. (easymock/easymock#274) Update: Might be working after all, that issue is probably not about general usage. |
I've reworked the SPI stuff to be a less intrusive change. |
* Add GitHub action to verify build with Java 17 (early access) * Rework to avoid use of reflection to override static final in testcase * Avoid SimpleDateFormat ParseException by not using Locale.CANADA * Handle XML prettify test result comparison issue * Upgrade Mockito
Downstream integration build fails because it also uses an illegal trick to override a static final field in testing. It needs to adapt to use the new public API introduced here, done with geoserver/geoserver#5319. |
I actually think this is complete now but needs local verification and coordinated merge with geoserver. @aaime and/or @dromagnoli are you interested in doing that? |
Wow great job, all tests with GeoTools passing! The downstream issue might indicate a change in behavior vs image reader choice (just guessing, I'm currently in bed with fever, writing from my phone) |
Oh ok, now saw the other comments about the downstream build. |
Did a quick pass... I have a hesitation about all the "add opens" added for tests. Maybe they are all for test code that does reflection things, but at the same time, don't they cover any other java 17 runtime issue? Tests are the only place where the code is actually run.... |
@aaime yes, ideally tests should not require those add opens to verify other potential runtime issues with illegal reflection. But I think the amount of work to refactor them into something that can work without that is very large/difficult. |
I'm smelling a trap here! But I also so much want to stick my nose into it! Found this actual issue... JAI cannot work on Java 17 without an export:
Long term I guess we can migrate to ImageN, and fix the direct reference to BytePackedRaster there... but I'm guessing it's just going to be one of many others. So that one justifies the export. About the opens, two are needed for EasyMock, one for Parboiled, one I guess for an old version of EHCache we're using in s3-geotiff. Pushed a commit that makes these open a bit more surgical, and comments on the origin of the need. |
@aaime, makes sense and also makes it a bit more clear what might be long term goals. |
@aaime, I'm actually right now running GeoServer through IntelliJ IDEA and the org.geoserver.web.Start entry point with Java 17 + local GeoTools compilation pointing at the release configuration and things actually seem to work including WMS, WFS 2.0.0 and raster mosiac sources. |
@@ -129,7 +129,7 @@ | |||
public static final String CONVERT_AXIS_KM_KEY = | |||
"org.geotools.coverage.io.netcdf.convertAxis.km"; | |||
|
|||
private static final boolean CONVERT_AXIS_KM; | |||
private static boolean convertAxisKm; |
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.
Just thinking out loud here... if the name is left as CONVERT_AXIS_KM then the existing code in GeoServer would not need to be changed right? This would allow starting to merge the GeoTools part without having to go and also have a GeoServer PR ready (the GeoServer PR for Java 17 would still use the new setter).
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.
Not sure that would work because the removal of final
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.
Don't have to finish/merge the full GeoServer Java 17 thing, could just merge this specific one in coordination - geoserver/geoserver#5319.
Re-build everything with Java 8, indeed both builds pass. Merging. Thanks! |
Checklist
main
branch (backports managed later; ignore for branch specific issues).For core and extension modules:
[GEOT-XYZW] Title of the Jira ticket
.