gnodet-bot commented on code in PR #13490: URL: https://github.com/apache/maven/pull/13490#discussion_r4284334212
########## apache-maven/src/test/scripts/test-mvn-path-conversion.sh: ########## @@ -65,7 +65,7 @@ project_dir="$work_dir/project" mkdir -p "$stub_dir" "$maven_home/bin" "$maven_home/boot" "$project_dir/.mvn" "$project_dir/module" cp "$mvn_script" "$maven_home/bin/mvn" -touch "$maven_home/bin/m2.conf" "$maven_home/boot/plexus-classworlds-9.9.9.jar" +touch "$maven_home/bin/m2.conf" "$maven_home/boot/maven-classworlds-9.9.9.jar" Review Comment: ⚠️ **Missing `maven-api-classworlds-9.9.9.jar` — compound `LAUNCHER_JAR` path conversion never exercised** `test-mvn-fast-version.sh` (updated in the same commit) correctly adds both boot jars. This script only adds one. The `mvn` script builds `LAUNCHER_JAR=jar1:jar2` — without the second jar, the glob `echo .../maven-api-classworlds-*.jar` is unexpanded, so `LAUNCHER_JAR` contains a literal `*` as the second component. The Cygwin `cygpath --path --windows` conversion of a compound two-jar path is never tested. ```suggestion touch "$maven_home/bin/m2.conf" "$maven_home/boot/maven-classworlds-9.9.9.jar" "$maven_home/boot/maven-api-classworlds-9.9.9.jar" ``` ########## 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 return type change on `getClassWorld()` — breaks existing embedders** This import swap changes the return type of the public `getClassWorld()` method from `org.codehaus.plexus.classworlds.ClassWorld` to `org.apache.maven.api.classworlds.ClassWorld`. Return types are part of the JVM method descriptor — any Maven embedder compiled against `maven-embedder` 4.0.x that calls `CliRequest.getClassWorld()` will get `NoSuchMethodError` at runtime (source-compatible because the impl implements the interface, but not binary-compatible). The compat module exists to keep existing embedders working without surprises. Keep the return type as `org.codehaus.plexus.classworlds.ClassWorld` — callers that want the API type can widen it themselves, since the impl implements the interface. -- 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]
