gnodet-bot commented on code in PR #13490:
URL: https://github.com/apache/maven/pull/13490#discussion_r4264830310
##########
impl/maven-core/src/main/resources/META-INF/maven/extension.xml:
##########
@@ -154,7 +151,7 @@ under the License.
<exportedArtifact>org.apache.maven:maven-api-xml</exportedArtifact>
<exportedArtifact>classworlds:classworlds</exportedArtifact>
- <exportedArtifact>org.codehaus.plexus:plexus-classworlds</exportedArtifact>
+ <exportedArtifact>org.apache.maven:maven-classworlds</exportedArtifact>
Review Comment:
⚠️ **`maven-api-classworlds` is missing from `<exportedArtifacts>`**
Every other `maven-api-*` artifact is listed (lines 142–151), but
`maven-api-classworlds` has no entry. Without it, a plugin that declares a
`maven-api-classworlds` dependency gets its own copy loaded into the plugin
realm instead of being filtered to use the container's. The result is a
`ClassCastException` the moment the plugin receives a `ClassRealm` or
`ClassWorld` instance from Maven core — the plugin's class and Maven's class
are different `Class<?>` objects from different loaders.
```suggestion
<exportedArtifact>org.apache.maven:maven-classworlds</exportedArtifact>
<exportedArtifact>org.apache.maven:maven-api-classworlds</exportedArtifact>
```
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/LookupInvoker.java:
##########
@@ -150,22 +147,6 @@ public final int invoke(InvokerRequest invokerRequest) {
} finally {
Thread.currentThread().setContextClassLoader(oldCL);
System.setProperties(oldProps);
Review Comment:
⚠️ **JUL handler leak when Maven runs embedded**
`restoreRootLoggerHandlers()` was removed from the `finally` block, but
`SLF4JBridgeHandler.install()` is still called in `activateLogging()`. The
deleted method's own Javadoc stated the exact problem:
> When Maven runs inside another JVM, the root logger belongs to that JVM,
and a `SLF4JBridgeHandler` left on it would route the host's logging to Maven
and fail once Maven's class realm is closed.
After Maven exits, the bridge handler remains on the JVM's root
`java.util.logging` logger. The next JUL event in the host process routes
through the closed Maven classloader, causing `NoClassDefFoundError:
org/slf4j/Logger` in the host. This regresses any in-process Maven invocation
(IDEs, CI tooling, the Verifier).
Fix: restore the handler cleanup in `finally`:
```suggestion
Thread.currentThread().setContextClassLoader(oldCL);
System.setProperties(oldProps);
SLF4JBridgeHandler.uninstall();
```
##########
impl/maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -364,4 +365,27 @@ private static Object getId(ClassLoader classLoader) {
}
return classLoader;
}
+
+ private void
applyExportOrOpen(org.codehaus.plexus.classworlds.realm.ClassRealm realm,
String spec, boolean open) {
Review Comment:
💡 **`applyExportOrOpen()` is dead code in this PR**
This method is added but never called from within this PR. Based on the PR
description, it will be invoked by `applyModuleAccessDescriptors()` in PR
#13491 (JLine on module-path). Shipping dead code here means CI cannot test
this code path, and any bugs in the parsing logic (e.g. the `=`-target
handling, the `ALL-UNNAMED` check) won't surface until #13491.
Consider either moving this method to PR #13491, or calling it in this PR so
it gets test coverage.
##########
compat/maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java:
##########
@@ -285,17 +259,13 @@ 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:
⚠️ **Behavioral change in the compat layer: embedded terminal and color
detection removed**
`installEmbeddedTerminal()` and its call in `doMain(args, workingDirectory,
stdout, stderr)` were removed, along with the dumb-terminal color detection
(`else if (MessageUtils.getTerminal().getType().startsWith("dumb")) {
MessageUtils.setColorEnabled(false); }`).
`installEmbeddedTerminal()` was added specifically for #13357: without it,
embedders calling `doMain()` with `--color=yes` get no color output because no
terminal is installed. Removing it silently regresses that fix for all
Maven-Verifier and IDE-tooling callers.
The dumb-terminal detection removal means terminals that do not support ANSI
escapes will now receive escape codes when `--color` is not explicitly set,
producing garbled output.
Both are behavioral changes in a `@Deprecated(since="4.0.0")` compat module
whose contract is to keep existing Maven 3 embedders working without surprises.
Per project convention, behavior changes in `compat/` require an equivalent
port to Maven 3.x. If the removal is intentional (e.g., this is superseded by a
change in `impl/maven-cli`), please document it in the PR description and
confirm the 3.x disposition.
--
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]
