Croway commented on PR #26039:
URL: https://github.com/apache/camel/pull/26039#issuecomment-5509675580
Follow-up after a deeper pass with a probe run inside the built
`camel-launcher-4.23.0-SNAPSHOT.jar` (Spring Boot loader 4.1.1). A few of the
points in my review turn out to be more serious than I framed them, plus two
new ones.
**Point 1 is a functional bug, not just duplication.** Inside the fat jar
the main class's code source is
`jar:nested:/.../camel-launcher.jar/!BOOT-INF/classes/!/`, so
`CamelLauncher.detectJarPath()` returns `null` and `camel.launcher.jar` is
never set. `InstallDetector.locate()` then returns `UNKNOWN`, which means
`camel self-update` exits with "unable to determine how the Camel CLI was
installed" and `UpdateChecker` never announces new releases. The self-update
feature introduced in 4.22.0 stays broken after this PR unless the main class
is fixed too.
Related: the description's diagnosis is slightly off. `camel.launcher=true`
*is* set by `CamelLauncher.main`, so `isRunningFromLauncher()` was already
true; the fallback to `camel` happened because `getLauncherJarPath()` returned
`null` inside `getCamelCommand()`.
**The Windows path is a regression, not a nit.** The new branch returns
`/C:/Users/me/.../camel-launcher.jar` and `getCamelCommand()` hands that
verbatim to `java -jar`, which the native launcher rejects with `Error: Unable
to access jarfile /C:/...`. Before this PR, Windows users who also had JBang
fell back to a working `camel.cmd`; after it, `run --background`, `infra run
--background` and every TUI launch fail for them. Decoding through the URI API
fixes both this and the `+` issue in one go, e.g.
```java
Path.of(URI.create("file:" + path.substring(0, idx))).toString()
```
and the same should be applied to the `jar:file:` / `file:` branches.
`BasePackageScanResolver` in camel-support documents this exact `URLDecoder`
pitfall and uses `new URI(...).getPath()` for that reason.
**JVM options are dropped on spawn.** Now that the `java -jar <launcher>`
branch is reachable for the first time, background and TUI children are started
as bare `<java.home>/bin/java -jar camel-launcher.jar ...` with no `-X`/`-D`
forwarding. The wrapper scripts (`camel.sh`, `camel.bat`) apply `JAVA_OPTS` to
the foreground process, and it is a documented contract in the 4.22 upgrade
guide, so a user with proxy or truststore settings in `JAVA_OPTS` gets a
foreground run that works and a background run that cannot download
dependencies. Consider spawning through the wrapper script when present, or
forwarding `JAVA_OPTS` / the relevant parent system properties.
**Silent fallback masks failures.** When `camel.launcher=true` but the jar
path cannot be resolved, `getCamelCommand()` falls through to whichever `camel`
is first on PATH, possibly a different version or an old JBang shim. That is
how this regression went unnoticed, and the next loader URL-shape change would
be masked the same way. Logging a warning that includes the raw code-source URL
(or failing) in that case would be a cheap safeguard.
**Minor:** `isRunningFromLauncher()` matches `contains("camel-launcher")`
against the whole absolute path, and this PR makes that branch reachable for
any Spring Boot 3.2+/4.x app that embeds camel-jbang-core. Matching on the file
name would avoid false positives such as
`~/projects/camel-launcher-poc/target/myapp.jar`.
A table test on an extracted `parseJarPath(String)` covering `jar:nested:`
(both `BOOT-INF/classes` and `BOOT-INF/lib` forms), `jar:file:`, `file:`,
`%20`, `+` and `/C:/` inputs would have caught the decoding and Windows issues.
---
_Claude Code on behalf of Croway_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
