Update Java FSST tests #666
Conversation
- Allow FSST tests to run without the native JNI module - Fix the JNI module build
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #666 +/- ##
=======================================
Coverage 74.58% 74.58%
=======================================
Files 66 66
Lines 3286 3286
Branches 1461 1461
=======================================
Hits 2451 2451
Misses 738 738
Partials 97 97 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the build system for the native FSST wrapper to improve cross-platform compatibility and enables previously disabled FSST tests. The changes update CMake configurations, build scripts, and add conditional loading of the JNI wrapper to gracefully handle missing native libraries.
- Updated build scripts to use corrected directory paths and standardized CMake arguments
- Modified Java code to conditionally load JNI wrapper and skip JNI tests when the library isn't available
- Re-enabled previously disabled FSST tests
- Updated package name in JNI function signatures from
com.mlttoorg.maplibre.mlt
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| java/resources/compile-windows.bat | Updated build directory paths and CMake command syntax |
| java/resources/compile | Updated build directory paths for Unix-like systems |
| java/resources/FsstWrapper.cpp | Updated JNI function name to match new package structure |
| java/resources/CMakeLists.txt | Added CMAKE_POLICY_VERSION_MINIMUM parameter to external project |
| java/mlt-core/src/test/java/org/maplibre/mlt/converter/encodings/fsst/FsstTest.java | Re-enabled tests, added conditional JNI loading, and fixed test data path |
| java/mlt-core/src/main/java/org/maplibre/mlt/converter/encodings/fsst/FsstJni.java | Added isLoaded tracking and updated module path |
| java/mlt-core/src/main/java/org/maplibre/mlt/converter/encodings/fsst/FsstDebug.java | Added conditional JNI loading check |
| java/mlt-core/build.gradle | Refactored compileWrapper task and added dependency from tests |
Comments suppressed due to low confidence (1)
java/resources/FsstWrapper.cpp:108
- The FindClass call uses the old package name 'com/mlt' but the JNI function signature was updated to 'org/maplibre/mlt'. This mismatch will cause a ClassNotFoundException at runtime. Change to 'org/maplibre/mlt/converter/encodings/fsst/SymbolTable'.
jclass symbolTableClass = env->FindClass("com/mlt/converter/encodings/fsst/SymbolTable");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
# Conflicts: # java/mlt-core/src/main/java/org/maplibre/mlt/converter/encodings/fsst/FsstEncoder.java # java/mlt-core/src/main/java/org/maplibre/mlt/converter/encodings/fsst/FsstJni.java
|
Java 25 is installed on the runners (not Java 22). https://github.com/actions/runner-images/blob/main/images/macos/macos-14-arm64-Readme.md#macos-14 https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2404-Readme.md A Windows build is not yet included in |
|
Should I update it to 25 then? Seems like it should b as low as it can be for widest compatibility. But I would have thought 25 could build a project set to 22. The code only uses the native implementation if it's present, so it should be fine that it's not available on all platforms. |
|
Cool, looks like the FSST tests passed for macOS. |
|
So I guess the issue is with the mlt-cli Gradle build? |
3 participants
Fix the JNI module build