gnodet-bot commented on code in PR #13494:
URL: https://github.com/apache/maven/pull/13494#discussion_r4358570782
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java:
##########
@@ -567,9 +608,10 @@ private <T> T loadV4Mojo(
org.apache.maven.api.plugin.Log log = new DefaultLog(
LoggerFactory.getLogger(mojoExecution.getMojoDescriptor().getImplementation()));
try {
- // Discover components from the effective ClassLoader (module
layer loader for modular plugins)
+ // For modular plugins, discover from the ModuleLayer's classloader
+ ClassLoader discoveryLoader = getEffectiveClassLoader(pluginRealm);
Injector injector = Injector.create();
- injector.discover(pluginRealm);
+ injector.discover(discoveryLoader);
Review Comment:
🔴 **F15 (new): Class identity mismatch — modular plugin mojo instantiation
will fail at runtime**
`injector.discover(discoveryLoader)` registers bindings keyed by `Class`
objects loaded by the **module layer's classloader** (`discoveryLoader`). But
the lookup 12 lines below uses
`pluginRealm.loadClass(mojoDescriptor.getImplementation())`, which loads from
the **`ClassRealm`'s URL classpath** — a different `ClassLoader` instance.
In Java, class identity is `(name, classLoader)`. Two `Class` objects with
the same name but different loaders are distinct.
`Key.of(pluginRealm.loadClass(...))` produces a key that won't match any
binding registered by `injector.discover(discoveryLoader)` → mojo instantiation
fails with "no binding found" or `ClassCastException` at runtime for every
modular plugin.
Fix: use `discoveryLoader.loadClass(...)` for the injector lookup:
```java
mojo = mojoInterface.cast(injector.getInstance(
Key.of(discoveryLoader.loadClass(mojoDescriptor.getImplementation()),
mojoDescriptor.getRoleHint())));
```
##########
impl/maven-classworlds/src/main/java/org/codehaus/plexus/classworlds/ClassWorld.java:
##########
@@ -207,60 +211,15 @@ public synchronized void addOpens(String moduleName,
String packageName, Module
*/
public synchronized void addReads(String sourceModuleName, Module target) {
Module source = findModule(sourceModuleName);
- if (source == null) {
- LOGGER.log(
- System.Logger.Level.WARNING,
- "module-access: module ''{0}'' not found, ignoring
add-reads",
- sourceModuleName);
- return;
- }
- if (moduleLayerController == null) {
- LOGGER.log(
- System.Logger.Level.WARNING,
- "module-access: no ModuleLayer controller available,
cannot add-reads {0}",
- sourceModuleName);
- return;
- }
- if (isBootLayerModule(source)) {
- LOGGER.log(
- System.Logger.Level.WARNING,
- "module-access: module ''{0}'' is in the boot layer and
cannot be modified at runtime;"
- + " use --add-reads {0}=ALL-UNNAMED on the JVM
command line instead",
- sourceModuleName);
+ if (source == null || moduleLayerController == null ||
isBootLayerModule(source)) {
Review Comment:
⚠️ **F2 REGRESSION WORSENED: All 6 WARNING diagnostics removed, now
completely silent**
The previous code had separate guarded returns with actionable
`WARNING`-level log messages for three distinct failure conditions in both
`addReads()` and `applyModuleAccess()`:
1. Module not found → `"module-access: module ''{0}'' not found, ignoring
add-reads"`
2. No controller → `"module-access: no ModuleLayer controller available,
cannot add-reads {0}"`
3. Boot-layer module → `"module-access: module ''{0}'' is in the boot layer
and cannot be modified at runtime"`
The new combined guard drops all logging entirely. When a user's
`META-INF/maven/module-access` has a typo'd module name, they get zero
feedback. This violates Maven's convention that warnings must be actionable.
Restore at minimum `DEBUG`-level logging:
```suggestion
if (source == null) {
logger.debug("module-access: module '{}' not found in runtime
layers; skipping add-reads directive", sourceModuleName);
return;
}
if (moduleLayerController == null) {
logger.debug("module-access: no ModuleLayer controller for this
realm; skipping add-reads directive for '{}'", sourceModuleName);
return;
}
if (isBootLayerModule(source)) {
logger.debug("module-access: module '{}' is in the boot layer
and cannot be modified at runtime; use --add-reads on the JVM command line
instead", sourceModuleName);
return;
}
```
##########
impl/maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -259,6 +272,101 @@ public ClassRealm createPluginRealm(
return createRealm(getKey(plugin, false), RealmType.Plugin, parent,
parentImports, foreignImports, artifacts);
}
+ @Override
+ public ClassRealm createModularPluginRealm(Plugin plugin, ClassLoader
parent, List<Artifact> artifacts)
+ throws ModuleLayerCreationException {
+ Objects.requireNonNull(plugin, "plugin cannot be null");
+
+ if (parent == null) {
+ parent = PARENT_CLASSLOADER;
+ }
+
+ String realmId = getKey(plugin, false);
+ ClassRealm classRealm = newRealm(realmId);
+ classRealm.setParentClassLoader(parent);
+
+ // Collect all artifact paths for the module finder
+ List<Path> modulePaths = new ArrayList<>();
+ if (artifacts != null) {
+ for (Artifact artifact : artifacts) {
+ if (artifact.getFile() != null &&
!isProvidedArtifact(artifact, true)) {
+ modulePaths.add(artifact.getFile().toPath());
+ } else if (logger.isDebugEnabled() && artifact.getFile() !=
null) {
+ logger.debug(" Excluded from module path: {}",
getId(artifact));
+ }
+ }
+ }
+
+ if (modulePaths.isEmpty()) {
+ throw new ModuleLayerCreationException("Plugin " + plugin.getId()
+ + " declares <modular>true</modular> but has no artifacts"
+ " to place on the module path");
+ }
+
+ try {
+ ModuleFinder finder = ModuleFinder.of(modulePaths.toArray(new
Path[0]));
+ Set<String> moduleNames = finder.findAll().stream()
+ .map(ref -> ref.descriptor().name())
+ .collect(Collectors.toSet());
+
+ if (moduleNames.isEmpty()) {
+ throw new ModuleLayerCreationException(
+ "Plugin " + plugin.getId() + " declares
<modular>true</modular> but none of its"
+ + " artifacts contain module descriptors
(module-info.class)."
+ + " Ensure the plugin JAR is compiled with a
module-info.java.");
+ }
+
+ // Parent layer: use the runtime layer from ClassWorld if
available (has Maven API
+ // modules + JLine), otherwise fall back to the boot layer.
+ // Flat hierarchy — all plugin layers are siblings under this
parent.
+ org.codehaus.plexus.classworlds.ClassWorld implWorld =
(org.codehaus.plexus.classworlds.ClassWorld) world;
+ ModuleLayer parentLayer =
+ implWorld.getModuleLayer() != null ?
implWorld.getModuleLayer() : ModuleLayer.boot();
+
+ // resolveAndBind: resolves modules and binds service providers,
+ // enabling ServiceLoader discovery within the plugin layer
+ Configuration cfg =
parentLayer.configuration().resolveAndBind(ModuleFinder.of(), finder,
moduleNames);
+
+ // One loader per plugin — isolation between plugins
+ ModuleLayer.Controller controller =
+ ModuleLayer.defineModulesWithOneLoader(cfg,
List.of(parentLayer), parent);
+
+ ModuleLayer pluginLayer = controller.layer();
+
+ // Store per-realm, not per-world
+ var implRealm = (org.codehaus.plexus.classworlds.realm.ClassRealm)
classRealm;
+ implRealm.setModuleLayer(pluginLayer, controller);
+
+ // Also populate the realm's classpath with the same artifacts
+ // so that ClassRealm.getURLs() returns meaningful results
+ // and resource loading works through the realm
+ populateRealm(
+ classRealm,
+ artifacts == null
+ ? List.of()
+ : artifacts.stream()
+ .filter(a -> a.getFile() != null &&
!isProvidedArtifact(a, true))
+ .map(ArtifactClassRealmConstituent::new)
+ .collect(Collectors.toList()));
+
+ // Apply META-INF/maven/module-access descriptors for access to
+ // boot-layer modules (e.g. java.base internals)
+ applyModuleAccessDescriptors(classRealm);
+
+ logger.debug(
+ "Created modular plugin realm {} with ModuleLayer
containing modules: {}", realmId, moduleNames);
+
+ return classRealm;
+
+ } catch (FindException | ResolutionException e) {
Review Comment:
⚠️ **F3: `LayerInstantiationException` not caught — unchecked exception
escapes**
`ModuleLayer.defineModulesWithOneLoader()` can throw
`LayerInstantiationException` (a `RuntimeException`) in addition to
`FindException` and `ResolutionException`. Common trigger: modules in different
layers have overlapping packages (e.g. shaded JARs). Since it extends
`RuntimeException`, it escapes the `catch` block unwrapped, bypassing
`ModuleLayerCreationException` wrapping and the `PluginContainerException` in
the caller.
```suggestion
} catch (FindException | ResolutionException |
LayerInstantiationException e) {
```
##########
compat/maven-plugin-api/src/main/java/org/apache/maven/plugin/descriptor/PluginDescriptor.java:
##########
@@ -467,6 +471,14 @@ public PluginDescriptor clone() {
}
}
+ public boolean isModular() {
Review Comment:
🔵 **F14: Missing `@since 4.1.0` Javadoc tag**
`isModular()` and `setModular()` are new public API in the compat module.
Add `@since 4.1.0` for API tracking.
```suggestion
/**
* @since 4.1.0
*/
public boolean isModular() {
```
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java:
##########
@@ -550,6 +573,24 @@ public <T> T getConfiguredMojo(Class<T> mojoInterface,
MavenSession session, Moj
}
}
+ /**
+ * 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)
{
Review Comment:
⚠️ **F8 (new code): Unguarded downcast of `ClassRealm` interface to impl
type**
`pluginRealm` is typed as `org.apache.maven.classrealm.ClassRealm`
(interface). The immediate downcast to
`org.codehaus.plexus.classworlds.realm.ClassRealm` (plexus impl) throws
`ClassCastException` if any alternative implementation is used. New code in
this PR should use `instanceof` pattern matching:
```suggestion
private static ClassLoader getEffectiveClassLoader(ClassRealm
pluginRealm) {
if (pluginRealm instanceof
org.codehaus.plexus.classworlds.realm.ClassRealm implRealm
&& implRealm.isModular()) {
ModuleLayer layer = implRealm.getModuleLayer();
String anyModuleName =
layer.modules().stream().findFirst().orElseThrow().getName();
return layer.findLoader(anyModuleName);
}
return pluginRealm.getClassLoader();
}
```
--
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]
