CAMEL-23703: camel-launcher - shared Java 17+ runtime discovery for camel.sh and camel.bat#24664
Conversation
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
…amel.sh Replaces ad-hoc JAVACMD/JAVA_HOME logic with the four-candidate precedence (JAVACMD, JAVA_HOME/bin/java, java on PATH, CAMEL_FALLBACK_JAVA), enforcing a Java 17 minimum via captured version probing. JAVA_HOME remains how SDKMAN's java candidate is honored. Intentionally removes the obsolete Darwin JavaVM.framework JAVA_HOME default and its JAVA_VERSION=CurrentJDK handling because it conflicts with the shared candidate precedence; retains Cygwin path handling. Co-authored-by: Codex <noreply@openai.com>
Isolate candidate version checks from launcher stdin so wrappers cannot consume piped input before the final Camel process executes. Adds a POSIX regression test that proves the original input reaches normal invocation. Co-authored-by: Codex <noreply@openai.com>
…amel.bat Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 3 tested, 0 compile-only — current: 1 all testedMaveniverse Scalpel detected 3 affected modules (current approach: 1).
|
gnodet
left a comment
There was a problem hiding this comment.
Thanks for this well-structured contribution, @ammachado. The unified Java 17+ discovery contract is a clear improvement over the ad-hoc logic in both scripts. I reviewed the shell scripts, batch files, test harness, and upgrade guide entry. CI is green.
Positive observations
- The 4-candidate precedence chain (JAVACMD, JAVA_HOME, PATH, CAMEL_FALLBACK_JAVA) is cleanly implemented in both
camel.shandcamel.batwith consistent behavior. - Good use of
command -v javainstead ofwhich java-- the former is POSIX-standard whilewhichis a GNU coreutils extension that may not be available on all systems. - Stdin preservation via
</dev/nullduring version probes prevents the probe from consuming input meant for the final Java process. This is a subtle but important detail. - The old
camel.bathad a bug whereif ERRORLEVEL 1 goto errormapped all non-zero exit codes to 1. The new code correctly preserves the actual child exit code viaset ERROR_CODE=%ERRORLEVEL%. Nice fix. - The
FakeJavatest harness is well-designed: it handles both POSIX and Windows, prevents deadlocks with proper stream draining and timeouts, and supports stdin injection for verifying that probes don't steal input. - Test coverage is comprehensive with good edge cases (early-access versions, Unicode paths, unparseable output, batch wrapper
callvs. native invocation).
Findings
1. Removed platform-specific auto-detection not documented in upgrade guide [low]
The upgrade guide correctly documents the removal of the macOS JavaVM.framework default, but two other platform-specific behaviors were also removed without mention:
- IBM AIX: The old
camel.shprobed$JAVA_HOME/jre/sh/javafirst (IBM's JDK on AIX uses non-standard paths). Users on AIX can still setJAVACMDexplicitly, but the automatic detection is gone. - Gentoo Linux: The old
camel.shdetected/etc/gentoo-releaseand ranjava-config --jre-hometo setJAVA_HOME. Gentoo users who relied on this would need to setJAVA_HOMEmanually.
Both are likely very low-impact for the Camel CLI user base, and the JAVACMD override provides a workaround. Consider adding a brief mention to the upgrade guide entry for completeness, e.g.:
The Gentoo
java-configauto-detection and the IBM AIX$JAVA_HOME/jre/sh/javaprobe were also removed; setJAVA_HOMEorJAVACMDexplicitly on those platforms.
2. No concerns found with the batch probe mechanism [info]
The camel.bat temp-file approach for capturing version output (camel-java-probe-%RANDOM%-%RANDOM%.tmp) is reasonable. The TOCTOU window between if exist and the actual write is negligible in practice for a CLI launcher. The cleanup is handled correctly on both success and failure paths.
Scanner coverage
No static analysis tools were available for this PR (SonarCloud was skipped). This review does not replace specialized review tools such as CodeRabbit, Sourcery, or SonarCloud.
Claude Code review on behalf of @gnodet
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Document the removed java-config and jre/sh/java auto-detection paths alongside the macOS JavaVM.framework note in the 4.22 upgrade guide. Co-authored-by: Cursor <cursoragent@cursor.com>
davsclaus
left a comment
There was a problem hiding this comment.
Review: CAMEL-23703 - camel-launcher shared Java 17+ runtime discovery
Well-structured PR with comprehensive testing across both POSIX and Windows platforms.
Highlights
- Thorough test coverage:
CamelShDiscoveryTest(POSIX),CamelBatDiscoveryTest(Windows),FakeJavaTest, and a sharedFakeJavatest harness covering the full discovery order, version parsing (including legacy1.xformat), fallback chains, rejection of old/garbage/non-executable candidates, stdin isolation, exit code propagation, and edge cases (spaces/unicode in paths). - Clean discovery contract: Clear candidate order (JAVACMD → JAVA_HOME → PATH → CAMEL_FALLBACK_JAVA), version validation with minimum Java 17, and clean fallback chain with diagnostic output on failure.
- Safe removals: The macOS
JavaVM.frameworkdefault (invalid since macOS 10.15+), Gentoojava-config, and AIXjre/sh/javaprobes are all correctly removed — none would ever resolve to Java 17+. Thedarwinvariable was only set but never read after the removed block. - Breaking changes documented: The upgrade guide covers all platform-specific removals (including Gentoo and AIX, added in response to gnodet's review comment).
- AI attribution: Properly attributed in commit metadata and PR description.
No blocking issues found. CI is green.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Description
First slice of the larger CAMEL-23703 unified Camel CLI package distribution effort, split out for focused review. This branch is independent of the other CAMEL-23703 slices and targets
maindirectly.It gives the self-contained
camel-launcherscripts (bin/camel.shandbin/camel.bat) a single, shared Java 17+ runtime discovery contract. Candidates are evaluated in a fixed order and the first that exists, is executable, and reports a Java major version >= 17 wins:JAVACMD(explicit override, unchanged)$JAVA_HOME/bin/java(%JAVA_HOME%\bin\java.exeon Windows; also honors the SDKMANjavacandidate)java(java.exe) onPATHCAMEL_FALLBACK_JAVA, a new variable set by package-manager installs when the JDK location is deterministicCandidates older than 17, missing, non-executable, or with unparseable version output are skipped. If none qualify, the launcher exits nonzero with a diagnostic listing the sources it checked instead of running an unsuitable Java. The obsolete macOS
JAVA_HOME=/System/Library/Frameworks/JavaVM.framework/...default is removed fromcamel.sh;JAVA_OPTShandling is unchanged.Included: the POSIX and Windows-batch discovery contracts, a shared
FakeJavatest harness, behavioral discovery tests for both scripts, and an upgrade-guide note.Target
mainbranch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.AI-assisted contributions
Co-authored-bytrailers) and the PR description identifies the AI tool used.Claude Code (Opus 4.8) on behalf of ammachado