gnodet-bot commented on code in PR #13350:
URL: https://github.com/apache/maven/pull/13350#discussion_r4301938748
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultPluginDependenciesResolver.java:
##########
@@ -75,17 +82,42 @@
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 static final Set<String> EXCLUDED_FROM_CORE_MANAGED_DEPENDENCIES =
Set.of(
+ "org.slf4j:slf4j-api",
+ "javax.inject:javax.inject",
+ "javax.annotation:javax.annotation-api",
+ "org.fusesource.jansi:jansi",
Review Comment:
💡 **Nit: exclusion list needs rationale comments.**
`EXCLUDED_FROM_CORE_MANAGED_DEPENDENCIES` contains 5 artifacts with no
explanation for why each is excluded. Without comments, future maintainers have
no way to know:
- Why `slf4j-api`? (different from Maven core SLF4J API? licensing?)
- Why `javax.inject` and `javax.annotation-api`? (provided by the DI
container?)
- Why `jansi`? (ANSI library — is this expected to conflict?)
- Why `plexus-interactivity-api`?
Add a brief rationale comment per entry (or a block comment above the set).
If these are excluded because they're "API contracts that plugins should be
able to choose their own version of" vs. "implementation artifacts that must
match the running core," say so.
```suggestion
/**
* Core-exported artifacts excluded from managed dependency injection.
* These are third-party API artifacts that plugins may legitimately
depend on
* at a version different from the one bundled with Maven core (e.g.,
SLF4J API,
* CDI/injection APIs). Constraining them to the core version would
prevent
* plugins from using a newer API version that remains
backward-compatible.
*/
private static final Set<String> EXCLUDED_FROM_CORE_MANAGED_DEPENDENCIES
= Set.of(
"org.slf4j:slf4j-api", // SLF4J API — plugins may
use a newer compatible version
"javax.inject:javax.inject", // JSR-330 injection API
— ABI stable, not version-sensitive
"javax.annotation:javax.annotation-api", // JSR-250 — ABI stable
"org.fusesource.jansi:jansi", // ANSI console — not
part of the classloading contract
"org.codehaus.plexus:plexus-interactivity-api"); // Plugin
interaction API
```
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultPluginDependenciesResolver.java:
##########
@@ -271,6 +304,18 @@ private DependencyResult resolveInternal(
request.setRequestContext(REPOSITORY_CONTEXT);
request.setRepositories(repositories);
request.setRoot(new
org.eclipse.aether.graph.Dependency(pluginArtifact, null));
+ Map<String, org.eclipse.aether.graph.Dependency> core =
+ injectCoreManagedDeps ? getCoreDependenciesMap(session) :
Collections.emptyMap();
+ if (injectCoreManagedDeps && !core.isEmpty()) {
+ Set<String> explicitDeps = plugin.getDependencies().stream()
Review Comment:
⚠️ **Design question: should explicit plugin deps on core artifacts be
excluded or aligned?**
The current code excludes explicit plugin deps from managed dependency
injection (any artifact the plugin explicitly declared is skipped). This means
a plugin declaring `maven-core:3.9.0` will **keep** its declared 3.9.0 version
rather than being aligned to the running Maven core version.
**Contrast with PR #2000 (cstamas, who filed MNG-8451):** that
implementation explicitly aligns *all* deps that match core artifacts —
including explicit ones — overriding both version and scope to `provided`.
**The question:** is it correct to leave explicit core deps unmanaged? There
are two valid positions:
1. **Align all** (PR #2000 approach): plugin declared `maven-core:3.9.0` →
override to current core version + provided scope. Prevents version conflicts
at runtime.
2. **Align only transitive** (this PR): explicit deps are intentional, don't
override them. Less surprising for plugin authors who pinned a specific version.
Position 2 (this PR) risks a scenario where a plugin declares an old
`maven-core:3.9.0` and gets it on its classpath alongside the running Maven 4.x
core — which is exactly the version conflict MNG-8451 aims to prevent. Could
you clarify the intended behavior per MNG-8451? If position 2 is intentional,
the Javadoc and PR description should explicitly explain the rationale.
--
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]
