PoC: "smart" module-path resolution#2520
Conversation
Proof of concept for jbangdev#2511: instead of special-casing JavaFX, treat the script as a class-path application but promote every resolved dependency that is a real (non-automatic) module onto the `--module-path`, adding those modules as `--add-modules` roots so the boot layer resolves them and their `requires` transitively. Plain jars and automatic modules stay on the class-path. Leaving automatic modules on the class-path is the key: their name is derived from the file name and may be an invalid Java identifier (e.g. `fastparse_2.13-2.3.3.jar` -> `fastparse.2.13`), which aborts boot-layer initialization if such a jar is forced onto the module-path. Gated behind the system property `jbang.hybrid.module.resolve` so the default behaviour is unchanged. Verified on a mixed dependency set (org.openjfx:javafx-web:26.0.1 + com.lihaoyi:fastparse_2.13:2.3.3): with the flag set, javafx.web/jdk.jsobject land on the module-path, fastparse stays on the class-path, and the app boots -- without any JavaFX special-casing. Refs jbangdev#559, jbangdev#2511 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dule Extends the hybrid PoC to follow `requires` edges. The seed is still the set of real (non-automatic) modules, but resolution now walks their `requires` transitively and promotes any required module that is present in the resolved artifacts -- including an *automatic* module. This is safe: a module can only appear in a `requires` clause if its name is a valid Java identifier, so an automatic module that is actually required is guaranteed to have a usable name and can live on the module-path. Automatic and plain jars that nobody requires still stay on the class-path, where their file-name-derived (possibly invalid) module name does no harm. Refs jbangdev#559, jbangdev#2511 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
-Djbang.hybrid.module.resolve)
|
Nice. Can we try and see what happens if we remove all the special JavaFx detection code? Do things still keep working? |
|
Or better yet, do what @maxandersen suggested (assuming things keep working) and automatically enable that feature when we detect JavaFx. |
…casing Module-path resolution used to kick in only for JavaFX (string-matched on `org/openjfx/javafx-`), while the generic hybrid path sat behind the `-Djbang.hybrid.module.resolve` PoC flag. Run hybrid resolution unconditionally (JDK 9+): promote every resolved real (non-automatic) module plus its transitive `requires` onto the `--module-path` and add them as roots, leaving plain/automatic jars on the class-path. This covers JavaFX, `//MODULE` scripts, and any other modular dependency uniformly, so the JavaFX-specific detection (`hasJavaFX`, the `javafx`/`jdk.jsobject` name matching) and the PoC flag are no longer needed. `jdk.jsobject` (required by `javafx.web`, shipped separately since JDK 26) is still handled, now via the requires-graph rather than a hard-coded name. Verified: TestRun, DependencyResolverTest and TestArtifactInfo pass, including all testJavaFX* cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
-Djbang.hybrid.module.resolve)|
Tried it — removed all the JavaFX-specific detection ( On @maxandersen's suggestion to only flip the feature on when we detect JavaFX: I started down that road but As a bonus the redundancy worry goes away: the only jars now living on both module-path and class-path are the JavaFX ones, which is exactly what the old code already did. Edit: Investigating failing tests |
Making the hybrid module-path promotion unconditional (dc3ef46) broke the smoke-quarkus-test. A plain class-path app such as a Quarkus CLI fails at startup with: Exception in thread "main" java.lang.ExceptionInInitializerError Caused by: java.lang.IllegalArgumentException: This library does not have private access to interface io.smallrye.config._private.ConfigLogging because hybrid promotes `jboss-logging` to a named module on the `--module-path` while `smallrye-config` stays on the class-path (unnamed module). A named module cannot read the unnamed module, so jboss-logging's reflective logger-proxy generation into smallrye-config's `_private` package is denied. Frameworks like Quarkus rely on the relaxed access of the class-path and cannot run as auto-generated named modules. Gate the hybrid resolution on `hasJavaFX()` again (keeping the generic requires-graph logic, dropping the name-matching). `//MODULE` scripts don't need this path: the command generators already put their whole class-path on the module-path via `-p` and launch with `-m`, so JavaFX detection is the correct and sufficient trigger. Verified: TestRun, DependencyResolverTest, TestArtifactInfo and TestModule pass; the qcli smoke test runs again (prints "Hello World!!", class-path only). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Seems like we can't be "brute force", but with some "intelligence" -- 1f0e725 - revealed by the tests 😅 Claude's text Tried
Root cause: hybrid promotes Where it landed (
Verified the four test suites pass and the qcli smoke test runs again ( |
|
...so what happens now when using quarkus-fx that includes javafx? :) |
|
I don't think the idea was to always turn on the hybrid mode, there's bound to be situations where that will not work, or where it is simply unwanted. And what I meant by Max's idea was : don't have special code to change the class/module path (which is what this PR does), but it should still use the special detection that decides when to turn this feature on!
Well ... what does the current JBang do in that case? |
Currently, it seems, it works in most cases 😅. I was so happy that it worked w/ JavaFX without any issues - and then I saw this Quarkus test 🙈 Will keep you updated. |
|
Oh, you responded quick, so you might not have read the edit: And what I meant by Max's idea was : don't have special code to change the class/module path (which is what this PR does), but it should still use the special detection that decides when to turn this feature on! So it's either the PoC property that turns it on or the old JavaFx detection. But using the new smart path code. |
The hybrid resolution promoted *every* real (non-automatic) module onto the module-path once JavaFX was detected. For a quarkus-fx style app this still broke startup identically to the original Quarkus regression: a named jboss-logging on the module-path cannot build a logger proxy for smallrye-config's ConfigLogging interface that stays in the unnamed module (class-path). Seed the requires-graph from the JavaFX modules only and follow their requires edges, matching PR jbangdev#2511's actual intent ("put modules required by JavaFX on the module-path"). javafx.* and what they require (e.g. jdk.jsobject) land on the module-path; everything else - including unrelated real modules like jboss-logging - stays on the class-path. Add a regression test for the non-//MODULE case (JavaFX next to an unrelated real module). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Good catch on quarkus-fx — I checked, and it did still break, even with the JavaFX gate. 🤖 A quarkus-fx app (Quarkus + JavaFX) failed identically to the original Quarkus regression: Root cause: once JavaFX was detected, the hybrid resolution promoted every real (non-automatic) module onto the module-path — including Fix: seed the requires-graph from the JavaFX modules only and follow their
This narrow scoping is also why no
So it differs only where it must and is provably inert everywhere else — safe as the default, with nothing for a toggle to add. Verified Notes / scope
|
|
Uff, I think, I am running in a circle now -- maybe, only support the test case as PR - and remove the other thing? -- I think, it's rubbish. Only JavaFX handling. No one asked for it to make the code worse as before. -- Hybrid was an idea, but creating dozens (?) of MWEs (by Claude) showed: Not working... |
|
Well, then we just leave the JavaFx detection but using the more generic path modification. That we we don't have to hard-code extra dependencies that might or might not exists and it also means we could more easily add other modules to the list if we ever find other well-known often-used modules that we'd like to put on the module path by default. |
Current state & verification 🤖After the quarkus-fx fix I wanted to pin down what this branch actually changes vs Results✅ = runs, ❌ = fails. ᵛ = verified by actually running it (or reproduced earlier in this PR); unmarked cells are reasoned from the code.
Honest takeawayFor all five runtime scenarios,
i.e. cleanup/hardening, not new capability. The "promote arbitrary modular deps to the module-path" idea was proven to break reflective frameworks (C1, then C5) and was reverted. The one genuinely unsolved limitation
Root cause: openjfx ships a stub Minimal jbang test scripts used aboveC1 — plain Quarkus (non-fx, modular + non-modular, reflective) //DEPS io.quarkus:quarkus-core:3.15.1
public class C1PlainQuarkus {
public static void main(String... args) { System.out.println("C1 plain-quarkus ok"); }
}C2 — JavaFX only //DEPS org.openjfx:javafx-base:21.0.2
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;
public class C2Fx {
public static void main(String... args) {
ObservableList<String> l = FXCollections.observableArrayList("hi");
System.out.println("C2 fx ok: " + l);
}
}C3 — JavaFX + non-modular jar (docopt) //DEPS org.openjfx:javafx-base:21.0.2
//DEPS com.offbytwo:docopt:0.6.0.20150202
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;
public class C3FxPlain {
public static void main(String... args) {
ObservableList<String> l = FXCollections.observableArrayList("hi");
System.out.println("C3 fx+plain ok: " + l + " / " + org.docopt.Docopt.class.getName());
}
}C4 — JavaFX + modular jar (jboss-logging), no cross-module reflection //DEPS org.openjfx:javafx-base:21.0.2
//DEPS org.jboss.logging:jboss-logging:3.6.0.Final
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;
import org.jboss.logging.Logger;
public class C4FxMod {
public static void main(String... args) {
ObservableList<String> l = FXCollections.observableArrayList("hi");
Logger.getLogger(C4FxMod.class).info("hello from jboss-logging");
System.out.println("C4 fx+mod ok: " + l);
}
}C5 — JavaFX + Quarkus (modular + non-modular + reflection = quarkus-fx) //DEPS io.quarkus:quarkus-core:3.15.1
//DEPS org.openjfx:javafx-base:21.0.2
public class C5FxQuarkus {
public static void main(String... args) { System.out.println("C5 fx+quarkus ok"); }
}
//MODULE
//DEPS info.picocli:picocli:4.6.3
package mplain;
public class MPlain {
public static void main(String... args) {
System.out.println("M-plain module ok: " + picocli.CommandLine.class.getName());
}
}
//MODULE
//DEPS org.openjfx:javafx-base:21.0.2
package mfx;
import javafx.collections.FXCollections;
public class MFx {
public static void main(String... args) {
System.out.println("M-fx module ok: " + FXCollections.observableArrayList("hi"));
}
}
|
|
Why did you close the PR? Isn't point 1 interesting enough to merge? |
I thought some clean working
My bet is: JavaFX will never add other non-javafx modules as dependency. If they will, we can change the algorithm to a more generic lookup. - OK, I might lean towards the YAGNI principle too hard? |
I simply dislike hard-coded stuff :-) |
Related comments and issues
Claude-generated information:
Proof of concept — hybrid module-path resolution
Follow-up to #2511 / #559. PoC for the idea @quintesse raised (at #2511 (comment)): instead of special-casing JavaFX, treat the script as a class-path application but move any dependency that is a module onto the
--module-path, and see if that resolves the JavaFX/jdk.jsobjectcase (and works in general).Gated behind a system property so default behaviour is unchanged:
What it does
When the flag is set,
ModularClassPath.getAutoDectectedModuleArgumentsskips the JavaFX path entirely and instead:--module-path.requiresedges transitively and promotes any required module present in the resolved artifacts — including an automatic module, which is safe because a validrequiresclause guarantees a usable module name.--add-modulesroots so the boot layer resolves them.The key rule: never force an automatic module onto the module-path unless it is required by a real module. Their name is derived from the file name and may be an invalid Java identifier (e.g.
fastparse_2.13-2.3.3.jar→fastparse.2.13), which aborts boot-layer init — this is exactly what killed the naive "put everything on the module-path" attempt.MWE / verification
Reproducer for the original bug (fails on stock jbang
0.138):0.138→FindException: Module jdk.jsobject not found, required by javafx.web.Tested the hybrid path on a mixed dependency set that contains both real modules and the offending non-modular jar:
With
-Djbang.hybrid.module.resolve=true:javafx.*+jdk.jsobject→ module-pathfastparse_2.13(+ transitives) → class-pathStatus
This is a proof of concept for discussion, not a finished feature — happy to iterate on how/whether to expose it to users (and whether it should eventually replace the JavaFX special-casing merged in #2511).
🤖 Generated with Claude Code