gnodet-bot commented on code in PR #13491:
URL: https://github.com/apache/maven/pull/13491#discussion_r4268134736
##########
apache-maven/src/assembly/maven/bin/mvn:
##########
@@ -445,20 +445,38 @@ fi
if [ -n "$MAVEN_DEBUG_SCRIPT" ]; then
printf '[DEBUG] Final MAVEN_OPTS: %s\n' "$MAVEN_OPTS" >&2
fi
-LAUNCHER_JAR=`echo "$MAVEN_HOME"/boot/plexus-classworlds-*.jar`
+LAUNCHER_JAR=""
+for _jar in "$MAVEN_HOME"/boot/*.jar; do
+ [ -f "$_jar" ] && LAUNCHER_JAR="${LAUNCHER_JAR:+$LAUNCHER_JAR:}$_jar"
+done
LAUNCHER_CLASS=org.codehaus.plexus.classworlds.launcher.Launcher
JLINE_NATIVE_PATH="$MAVEN_HOME/lib/jline-native"
+MODULES_DIR="$MAVEN_HOME/lib/modules"
+
+# Module-path and native access flags require a JDK that can read the module
jars.
+# The probe includes --module-path so it fails on JDKs that cannot read the
module descriptors.
+USE_MODULE_PATH=false
+if "$JAVACMD" --module-path "$MODULES_DIR" --add-modules ALL-MODULE-PATH
--enable-native-access=org.jline.terminal.ffm -version >/dev/null 2>&1; then
+ USE_MODULE_PATH=true
+fi
Review Comment:
⚠️ **Cygwin/MinGW regression: probe runs before path conversion**
The `USE_MODULE_PATH` probe (lines 456–461) runs before the cygwin path
conversion block that converts `MODULES_DIR` to Windows format (line 474). On
Cygwin and MinGW, `MODULES_DIR` is still `/cygdrive/c/.../lib/modules` (a POSIX
path) at probe time; `java.exe` (a native Windows binary) cannot parse it, so
the probe always exits non-zero → `USE_MODULE_PATH=false` →
`MAVEN_MODULE_OPTS=""` → **no `--enable-native-access` at all**.
The old behaviour hardcoded `--enable-native-access=ALL-UNNAMED`
unconditionally. The new behaviour silently drops it on Cygwin/MinGW, breaking
JLine native terminal support.
Fix: move the probe block to **after** the cygwin conversion block (after
line 475), where `MODULES_DIR` is already in the correct Windows format and
`MAVEN_MODULE_OPTS` is set:
```bash
MAVEN_MODULE_OPTS=""
# Module-path and native access flags require a JDK that can read the module
jars.
# The probe includes --module-path so it fails on JDKs that cannot read the
module descriptors.
USE_MODULE_PATH=false
if "$JAVACMD" --module-path "$MODULES_DIR" --add-modules ALL-MODULE-PATH
--enable-native-access=org.jline.terminal.ffm -version >/dev/null 2>&1; then
MAVEN_MODULE_OPTS="--module-path \"$MODULES_DIR\" --add-modules
ALL-MODULE-PATH --enable-native-access=org.jline.terminal.ffm,org.jline.nativ"
fi
```
(i.e., fold the probe directly into the `MAVEN_MODULE_OPTS` assignment and
move the whole block after line 475)
##########
compat/maven-embedder/src/main/java/org/apache/maven/cli/CliRequest.java:
##########
@@ -23,9 +23,9 @@
import java.util.Properties;
import org.apache.commons.cli.CommandLine;
+import org.apache.maven.api.classworlds.ClassWorld;
Review Comment:
⚠️ **Binary-incompatible change in the compat layer**
The `classWorld` field's declared type changes from
`org.codehaus.plexus.classworlds.ClassWorld` to
`org.apache.maven.api.classworlds.ClassWorld` at the bytecode level (only the
import is changed here, but the field descriptor in the class file changes).
This is a binary-incompatible change for any existing embedder that accesses
`CliRequest.classWorld` directly — they'll get a `NoSuchFieldError` or
`ClassCastException` at runtime without recompilation.
The compat-layer convention is explicit: no behaviour changes in
`compat/maven-embedder` unless the same change is ported to Maven 3.x.
`CliRequest` is `@Deprecated`, but it's still a public field used by external
embedders (IDEs, build tools, Maven wrapper implementations). If the intent is
to migrate embedders to the new API type, this needs a release note and ideally
a `@deprecated` field-level Javadoc pointing to the replacement.
##########
apache-maven/src/assembly/maven/bin/m2.conf:
##########
@@ -29,3 +31,4 @@ optionally ${user.home}/.m2/ext/*.jar
optionally ${maven.home}/lib/ext/*.jar
load ${maven.home}/lib/maven-*.jar
load ${maven.home}/lib/*.jar
+load ${maven.home}/lib/modules/*.jar
Review Comment:
❓ **Is the `load lib/modules/*.jar` intentional alongside `module
lib/modules/*.jar`?**
JLine JARs appear in both directives:
- Line 24: `module ${maven.home}/lib/modules/*.jar` → deferred for
`createModuleLayer()` → loaded into a child `ModuleLayer`
- Line 34: `load ${maven.home}/lib/modules/*.jar` → added directly to the
`plexus.core` realm's classpath
After `createModuleLayer()` runs, `plexus.core`'s parent ClassLoader is
retroactively updated to the ModuleLayer loader. With the default
`ParentFirstStrategy`, the parent (module loader) is checked first, so the
classpath entries are effectively shadowed — no functional duplication. However
with `SelfFirstStrategy`, the realm loads JLine from its own classpath (a
different `ClassLoader` instance than the module loader), creating class
identity mismatches if JLine objects cross realm boundaries.
If the `load` line is there only as a classpath fallback (for when the
module probe fails), the existing `addModulePathsToClasspath()` in
`Configurator` already handles that case. Please clarify: is the `load`
directive intentional, or is it a duplicate that should be removed?
##########
compat/maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -285,7 +285,7 @@ public int doMain(String[] args, String workingDirectory,
PrintStream stdout, Pr
final Set<String> realms;
if (classWorld != null) {
realms = new HashSet<>();
- for (ClassRealm realm : classWorld.getRealms()) {
+ for (ClassRealm realm :
((org.codehaus.plexus.classworlds.ClassWorld) classWorld).getRealms()) {
Review Comment:
⚠️ **Repeated downcasts from API type to impl type in compat layer**
This file now casts `ClassWorld` (declared as
`org.apache.maven.api.classworlds.ClassWorld` from `CliRequest.classWorld`)
back down to `org.codehaus.plexus.classworlds.ClassWorld` to access methods
like `getRealms()` that don't exist on the API interface. The same pattern
repeats at lines 328, 766, 781, 782, 824, 899, 900, 966, 967.
These casts will work at runtime (the implementation IS-A API), but this
pattern signals an incomplete API design: if `getRealms()` and
`setParentRealm()` are needed in the compat layer, they should be part of
`org.apache.maven.api.classworlds.ClassWorld`/`ClassRealm`. Alternatively, keep
`CliRequest.classWorld` as the plexus implementation type (avoid the import
swap in `CliRequest.java`) and only use the API type in new code.
--
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]
