Tarun4201 commented on PR #13277: URL: https://github.com/apache/maven/pull/13277#issuecomment-5872461531
Hi @gnodet and @elharo, Update on the PR and review findings: ### 1. Fixes in Commit [`ca84dc3d`](https://github.com/apache/maven/pull/13277/commits/ca84dc3d8dcb4a9ecbe45661772d42a591f92412) - **CI Build & Formatting**: Re-formatted with `mvn spotless:apply` to resolve formatting violations in `MavenClappCling.java` and `MavenClappClingTest.java`. Spotless and Checkstyle now pass with 0 violations. - **Launcher Recursion & Missing `mainClass` Guard (Copilot High)**: `MavenClappCling.main()` now throws a fast-failing `ClappException` if `mainClass` is unconfigured. The fallback to standard Maven now calls `new MavenCling(world).run(...)` directly instead of re-entering `MavenCling.main()`, eliminating infinite recursion. - **Child-First Classloading (Copilot Medium)**: Replaced `URLClassLoader` with `ClappClassLoader` implementing child-first resolution for private tool JARs in `${maven.home}/lib/clapp/<tool>/`, preserving core Java and Maven/Plexus API classes parent-first. - **Dynamic JAR Verification (Copilot Low)**: Added `launchClappLoadsFromJarInClappDirectory()` which compiles and packages a standalone class into a fixture `.jar` under `lib/clapp/standalone/` at test time, verifying that tool classes are properly loaded from disk by the child classloader. - **Test Suite**: All 13 tests in `MavenClappClingTest` pass cleanly. --- ### 2. Reply to @gnodet regarding Architectural Direction Thank you @gnodet for the detailed perspective and context on `mvnup` and the `lib/` dependency layout. The original motivation of MNG-8758 was to provide a safe place for CLI sub-tools without leaking dependencies into `plexus.core`. However, I completely understand and respect the PMC's concern regarding adding a public external-tool dispatch mechanism and maintaining `clapp.properties` protocol if third-party CLI tooling is outside Maven's design goals. If the PMC consensus is that `mvnup`'s dependencies should instead be isolated by splitting `mvnup` out of `maven-cli` into its own module with a dedicated realm in ClassWorlds, I am fully in support of that direction. Please let me know if you would like me to close this PR and open an issue/PR for the dedicated `mvnup` ClassWorlds module, or if you prefer a different approach. Thank you! -- 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]
