gnodet-bot commented on code in PR #13490:
URL: https://github.com/apache/maven/pull/13490#discussion_r4292739723
##########
pom.xml:
##########
@@ -142,9 +142,8 @@ under the License.
<maven.site.path>ref/4-LATEST</maven.site.path>
<project.build.outputTimestamp>2025-06-24T20:18:00Z</project.build.outputTimestamp>
<!-- various versions -->
- <asmVersion>9.11</asmVersion>
+ <asmVersion>9.10.1</asmVersion>
Review Comment:
⚠️ **Version downgrades introduced by rebase divergence** — the PR branch is
behind `master` and introduces three unintended version regressions:
- `asmVersion`: 9.10.1 (PR) vs 9.11 (master)
- `jlineVersion` (line 155): 4.4.7 (PR) vs 4.4.8 (master)
- `stax2ApiVersion` (line 169): 4.3.1 (PR) vs 4.3.2 (master)
Please rebase on current `master` before merging.
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultProject.java:
##########
@@ -58,7 +58,8 @@ public DefaultProject(InternalMavenSession session,
MavenProject project) {
this.project = project;
ClassLoader ttcl = Thread.currentThread().getContextClassLoader();
try {
-
Thread.currentThread().setContextClassLoader(project.getClassRealm());
+ Thread.currentThread()
+ .setContextClassLoader(project.getClassRealm() != null ?
project.getClassRealm() : null);
Review Comment:
💡 **No-op ternary:** Same pattern as `DefaultMaven.java` — `x != null ? x :
null` simplifies to `x`. `setContextClassLoader(null)` is actually valid
(resets to bootstrap classloader) and was the pre-existing behavior when
`getClassRealm()` returned null, so the ternary doesn't change anything.
```suggestion
Thread.currentThread().setContextClassLoader(project.getClassRealm());
```
##########
impl/maven-core/src/main/java/org/apache/maven/DefaultMaven.java:
##########
@@ -467,7 +467,7 @@ protected <T> Collection<T>
getProjectScopedExtensionComponents(Collection<Maven
ClassLoader originalContextClassLoader =
currentThread.getContextClassLoader();
try {
for (MavenProject project : projects) {
- ClassLoader projectRealm = project.getClassRealm();
+ ClassLoader projectRealm = project.getClassRealm() != null ?
project.getClassRealm() : null;
Review Comment:
💡 **No-op ternary:** `x != null ? x : null` is always equivalent to `x`.
This is a style regression vs the original `ClassLoader projectRealm =
project.getClassRealm();` — the null check before
`scannedRealms.add(projectRealm)` on the next line already guards correctly.
```suggestion
ClassLoader projectRealm = project.getClassRealm();
```
##########
apache-maven/src/assembly/maven/bin/mvn:
##########
@@ -484,15 +484,15 @@ 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=`echo "$MAVEN_HOME"/boot/maven-classworlds-*.jar`
LAUNCHER_CLASS=org.codehaus.plexus.classworlds.launcher.Launcher
JLINE_NATIVE_PATH="$MAVEN_HOME/lib/jline-native"
# 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 --path --windows "$LAUNCHER_JAR"`
Review Comment:
💡 **Minor:** `cygpath --path` is designed for colon-separated `PATH` strings
(converts each component). `LAUNCHER_JAR` is a single file path with no colons,
so `--path` is a no-op here. The other entries in this block (`JAVA_HOME`,
`CLASSWORLDS_CONF`, `MAVEN_HOME`, `JLINE_NATIVE_PATH`) all use plain
`--windows`. Consider aligning:
```suggestion
LAUNCHER_JAR=`cygpath --windows "$LAUNCHER_JAR"`
```
--
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]
