gnodet-bot commented on code in PR #11029:
URL: https://github.com/apache/maven/pull/11029#discussion_r4062356486
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java:
##########
@@ -529,21 +549,41 @@ public <T> T getConfiguredMojo(Class<T> mojoInterface,
MavenSession session, Moj
// We are forcing the use of the plugin realm for all lookups that
might occur during
// the lifecycle that is part of the lookup. Here we are specifically
trying to keep
// lookups that occur in contextualize calls in line with the right
realm.
- ClassRealm oldLookupRealm = container.setLookupRealm(pluginRealm);
+ ClassRealm oldLookupRealm =
+
container.setLookupRealm((org.codehaus.plexus.classworlds.realm.ClassRealm)
pluginRealm);
+ // For modular plugins, set TCCL to the ModuleLayer's classloader
ClassLoader oldClassLoader =
Thread.currentThread().getContextClassLoader();
- Thread.currentThread().setContextClassLoader(pluginRealm);
+ ClassLoader effectiveLoader = getEffectiveClassLoader(pluginRealm);
+ Thread.currentThread().setContextClassLoader(effectiveLoader);
try {
- if (mojoDescriptor.isV4Api()) {
+ if (pluginDescriptor.isModular()) {
+ // Modular plugins always use the v4 Mojo loading path with
maven-di
+ return loadV4Mojo(mojoInterface, session, mojoExecution,
mojoDescriptor, pluginDescriptor, pluginRealm);
+ } else if (mojoDescriptor.isV4Api()) {
return loadV4Mojo(mojoInterface, session, mojoExecution,
mojoDescriptor, pluginDescriptor, pluginRealm);
} else {
return loadV3Mojo(mojoInterface, session, mojoExecution,
mojoDescriptor, pluginDescriptor, pluginRealm);
}
} finally {
Thread.currentThread().setContextClassLoader(oldClassLoader);
- container.setLookupRealm(oldLookupRealm);
+
container.setLookupRealm((org.codehaus.plexus.classworlds.realm.ClassRealm)
oldLookupRealm);
+ }
+ }
+
+ /**
+ * Returns the effective classloader for a plugin realm.
+ * For modular plugins, returns the ModuleLayer's classloader;
+ * for classic plugins, returns the realm's own classloader.
+ */
+ private static ClassLoader getEffectiveClassLoader(ClassRealm pluginRealm)
{
+ if (pluginRealm.isModular()) {
+ ModuleLayer layer = pluginRealm.getModuleLayer();
+ // defineModulesWithOneLoader uses a single loader for all modules
in the layer
+ return layer.modules().iterator().next().getClassLoader();
Review Comment:
⚠️ **Fragile: `layer.modules().iterator().next()` is order-dependent**
`ModuleLayer.modules()` returns a `Set<Module>` with unspecified iteration
order. For a single-module plugin this is harmless today, but for any plugin
with multiple modules in its layer this picks an arbitrary module's classloader.
`ModuleLayer.defineModulesWithOneLoader()` guarantees **one shared
ClassLoader** for all modules in the layer, so `module.getClassLoader()`
returns the same instance regardless of which module you pick. The intent is
obscured and the code will silently break if `defineModulesWithManyLoaders` is
used.
Use `pluginRealm.getClassLoader()` directly — the realm's URLClassLoader is
the same parent passed to `defineModulesWithOneLoader`, already holding the
full classpath from `populateRealm`:
```suggestion
return pluginRealm.getClassLoader();
```
##########
apache-maven/src/assembly/component.xml:
##########
@@ -23,14 +23,25 @@ under the License.
<useProjectArtifact>false</useProjectArtifact>
<outputDirectory>boot</outputDirectory>
<includes>
- <include>org.codehaus.plexus:plexus-classworlds</include>
+ <include>org.apache.maven:maven-classworlds</include>
+ <include>org.apache.maven:maven-api-classworlds</include>
</includes>
</dependencySet>
+
+ <dependencySet>
+ <useProjectArtifact>false</useProjectArtifact>
+ <outputDirectory>lib/modules</outputDirectory>
+ <includes>
+ <include>org.jline:*</include>
+ </includes>
+ </dependencySet>
+
<dependencySet>
<useProjectArtifact>false</useProjectArtifact>
<outputDirectory>lib</outputDirectory>
<excludes>
- <exclude>org.codehaus.plexus:plexus-classworlds</exclude>
+ <exclude>org.apache.maven:maven-classworlds</exclude>
+ <exclude>org.apache.maven:maven-api-classworlds</exclude>
Review Comment:
⚠️ **Bug: JLine deployed to both `lib/modules/` and `lib/`**
The second `dependencySet` places all `org.jline:*` artifacts into
`lib/modules/`. The third `dependencySet` (for `lib/`) has no exclusion for
`org.jline`, so the Maven assembly plugin copies JLine to **both** directories.
At runtime, `lib/*.jar` lands on the unnamed-module classpath (loaded via
`m2.conf` `load ${maven.home}/lib/*.jar`), while `lib/modules/*.jar` is placed
on the `--module-path`. Running the same JLine classes as both a named module
(from `lib/modules/`) and an unnamed-module class (from `lib/`) produces a
split-package violation — the JVM will refuse to create the module layer
(`LayerInstantiationException`) or silently shadow the named module, defeating
the purpose of this change.
Fix: add `<exclude>org.jline:*</exclude>` to the `lib/` dependencySet:
```suggestion
<exclude>org.apache.maven:maven-classworlds</exclude>
<exclude>org.apache.maven:maven-api-classworlds</exclude>
<exclude>org.jline:*</exclude>
```
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/DefaultPluginRealmCache.java:
##########
@@ -29,11 +29,11 @@
import java.util.concurrent.ConcurrentHashMap;
import org.apache.maven.RepositoryUtils;
+import org.apache.maven.api.classworlds.ClassRealm;
Review Comment:
⚠️ **Cache correctness: `CacheKey` does not include the `modular` flag**
`CacheKey` is keyed on `(plugin, parentRealm, foreignImports, filter,
repositories, session)` with no `modular` field. If the same plugin coordinate
appears in two projects in a multi-module build — one with
`<modular>true</modular>` and one without — the second lookup gets a cache hit
and receives the wrong realm type. A classpath realm returned where a
`ModuleLayer` realm was expected (or vice versa) causes a `ClassCastException`
in `getConfiguredMojo` or `Injector.discover()` failing to find module
descriptors.
Add `boolean modular` to `CacheKey` and include it in `hashCode`/`equals`.
The `createKey(...)` factory needs a corresponding `boolean modular` parameter,
propagated from `PluginDescriptor.isModular()` at the call site in
`DefaultMavenPluginManager.setupPluginRealm`.
--
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]
