gnodet-bot commented on code in PR #13400:
URL: https://github.com/apache/maven/pull/13400#discussion_r4242807607
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/MojoExecutor.java:
##########
@@ -361,7 +361,18 @@ public void ensureDependenciesAreResolved(
MavenProject project = dependencyContext.getProject();
boolean aggregating = mojoDescriptor.isAggregator();
- if (dependencyContext.isResolutionRequiredForCurrentProject()) {
+ Collection<String> mojoScopesToCollect =
toScopes(mojoDescriptor.getDependencyCollectionRequired());
+ Collection<String> mojoScopesToResolve =
toScopes(mojoDescriptor.getDependencyResolutionRequired());
+
+ if (!mojoScopesToCollect.isEmpty() || !mojoScopesToResolve.isEmpty()) {
+ // Each mojo gets the graph of the scopes it asked for: a
dependency of a scope the mojo does not
+ // use, a test dependency for a compilation for instance, must not
mediate that graph.
+ // The results are cached per combination of scopes.
+ lifeCycleDependencyResolver.resolveProjectDependencies(
+ project, mojoScopesToCollect, mojoScopesToResolve,
session, aggregating, Collections.emptySet());
+
+ dependencyContext.synchronizeWithProjectState();
Review Comment:
⚠️ **Performance: unconditional resolution on every mojo invocation**
This path calls `resolveProjectDependencies()` and
`synchronizeWithProjectState()` for every mojo that declares non-empty scopes —
with no fast-path check at this call site for whether the exact scope
combination was already resolved for a prior mojo in the same build.
For a typical build with multiple compile-scope mojos (`compiler:compile`,
`resources:resources`, `checkstyle`, etc.), each invocation hits
`resolveProjectDependencies()` unconditionally. The heavy resolution work is
cached inside `projectArtifactsCache`, but `resolveProjectDependencies()` still
mutates `project.resolvedArtifacts`, iterates `project.dependencyArtifacts` to
set file references, and iterates reactor projects — on every call.
Consider tracking already-resolved scope combinations in `DependencyContext`
(similar to how it previously short-circuited via
`isResolutionRequiredForCurrentProject()`) so that repeated calls for the same
scope set can skip the project mutation entirely.
##########
impl/maven-core/src/main/java/org/apache/maven/project/DefaultProjectDependenciesResolver.java:
##########
@@ -140,6 +147,9 @@ public DependencyResolutionResult
resolve(DependencyResolutionRequest request)
art = art.setFile(null).setVersion(art.getBaseVersion());
dep = dep.setArtifact(art);
}
+ if (excludedScopes.contains(dep.getScope())) {
Review Comment:
🔧 **Inconsistent null-scope guard between the two branches**
The first branch (line ~119) uses:
```java
dependency.getScope() != null ? dependency.getScope() : "compile"
```
This branch (line 150) passes `dep.getScope()` directly — no null guard. The
Aether `Dependency` constructor normalizes `null` to `""`, so there's no NPE
risk in practice, but the asymmetry is confusing and fragile if that Aether
contract changes.
Suggest applying the same guard for consistency:
```suggestion
if (excludedScopes.contains(dep.getScope() != null ?
dep.getScope() : "compile")) {
```
--
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]
