gnodet-bot commented on code in PR #13491:
URL: https://github.com/apache/maven/pull/13491#discussion_r4288713357
##########
apache-maven/src/assembly/maven/bin/mvn:
##########
@@ -484,20 +484,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
# For Cygwin and MinGW, switch paths to Windows format before running java(1)
command
if $cygwin || $mingw ; then
[ -n "$JAVA_HOME" ] &&
JAVA_HOME=`cygpath --windows "$JAVA_HOME"`
- LAUNCHER_JAR=`cygpath --windows "$LAUNCHER_JAR"`
+ LAUNCHER_JAR=`cygpath --windows --path "$LAUNCHER_JAR"`
CLASSWORLDS_CONF=`cygpath --windows "$CLASSWORLDS_CONF"`
MAVEN_HOME=`cygpath --windows "$MAVEN_HOME"`
# converted separately, appending to the already converted MAVEN_HOME would
# produce a path mixing both separators, e.g. C:\maven/lib/jline-native
JLINE_NATIVE_PATH=`cygpath --windows "$JLINE_NATIVE_PATH"`
+ MAVEN_PROJECTBASEDIR=`cygpath --windows "$MAVEN_PROJECTBASEDIR"`
+ MODULES_DIR=`cygpath --windows "$MODULES_DIR"`
+fi
+
+MAVEN_MODULE_OPTS=""
+if $USE_MODULE_PATH; then
+ MAVEN_MODULE_OPTS="--module-path \"$MODULES_DIR\" --add-modules
ALL-MODULE-PATH --enable-native-access=org.jline.terminal.ffm,org.jline.nativ"
Review Comment:
⚠️ **`--enable-native-access` regression when module-path probe fails (still
open)**
`MAVEN_MODULE_OPTS` is initialized to empty. When the probe fails (older
JDK, missing `lib/modules/`, Cygwin with POSIX path — see below), the java
command runs with **no** `--enable-native-access` flag at all. On JDK 22+ this
silently denies JLine's native FFM access, breaking terminal functionality. The
old script passed `--enable-native-access=ALL-UNNAMED` unconditionally.
Initialize the fallback before the probe result is applied:
```suggestion
MAVEN_MODULE_OPTS="--enable-native-access=ALL-UNNAMED"
if $USE_MODULE_PATH; then
MAVEN_MODULE_OPTS="--module-path \"$MODULES_DIR\" --add-modules
ALL-MODULE-PATH --enable-native-access=org.jline.terminal.ffm,org.jline.nativ"
fi
```
##########
apache-maven/src/assembly/maven/bin/mvn:
##########
@@ -484,20 +484,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 with pre-conversion MODULES_DIR
(partially addressed)**
The path conversion at line 513 (`MODULES_DIR=\`cygpath --windows ...\``) is
now correctly placed before `MAVEN_MODULE_OPTS` is computed — that part is
fixed. However, the **probe itself** at line 498 still runs with `MODULES_DIR`
in POSIX format (`/cygdrive/c/…`). On Cygwin/MinGW, `$JAVACMD` is a native
Windows binary that cannot parse POSIX paths → probe always exits non-zero →
`USE_MODULE_PATH=false` → combined with the issue at line 516, this means no
`--enable-native-access` at all on Cygwin.
Convert `MODULES_DIR` before the probe runs, or probe with a separately
converted copy:
```suggestion
_PROBE_MODULES_DIR="$MODULES_DIR"
if $cygwin || $mingw ; then
_PROBE_MODULES_DIR=`cygpath --windows "$MODULES_DIR"`
fi
USE_MODULE_PATH=false
if "$JAVACMD" --module-path "$_PROBE_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
```
##########
apache-maven/src/assembly/maven/bin/mvn:
##########
@@ -484,20 +484,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
# For Cygwin and MinGW, switch paths to Windows format before running java(1)
command
if $cygwin || $mingw ; then
[ -n "$JAVA_HOME" ] &&
JAVA_HOME=`cygpath --windows "$JAVA_HOME"`
- LAUNCHER_JAR=`cygpath --windows "$LAUNCHER_JAR"`
+ LAUNCHER_JAR=`cygpath --windows --path "$LAUNCHER_JAR"`
CLASSWORLDS_CONF=`cygpath --windows "$CLASSWORLDS_CONF"`
MAVEN_HOME=`cygpath --windows "$MAVEN_HOME"`
# converted separately, appending to the already converted MAVEN_HOME would
# produce a path mixing both separators, e.g. C:\maven/lib/jline-native
JLINE_NATIVE_PATH=`cygpath --windows "$JLINE_NATIVE_PATH"`
+ MAVEN_PROJECTBASEDIR=`cygpath --windows "$MAVEN_PROJECTBASEDIR"`
Review Comment:
⚠️ **Dead code: `MAVEN_PROJECTBASEDIR` converted then immediately
overwritten**
Line 512 converts `MAVEN_PROJECTBASEDIR` to Windows format via `cygpath`,
but line 520 unconditionally overwrites it with `MAVEN_PROJECTBASEDIR_NATIVE`
(the native value set earlier). The conversion here has no effect. This new
line should be removed.
```suggestion
MODULES_DIR=`cygpath --windows "$MODULES_DIR"`
```
##########
apache-maven/src/assembly/maven/bin/mvn.cmd:
##########
@@ -391,25 +391,39 @@ goto processArgs
:endHandleArgs
call :processArgs %*
-for %%i in ("%MAVEN_HOME%"\boot\plexus-classworlds-*) do set LAUNCHER_JAR="%%i"
+set LAUNCHER_JAR="%MAVEN_HOME%\boot\*"
set LAUNCHER_CLASS=org.codehaus.plexus.classworlds.launcher.Launcher
+set MODULES_DIR="%MAVEN_HOME%\lib\modules"
if "%MAVEN_MAIN_CLASS%"=="" @set
MAVEN_MAIN_CLASS=org.apache.maven.cling.MavenCling
-@REM Only pass MAVEN_ARGS for the default Maven build command (MavenCling),
+@rem Module-path and native access flags require a JDK that can read the
module jars.
+@rem The probe includes --module-path so it fails on JDKs that cannot read the
module descriptors.
+set MAVEN_MODULE_OPTS=
Review Comment:
⚠️ **Same `--enable-native-access` regression in Windows script (still
open)**
`MAVEN_MODULE_OPTS` is initialized to empty. When the probe fails, no
`--enable-native-access` is passed — regression from the old unconditional
`ALL-UNNAMED`. Initialize with the fallback:
```suggestion
set MAVEN_MODULE_OPTS=--enable-native-access=ALL-UNNAMED
"%JAVACMD%" --module-path %MODULES_DIR% --add-modules ALL-MODULE-PATH
--enable-native-access=org.jline.terminal.ffm -version >NUL 2>&1
if not ERRORLEVEL 1 (
set MAVEN_MODULE_OPTS=--module-path %MODULES_DIR% --add-modules
ALL-MODULE-PATH --enable-native-access=org.jline.terminal.ffm,org.jline.nativ
)
```
##########
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 field type change in compat layer (still open)**
The `classWorld` field at line 39 declares `ClassWorld classWorld`. With the
old import it was `Lorg/codehaus/plexus/classworlds/ClassWorld;` in bytecode;
with the new import (`org.apache.maven.api.classworlds.ClassWorld`, line 26) it
becomes `Lorg/apache/maven/api/classworlds/ClassWorld;`. Any embedder compiled
against Maven 4.0.x that reads `cliRequest.classWorld` directly via field
access will get `NoSuchFieldError` / `IncompatibleClassChangeError` at runtime
without recompilation.
Restoring `getClassWorld()` to return the impl type (line 75–76) is not
sufficient — the field itself is the problem. The compat convention is
explicit: no binary-incompatible changes in `compat/maven-embedder` without a
release note and explicit embedder migration path.
##########
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 (still open)**
The same pattern continues: `ClassWorld` is declared as the API type, but
`getRealms()`, `setParentRealm()`, `ContainerConfiguration.setClassWorld()`,
and `ContainerConfiguration.setRealm()` don't exist on the API interface — so
every use site requires an explicit downcast to the plexus impl. This occurs at
lines 288, 328, 766, 781, 782, 824, 899, 900, 963–967.
The `ClassWorld` API interface explicitly omits `getRealms()` (noted in a
comment as intentional). If the compat layer cannot use the API types without
constant downcasting, the API surface is incomplete for this use case. The
cleaner alternative: keep `CliRequest.classWorld` as the impl type
(`org.codehaus.plexus.classworlds.ClassWorld`) and only use the API type in new
non-compat code.
##########
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`? (still unanswered)**
JLine JARs appear in both directives:
- Line 23: `module ${maven.home}/lib/modules/*.jar` → loaded into a child
`ModuleLayer`
- Line 34: `load ${maven.home}/lib/modules/*.jar` → added to `plexus.core`
realm's classpath
This question from the first review was not answered. With
`ParentFirstStrategy`, the module-layer parent is checked first so the
classpath entries are shadowed — functionally no duplication. But with
`SelfFirstStrategy`, classes could be loaded from the realm's own classpath (a
different `ClassLoader` instance), causing class identity mismatches if JLine
objects cross realm boundaries.
If `load` is a fallback for when the `module` directive is unsupported,
please add a comment clarifying this. If it's truly redundant, removing it
would avoid the edge case entirely.
--
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]
