gnodet-bot commented on code in PR #13494:
URL: https://github.com/apache/maven/pull/13494#discussion_r4357851550
##########
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)) {
return;
}
moduleLayerController.addReads(source, target);
}
private void applyModuleAccess(String moduleName, String packageName,
Module target, boolean open) {
- String directive = open ? "add-opens" : "add-exports";
Module source = findModule(moduleName);
- if (source == null) {
- LOGGER.log(
- System.Logger.Level.WARNING,
- "module-access: module ''{0}'' not found, ignoring {1}
{0}/{2}",
- moduleName,
- directive,
- packageName);
- return;
- }
- if (moduleLayerController == null) {
- LOGGER.log(
- System.Logger.Level.WARNING,
- "module-access: no ModuleLayer controller available,
cannot {0} {1}/{2}",
- directive,
- moduleName,
- packageName);
- 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 --{1} {0}/{2}=ALL-UNNAMED on the JVM
command line instead",
- moduleName,
- directive,
- packageName);
+ if (source == null || moduleLayerController == null ||
isBootLayerModule(source)) {
Review Comment:
🔴 Same issue as in `addReads()`: all three failure conditions silently
return. The previous `applyModuleAccess()` implementation logged `WARNING` for
each case. Please restore the diagnostic logging.
##########
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:
🔴 **Usability regression:** Three distinct failure conditions (`source ==
null`, `moduleLayerController == null`, `isBootLayerModule(source)`) are
silently swallowed. The previous code logged each as `WARNING` with an
actionable message. Without the diagnostic, users who misconfigure
`module-access` directives will get no feedback.
Consider at minimum:
```java
if (source == null) {
logger.warn("module-access: module '{}' not found, ignoring add-reads",
sourceModuleName);
return;
}
if (moduleLayerController == null) {
logger.warn("module-access: no ModuleLayer controller, cannot add-reads
{}", sourceModuleName);
return;
}
if (isBootLayerModule(source)) {
logger.warn("module-access: module '{}' is in boot layer; use
--add-reads on JVM command line", sourceModuleName);
return;
}
```
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java:
##########
@@ -378,20 +379,18 @@ public void setupPluginRealm(
foreignImports,
filter,
project.getRemotePluginRepositories(),
- session.getRepositorySession());
+ session.getRepositorySession(),
+ pluginDescriptor.isModular());
PluginRealmCache.CacheRecord cacheRecord =
pluginRealmCache.get(cacheKey, () -> {
createPluginRealm(pluginDescriptor, session, parent,
foreignImports, filter);
return new PluginRealmCache.CacheRecord(
- pluginDescriptor.getClassRealm(),
- pluginDescriptor.getArtifacts(),
- pluginDescriptor.getDependencyNode());
+ pluginDescriptor.getClassRealm(),
pluginDescriptor.getArtifacts());
Review Comment:
🔴 **Regression (reverts #13307):** The `CacheRecord` is now constructed with
only `classRealm` and `artifacts`, dropping `dependencyNode`. On a cache hit
(when `pluginRealmCache.get(cacheKey, supplier)` returns a cached record
without invoking the supplier), `pluginDescriptor.setDependencyNode()` is never
called, leaving `getDependencyNode()` as `null`.
The previous code restored it here:
```java
// removed:
pluginDescriptor.setDependencyNode(cacheRecord.getDependencyNode());
```
Please restore `dependencyNode` to `CacheRecord` and reinstate the cache-hit
restoration.
##########
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:
🔴 **Missing exception type:** `ModuleLayer.defineModulesWithOneLoader()` can
throw `LayerInstantiationException` (e.g., when modules have overlapping
packages). `LayerInstantiationException` is a `RuntimeException` that does not
extend `FindException` or `ResolutionException`, so it will escape this `catch`
block as an unchecked exception rather than being wrapped in
`ModuleLayerCreationException`.
```suggestion
} catch (FindException | ResolutionException |
LayerInstantiationException e) {
```
##########
impl/maven-classworlds/src/main/java/org/codehaus/plexus/classworlds/realm/ClassRealm.java:
##########
@@ -64,9 +66,9 @@
*/
public class ClassRealm extends URLClassLoader {
- private final ClassWorld world;
+ private ClassWorld world;
Review Comment:
🟡 **Identity fields made non-final:** Both `world` and `id` (line 71) were
`final` — they define the identity of a `ClassRealm`. No setters are added.
What is the justification for removing `final`? If no mutability is needed,
please restore `final` to preserve the immutability guarantee.
##########
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:
🟡 **Unnecessary overhead for non-plugin realms:**
`applyModuleAccessDescriptors()` scans `META-INF/maven/module-access` in every
class realm — project, extension, and plugin. Only plugin realms are ever
modular. Consider guarding with a realm-type check or a fast
`implRealm.isModular()` short-circuit before scanning.
##########
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;
Review Comment:
🟡 **Unguarded downcast:** `world` is typed as `ClassWorld` (interface/API
type); this cast to the plexus-classworlds impl type will throw
`ClassCastException` if a custom `ClassWorld` is injected (e.g., by an
embedder). Consider adding an `instanceof` guard:
```java
if (!(world instanceof org.codehaus.plexus.classworlds.ClassWorld
implWorld)) {
// fallback: no runtime layer available
return null;
}
```
Same pattern applies in `DefaultClassRealmManager` line 530 and
`DefaultMavenPluginManager` line 579.
##########
impl/maven-classworlds/src/main/java/org/codehaus/plexus/classworlds/realm/ClassRealm.java:
##########
@@ -76,6 +78,14 @@ public class ClassRealm extends URLClassLoader {
private ClassLoader parentClassLoader;
+ private ModuleLayer moduleLayer;
+
+ private ModuleLayer.Controller moduleLayerController;
+
+ private static final boolean IS_PARALLEL_CAPABLE =
Closeable.class.isAssignableFrom(URLClassLoader.class);
Review Comment:
🟠**Dead code:** `Closeable.class.isAssignableFrom(URLClassLoader.class)` is
unconditionally `true` since Java 7. Maven 4 requires Java 21+. The field and
all `IS_PARALLEL_CAPABLE` branches can be simplified to unconditional
`registerAsParallelCapable()` and the `lockMap` initialized without the ternary.
##########
impl/maven-classworlds/src/main/java/org/codehaus/plexus/classworlds/realm/ClassRealm.java:
##########
@@ -76,6 +78,14 @@ public class ClassRealm extends URLClassLoader {
private ClassLoader parentClassLoader;
+ private ModuleLayer moduleLayer;
+
+ private ModuleLayer.Controller moduleLayerController;
+
+ private static final boolean IS_PARALLEL_CAPABLE =
Closeable.class.isAssignableFrom(URLClassLoader.class);
+
+ private final ConcurrentMap<String, Object> lockMap;
Review Comment:
🟠**Redundant and unbounded custom lock map:** After
`registerAsParallelCapable()`, the JDK's `ClassLoader` already provides
per-class-name locking internally — overriding `getClassLoadingLock()` with a
custom `ConcurrentHashMap` reimplements identical behavior. Additionally,
entries are never evicted (loaded class entries accumulate indefinitely),
making this an unbounded memory leak. Please remove the override and rely on
the JDK's built-in parallel class-loading support.
##########
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:
🔵 **Unnecessary `.stream()` wrapper:** `Collection.forEach()` suffices here.
```suggestion
realms.values().forEach(this::disposeRealm);
```
##########
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:
🔵 **Missing `@since` tag:** Same as `isModular()` — please add `@since
4.1.0`.
##########
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:
🔵 **Missing `@since` tag:** The `.mdo` source annotates this field with
`@since Maven 4.1.0`, but the Java method lacks the conventional Javadoc tag.
```suggestion
/**
* @since 4.1.0
*/
public boolean isModular() {
```
##########
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) {
+ logger.debug("Failed to read module-access descriptors for realm
{}", classRealm.getId(), e);
Review Comment:
🟠**Log level too low:** An `IOException` reading module-access descriptors
indicates a real I/O failure (corrupted JAR, permission denied), not a normal
condition. At `DEBUG`, production users will never see it.
```suggestion
logger.warn("Failed to read module-access descriptors for realm
{}", classRealm.getId(), e);
```
--
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]
