gnodet-bot commented on code in PR #13494:
URL: https://github.com/apache/maven/pull/13494#discussion_r4495685268
##########
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: WARNING diagnostics silently deleted — user gets zero feedback on
directive failures**
The combined guard now silently returns when `source == null`,
`moduleLayerController == null`, or `isBootLayerModule(source)`. The 6
`WARNING`-level messages that explained *why* the directive was ignored are
gone with no replacement, not even `DEBUG`.
When a user typos a module name in `META-INF/maven/module-access`, they get
silence. This violates Maven's "warnings must be actionable" rule.
```suggestion
if (source == null || moduleLayerController == null) {
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}={1} on the JVM command
line instead",
sourceModuleName,
target.getName());
return;
}
```
##########
impl/maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -207,6 +218,8 @@ private ClassRealm createRealm(
populateRealm(classRealm, constituents);
+ applyModuleAccessDescriptors(classRealm);
Review Comment:
🟡 **F5: `applyModuleAccessDescriptors()` called for ALL realm types —
unnecessary overhead**
`createRealm()` is used for core, project, extension, and plugin realms.
Adding `applyModuleAccessDescriptors()` here scans
`META-INF/maven/module-access` on every classpath entry of every realm in the
build, not just plugin realms. The module-access descriptor is a
plugin-specific mechanism — it should only be applied when creating plugin
realms.
Move this call to `createPluginRealm()` / `createModularPluginRealm()`
instead of the shared `createRealm()`.
##########
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;
Review Comment:
🟡 **F4: Unguarded downcast — `ClassCastException` if a non-plexus
`ClassRealm` is injected**
Casting `ClassRealm` interface directly to the plexus impl without
`instanceof`. Any alternative implementation (test double, extension, future
refactor) causes an opaque `ClassCastException`.
```suggestion
if (!(classRealm instanceof
org.codehaus.plexus.classworlds.realm.ClassRealm implRealm)) {
throw new IllegalStateException("Expected plexus ClassRealm
impl, got: " + classRealm.getClass());
}
```
##########
impl/maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -364,4 +472,116 @@ private static Object getId(ClassLoader classLoader) {
}
return classLoader;
}
+
+ private static final String MODULE_ACCESS_DESCRIPTOR =
"META-INF/maven/module-access";
+
+ /**
+ * Reads {@code META-INF/maven/module-access} descriptors from the plugin
realm's classpath and
+ * applies the access directives they contain.
+ *
+ * <h4>Purpose</h4>
+ * <p>
+ * When a plugin (or extension) runs on the <em>classpath</em> (unnamed
module) it may need to
+ * call into packages of a <em>named</em> module that lives in Maven's
boot/runtime layer —
+ * for example JLine modules in {@code lib/modules/}, or any future Maven
API module that ships
+ * as a named module. Named modules do not export their internals to the
unnamed module by
+ * default; the JVM enforces this at runtime via strong encapsulation.
+ * </p>
+ * <p>
+ * The {@code META-INF/maven/module-access} file is a plugin-side escape
hatch: it lets the
+ * plugin declare which packages of which boot-layer modules it needs
opened or exported
+ * <strong>to itself</strong> (always {@code ALL-UNNAMED} as the target).
Maven applies these
+ * directives programmatically via {@link Module#addExports}/{@link
Module#addOpens} on startup
+ * of the plugin realm, before any plugin class is loaded.
+ * </p>
+ *
+ * <h4>Direction of access</h4>
+ * <p>
+ * Directives go from an <em>existing</em> named module in the
boot/runtime layer
+ * <strong>to</strong> the unnamed-module classloader of the plugin. In
other words:
+ * a plugin cannot use this mechanism to export its <em>own</em> packages;
it can only
+ * widen access from modules that Maven already owns (and that are loaded
before the plugin).
+ * </p>
+ *
+ * <h4>File format</h4>
+ * <p>Each non-blank, non-comment line is one of:</p>
+ * <pre>
+ * add-exports <module>/<package>=ALL-UNNAMED
+ * add-opens <module>/<package>=ALL-UNNAMED
+ * add-reads <module>
+ * </pre>
+ * <p>
+ * These mirror the JVM flags {@code --add-exports}, {@code --add-opens},
and
+ * {@code --add-reads} respectively. Lines starting with {@code #} are
comments.
+ * </p>
+ *
+ * <h4>Why not {@code module-info.java}?</h4>
+ * <p>
+ * The boot-layer modules (e.g. JLine) are compiled independently and
cannot reference
+ * the plugin's module name at compile time. Adding {@code exports … to
plugin.module} in
+ * their {@code module-info} would create an unacceptable coupling.
Programmatic
+ * {@code addExports}/{@code addOpens} is the standard JDK-endorsed
workaround for
+ * this exact scenario (see {@link java.lang.Module#addExports}).
+ * </p>
+ *
+ * @param classRealm the plugin class realm whose JAR(s) are scanned for
descriptors
+ */
+ private void applyModuleAccessDescriptors(ClassRealm classRealm) {
+ var implRealm = (org.codehaus.plexus.classworlds.realm.ClassRealm)
classRealm;
+ try {
+ Enumeration<URL> resources =
implRealm.getResources(MODULE_ACCESS_DESCRIPTOR);
+ while (resources.hasMoreElements()) {
+ URL resource = resources.nextElement();
+ try (BufferedReader reader =
+ new BufferedReader(new
InputStreamReader(resource.openStream(), StandardCharsets.UTF_8))) {
+ String line;
+ while ((line = reader.readLine()) != null) {
+ line = line.trim();
+ if (line.isEmpty() || line.startsWith("#")) {
+ continue;
+ }
+ applyModuleAccessDirective(implRealm, line);
+ }
+ }
+ }
+ } catch (IOException e) {
Review Comment:
🟡 **F6: Real `IOException` logged at `DEBUG` — silent failure for users**
A filesystem `IOException` reading `module-access` descriptors should be
`WARN`: the user's realm configuration was not applied and they have no way to
know. `DEBUG` is for trace-level noise that only developers enable.
```suggestion
logger.warn("Failed to read module-access descriptors for realm
{}", classRealm.getId(), e);
```
##########
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)
{
+ var implRealm = (org.codehaus.plexus.classworlds.realm.ClassRealm)
pluginRealm;
Review Comment:
🟡 **F8: Unguarded downcast in `getEffectiveClassLoader()`**
Same cast-without-`instanceof` pattern. If `pluginRealm` is not a plexus
`ClassRealm` impl (e.g. in a test harness), this throws `ClassCastException`
with no context.
```suggestion
if (!(pluginRealm instanceof
org.codehaus.plexus.classworlds.realm.ClassRealm implRealm)) {
return pluginRealm.getClassLoader();
}
```
##########
compat/maven-plugin-api/src/main/java/org/apache/maven/plugin/descriptor/PluginDescriptor.java:
##########
@@ -467,6 +471,14 @@ public PluginDescriptor clone() {
}
}
+ public boolean isModular() {
+ return modular;
+ }
+
+ public void setModular(boolean modular) {
Review Comment:
🟠**F14: Missing `@since` on new public API methods**
`isModular()` and `setModular()` are new public methods added to the compat
`PluginDescriptor`. They need a `@since 4.1.0` Javadoc tag so that API audits
and tooling can determine when they were introduced.
```suggestion
/**
* @since 4.1.0
*/
public boolean isModular() {
return modular;
}
/**
* @since 4.1.0
*/
public void setModular(boolean modular) {
this.modular = modular;
}
```
##########
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) {
+ throw new ModuleLayerCreationException(
+ "Failed to create module layer for plugin " +
plugin.getId()
Review Comment:
🔴 **F3: `LayerInstantiationException` not caught — raw `RuntimeException`
escapes to the user**
`ModuleLayer.defineModulesWithOneLoader()` can throw
`LayerInstantiationException` (a `RuntimeException`) for overlapping packages —
a common real-world failure with shaded JARs. It is not caught here, so it
escapes the entire `try/catch (FindException | ResolutionException)` block
unwrapped, bypassing the `ModuleLayerCreationException` →
`PluginContainerException` chain and producing a confusing raw stack trace with
no actionable message.
```suggestion
} catch (FindException | ResolutionException |
LayerInstantiationException e) {
throw new ModuleLayerCreationException(
"Failed to create module layer for plugin " +
plugin.getId()
+ " which declares <modular>true</modular>."
+ " Ensure all dependencies are module-path
compatible"
+ " (no split packages, valid module
descriptors): " + e.getMessage(),
e);
}
```
##########
impl/maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -364,4 +472,116 @@ private static Object getId(ClassLoader classLoader) {
}
return classLoader;
}
+
+ private static final String MODULE_ACCESS_DESCRIPTOR =
"META-INF/maven/module-access";
+
+ /**
+ * Reads {@code META-INF/maven/module-access} descriptors from the plugin
realm's classpath and
+ * applies the access directives they contain.
+ *
+ * <h4>Purpose</h4>
+ * <p>
+ * When a plugin (or extension) runs on the <em>classpath</em> (unnamed
module) it may need to
+ * call into packages of a <em>named</em> module that lives in Maven's
boot/runtime layer —
+ * for example JLine modules in {@code lib/modules/}, or any future Maven
API module that ships
+ * as a named module. Named modules do not export their internals to the
unnamed module by
+ * default; the JVM enforces this at runtime via strong encapsulation.
+ * </p>
+ * <p>
+ * The {@code META-INF/maven/module-access} file is a plugin-side escape
hatch: it lets the
+ * plugin declare which packages of which boot-layer modules it needs
opened or exported
+ * <strong>to itself</strong> (always {@code ALL-UNNAMED} as the target).
Maven applies these
+ * directives programmatically via {@link Module#addExports}/{@link
Module#addOpens} on startup
+ * of the plugin realm, before any plugin class is loaded.
+ * </p>
+ *
+ * <h4>Direction of access</h4>
+ * <p>
+ * Directives go from an <em>existing</em> named module in the
boot/runtime layer
+ * <strong>to</strong> the unnamed-module classloader of the plugin. In
other words:
+ * a plugin cannot use this mechanism to export its <em>own</em> packages;
it can only
+ * widen access from modules that Maven already owns (and that are loaded
before the plugin).
+ * </p>
+ *
+ * <h4>File format</h4>
+ * <p>Each non-blank, non-comment line is one of:</p>
+ * <pre>
+ * add-exports <module>/<package>=ALL-UNNAMED
+ * add-opens <module>/<package>=ALL-UNNAMED
+ * add-reads <module>
+ * </pre>
+ * <p>
+ * These mirror the JVM flags {@code --add-exports}, {@code --add-opens},
and
+ * {@code --add-reads} respectively. Lines starting with {@code #} are
comments.
+ * </p>
+ *
+ * <h4>Why not {@code module-info.java}?</h4>
+ * <p>
+ * The boot-layer modules (e.g. JLine) are compiled independently and
cannot reference
+ * the plugin's module name at compile time. Adding {@code exports … to
plugin.module} in
+ * their {@code module-info} would create an unacceptable coupling.
Programmatic
+ * {@code addExports}/{@code addOpens} is the standard JDK-endorsed
workaround for
+ * this exact scenario (see {@link java.lang.Module#addExports}).
+ * </p>
+ *
+ * @param classRealm the plugin class realm whose JAR(s) are scanned for
descriptors
+ */
+ private void applyModuleAccessDescriptors(ClassRealm classRealm) {
+ var implRealm = (org.codehaus.plexus.classworlds.realm.ClassRealm)
classRealm;
Review Comment:
🟡 **F7: Unguarded downcast — same issue as F4**
Same pattern as the downcast at line 336. Add an `instanceof` pattern match.
##########
impl/maven-classworlds/src/main/java/org/codehaus/plexus/classworlds/ClassWorld.java:
##########
@@ -123,12 +125,13 @@ public synchronized ClassRealm newRealm(String id,
ClassLoader classLoader, Pred
*/
@Override
public synchronized void close() throws IOException {
- realms.values().forEach(this::disposeRealm);
+ realms.values().stream().forEach(this::disposeRealm);
Review Comment:
🟠**F12: Redundant `.stream()` intermediary — regression from pre-PR code**
The original code was `realms.values().forEach(this::disposeRealm)` (direct
`Collection.forEach`). This commit replaced it with
`realms.values().stream().forEach(...)`, which creates an unnecessary `Stream`
object on every close.
```suggestion
realms.values().forEach(this::disposeRealm);
```
--
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]
