gnodet-bot commented on code in PR #13350:
URL: https://github.com/apache/maven/pull/13350#discussion_r4187265737
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultPluginDependenciesResolver.java:
##########
@@ -271,6 +290,7 @@ private DependencyResult resolveInternal(
request.setRequestContext(REPOSITORY_CONTEXT);
request.setRepositories(repositories);
request.setRoot(new
org.eclipse.aether.graph.Dependency(pluginArtifact, null));
+ request.setManagedDependencies(getManagedDependencies(session));
Review Comment:
🔴 **Design concern: core extension path affected.** `resolveInternal` is
called by both `resolvePluginAndFlatten` (for plugins) and
`resolveCoreExtensionAndFlatten` (for core extensions). Injecting core managed
dependencies unconditionally here means core extensions also get them — but
core extensions load *before* Maven core is fully initialized, and their
dependency resolution semantics may differ.
Consider whether this should only apply to the plugin resolution path, not
the core extension path. The competing PR #2000 by @cstamas has the same
behavior, so this may be intentional — but it deserves an explicit comment
explaining why.
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultPluginDependenciesResolver.java:
##########
@@ -307,4 +327,56 @@ private DependencyResult resolveInternal(
RequestTraceHelper.exit(trace);
}
}
+
+ @SuppressWarnings("unchecked")
+ private List<org.eclipse.aether.graph.Dependency>
getManagedDependencies(RepositorySystemSession session) {
+ if (coreExports == null) {
+ return Collections.emptyList();
+ }
+ return (List<org.eclipse.aether.graph.Dependency>)
+ session.getData().computeIfAbsent(MANAGED_DEPENDENCIES_KEY,
this::computeManagedDependencies);
+ }
+
+ private List<org.eclipse.aether.graph.Dependency>
computeManagedDependencies() {
+ if (coreExports == null || coreExports.getExportedArtifacts() == null)
{
+ return Collections.emptyList();
+ }
+ List<org.eclipse.aether.graph.Dependency> managed = new ArrayList<>();
+ ClassLoader cl = Thread.currentThread().getContextClassLoader();
+ if (cl == null) {
+ cl = getClass().getClassLoader();
+ }
Review Comment:
⚠️ **Fragile ClassLoader selection.** Using
`Thread.currentThread().getContextClassLoader()` (TCCL) in Maven is unreliable
— the TCCL may be a plugin realm, not the core realm, depending on the
execution context. This means `pom.properties` lookups could find the wrong
versions or fail to find them entirely.
Compare with PR #2000 by @cstamas, which uses
`coreExports.getExportedPackages()` to get the core realm classloader directly
— a more robust approach since `CoreExports` already holds a reference to the
correct classloader.
```suggestion
ClassLoader cl = coreExports.getExportedPackages().values().stream()
.findFirst().orElse(getClass().getClassLoader());
```
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultPluginDependenciesResolver.java:
##########
@@ -307,4 +327,56 @@ private DependencyResult resolveInternal(
RequestTraceHelper.exit(trace);
}
}
+
+ @SuppressWarnings("unchecked")
+ private List<org.eclipse.aether.graph.Dependency>
getManagedDependencies(RepositorySystemSession session) {
+ if (coreExports == null) {
+ return Collections.emptyList();
+ }
+ return (List<org.eclipse.aether.graph.Dependency>)
+ session.getData().computeIfAbsent(MANAGED_DEPENDENCIES_KEY,
this::computeManagedDependencies);
+ }
+
+ private List<org.eclipse.aether.graph.Dependency>
computeManagedDependencies() {
+ if (coreExports == null || coreExports.getExportedArtifacts() == null)
{
+ return Collections.emptyList();
+ }
+ List<org.eclipse.aether.graph.Dependency> managed = new ArrayList<>();
+ ClassLoader cl = Thread.currentThread().getContextClassLoader();
+ if (cl == null) {
+ cl = getClass().getClassLoader();
+ }
+ for (String ga : coreExports.getExportedArtifacts()) {
+ int idx = ga.indexOf(':');
+ if (idx <= 0 || idx >= ga.length() - 1) {
+ continue;
+ }
+ String groupId = ga.substring(0, idx);
+ String artifactId = ga.substring(idx + 1);
+ String resource = "META-INF/maven/" + groupId + "/" + artifactId +
"/pom.properties";
+ Properties props = new Properties();
+ try (InputStream is = cl.getResourceAsStream(resource)) {
+ if (is != null) {
+ props.load(is);
+ } else if (cl != getClass().getClassLoader()) {
+ try (InputStream fallbackIs =
getClass().getClassLoader().getResourceAsStream(resource)) {
+ if (fallbackIs != null) {
+ props.load(fallbackIs);
+ }
+ }
+ }
+ } catch (IOException e) {
+ logger.debug("Could not read " + resource, e);
Review Comment:
💡 **Nit:** Use parameterized logging instead of string concatenation.
```suggestion
logger.debug("Could not read {}", resource, e);
```
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultPluginDependenciesResolver.java:
##########
@@ -75,17 +81,30 @@
public class DefaultPluginDependenciesResolver implements
PluginDependenciesResolver {
private static final String REPOSITORY_CONTEXT =
org.apache.maven.api.services.RequestTrace.CONTEXT_PLUGIN;
+ private static final String MANAGED_DEPENDENCIES_KEY =
+ DefaultPluginDependenciesResolver.class.getName() +
".managedDependencies";
+
private final Logger logger = LoggerFactory.getLogger(getClass());
private final RepositorySystem repoSystem;
private final List<MavenPluginDependenciesValidator>
dependenciesValidators;
- @Inject
+ private final CoreExports coreExports;
+
public DefaultPluginDependenciesResolver(
RepositorySystem repoSystem,
List<MavenPluginDependenciesValidator> dependenciesValidators) {
+ this(repoSystem, dependenciesValidators, null);
+ }
Review Comment:
⚠️ **Backward-compat constructor.** This two-arg constructor with
`coreExports=null` means the feature is silently disabled when used outside of
DI contexts. Consider adding `@Deprecated` and a Javadoc explaining that the
three-arg constructor (via `@Inject`) should be preferred.
Also, `@Inject` on the three-arg constructor with a non-optional
`CoreExports` parameter is risky — if `CoreExports` is not available in the DI
container, injection will fail at startup. Consider using `@Nullable` on the
`coreExports` parameter (as done in PR #2000).
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultPluginDependenciesResolver.java:
##########
@@ -307,4 +327,56 @@ private DependencyResult resolveInternal(
RequestTraceHelper.exit(trace);
}
}
+
+ @SuppressWarnings("unchecked")
+ private List<org.eclipse.aether.graph.Dependency>
getManagedDependencies(RepositorySystemSession session) {
+ if (coreExports == null) {
+ return Collections.emptyList();
+ }
+ return (List<org.eclipse.aether.graph.Dependency>)
+ session.getData().computeIfAbsent(MANAGED_DEPENDENCIES_KEY,
this::computeManagedDependencies);
+ }
+
+ private List<org.eclipse.aether.graph.Dependency>
computeManagedDependencies() {
+ if (coreExports == null || coreExports.getExportedArtifacts() == null)
{
+ return Collections.emptyList();
+ }
+ List<org.eclipse.aether.graph.Dependency> managed = new ArrayList<>();
+ ClassLoader cl = Thread.currentThread().getContextClassLoader();
+ if (cl == null) {
+ cl = getClass().getClassLoader();
+ }
+ for (String ga : coreExports.getExportedArtifacts()) {
+ int idx = ga.indexOf(':');
+ if (idx <= 0 || idx >= ga.length() - 1) {
+ continue;
+ }
+ String groupId = ga.substring(0, idx);
+ String artifactId = ga.substring(idx + 1);
+ String resource = "META-INF/maven/" + groupId + "/" + artifactId +
"/pom.properties";
+ Properties props = new Properties();
+ try (InputStream is = cl.getResourceAsStream(resource)) {
+ if (is != null) {
+ props.load(is);
+ } else if (cl != getClass().getClassLoader()) {
+ try (InputStream fallbackIs =
getClass().getClassLoader().getResourceAsStream(resource)) {
+ if (fallbackIs != null) {
+ props.load(fallbackIs);
+ }
+ }
+ }
+ } catch (IOException e) {
+ logger.debug("Could not read " + resource, e);
+ }
+ String version = props.getProperty("version");
+ if (version != null) {
+ version = version.trim();
+ if (!version.isEmpty() && !version.startsWith("${")) {
+ Artifact artifact = new DefaultArtifact(groupId,
artifactId, "jar", version);
+ managed.add(new
org.eclipse.aether.graph.Dependency(artifact, DependencyScope.PROVIDED.id()));
Review Comment:
💡 **Documentation gap.** The `DependencyScope.PROVIDED` choice is the crux
of this entire PR and deserves a comment explaining the rationale: core
artifacts are set as `provided` managed dependencies so that (1) their versions
are pinned to the running Maven core version, and (2) the
`ScopeDependencyFilter("provided", "test")` in `resolveInternal` excludes them
from plugin artifact resolution — since they're already available through
Maven's core classloader.
Without this comment, the next person reading this code will have to
reverse-engineer the interaction between managed deps scope and the resolution
filter.
--
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]
