fix(conf): auto-detect native libs in jlink runtime#6493
fix(conf): auto-detect native libs in jlink runtime#6493bluestreak01 merged 4 commits intomasterfrom
Conversation
Improvements for native library loading: - Auto-detect native libraries in jlink-ed runtime images - Uses java.home to locate a directory with native libs - Only activates when running from jrt: protocol (jlink runtime) - Eliminates the need for -Dquestdb.libs.dir in a start script
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdded a new public method Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai dear 🐰 , please review this little piggie. |
|
@jerrinot, I'll be happy to review this PR! 🐰 Let me take a look at the changes for auto-detecting native libraries in jlink runtime images. ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
core/src/main/java/io/questdb/std/Os.java (3)
276-308: Add Javadoc and extract property name constant for public API clarityThe flow is correct and robust—it prefers explicit
questdb.libs.dirwhen set, only attempts$java.home/lib/<libName>in jrt-based runtimes, and cleanly returnsnullto trigger fallback extraction. Two improvements would enhance maintainability:
Add Javadoc – The public method lacks documentation explaining when it returns non-null, that
nulltriggers embedded JAR libs fallback, and thatlibNameis only used for auto-detection.Extract property constant – The hard-coded
"questdb.libs.dir"string appears in three places (Os.java line 279, questdb.sh line 282, docker-entrypoint.sh line 12) and must stay synchronized. Defineprivate static final String LIBS_DIR_PROP = "questdb.libs.dir";to reduce drift risk.
266-274: Consider clarifying the method's actual scope:isJlinkRuntime()detects modular jrt: image runtimes in general, not exclusively custom jlink distributions. Stock JDK 9+ also uses thejrt:protocol.Renaming to
isJrtImageRuntime()or expanding the method comment to clarify this detects any modular image (stock JDK or custom jlink) would improve clarity. The implementation is safe—multiple validation checks ensure graceful fallback if assumptions don't hold—but the current name may mislead maintainers about its actual scope.
310-326:tryLoadFromDistributionbehavior and partial‑load scenariosThe distribution loading helper is generally solid:
- Respects the new
getNativeLibsDircontract (returns early whennull).- Uses absolute paths via
Paths.get(libsDir, ...)for both C++ and Rust libs.- On any failure, logs once to
System.errand returnsfalse, letting the existing JAR‑based fallback run.One subtle edge case to be aware of: if the C++ lib loads successfully from the distribution directory but the Rust lib fails, you'll:
- Have C++ already loaded from the distribution path.
- Fall back and attempt to load the same C++ library again from a temp file extracted from the JAR.
Per the JDK's
Runtime.loadLibrarysemantics, repeated loads of the same native library name in the same classloader are ignored, so this should be harmless in practice. Still, behavior here is a little non‑obvious.If you want to tighten this up, one option would be:
- First verify that both expected library files exist under
libsDir(for the jlink case you already check the C++ lib ingetNativeLibsDir; you could extend that check or add a parallel existence check for the Rust lib here) before callingSystem.loadat all. That would avoid "partially loaded from distribution, then falling back" scenarios.Not required for correctness, but would make failure modes and logging slightly cleaner.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/java/io/questdb/std/Os.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (36)
- GitHub Check: New pull request (Hosted Running tests on windows-other-2)
- GitHub Check: New pull request (Hosted Running tests on windows-other-1)
- GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo-2)
- GitHub Check: New pull request (Hosted Running tests on windows-cairo-1)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz2)
- GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-other)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-pgwire)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin-sub)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-sub)
- GitHub Check: New pull request (Hosted Running tests on windows-griffin-base)
- GitHub Check: New pull request (Hosted Running tests on mac-other)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-root)
- GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz2)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz1)
- GitHub Check: New pull request (Hosted Running tests on mac-cairo)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-sub)
- GitHub Check: New pull request (Hosted Running tests on mac-griffin)
- GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x86-graal)
- GitHub Check: New pull request (Trigger Enterprise CI Trigger Enterprise Pipeline)
- GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-root)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-arm64)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Griffin tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Cairo tests on linux-x64-zfs)
- GitHub Check: New pull request (SelfHosted Other tests on linux-x86-graal)
- GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
- GitHub Check: New pull request (Check Changes Check changes)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (3)
core/src/main/java/io/questdb/std/Os.java (3)
32-32: Nullability import use is appropriateUsing
@Nullablefor the new public method’s return improves the contract clarity for callers and matches hownullis used to signal “fallback to JAR extraction”. No issues here.
39-42: New imports forURL/CodeSourceare correct and scopedThe additional imports (
URL,CodeSource) are the right primitives for runtime image detection, and you’ve avoided importingjava.nio.file.Pathto keep it distinct fromio.questdb.std.str.Path, which is important for readability and ambiguity avoidance.
375-377: Integration into static initializer preserves previous fallback behaviorThe new
tryLoadFromDistribution(cxxLibName, rustLibName)call in the static initializer cleanly precedes the existing JAR/dev loading logic and only short‑circuits when both native libs are successfully loaded from a detected distribution directory. If detection fails or loading throws, you fall back to the prior JAR‑based path unchanged.This aligns nicely with the PR goal (auto‑detect libs in jlink images / configured dists, but never regress the old behavior) and looks good to me.
|
[PR Coverage check]😞 fail : 8 / 20 (40.00%) file detail
|
4 participants
Improvements for native library loading: