gnodet-bot commented on code in PR #13582:
URL: https://github.com/apache/maven/pull/13582#discussion_r4308984290
##########
maven-core/src/main/java/org/apache/maven/lifecycle/DefaultLifecycles.java:
##########
@@ -132,24 +127,8 @@ public Map<String, Lifecycle> getPhaseToLifecycleMap() {
public List<Lifecycle> getLifeCycles() {
// ensure canonical order of standard lifecycles
- Map<String, Lifecycle> lifecycles = new LinkedHashMap<>();
-
- // filter by visibility (plexus vs sisu diff; "realms" are plexus
thing)
- // in some tests container is not injected
- if (this.plexusContainer != null) {
- for (String name : this.lifecycles.keySet()) {
- if (!isEnabled(name)) {
- continue;
- }
- try {
- lifecycles.put(name,
plexusContainer.lookup(Lifecycle.class, name));
- } catch (ComponentLookupException e) {
- // skip it
- }
- }
- } else {
- lifecycles.putAll(this.lifecycles);
- }
+ // take "snapshot": will filter by visibility
+ Map<String, Lifecycle> lifecycles = new
LinkedHashMap<>(this.lifecycles);
LinkedHashSet<String> lifecycleNames = new
LinkedHashSet<>(getStandardLifecycles());
lifecycleNames.addAll(lifecycles.keySet());
Review Comment:
⚠️ **Bug: `isEnabled()` filtering is lost, and the comment is misleading.**
The comment says "will filter by visibility" but no filtering happens here.
The Sisu visibility predicate configured in
`MavenCli.setVisibilityPredicateSupplierTransformer` operates at injection time
— it does NOT filter the `Map<String, Lifecycle>` snapshot taken on line 131.
More critically, `lifecycleNames.addAll(lifecycles.keySet())` on line 134
re-adds **all** lifecycle names (including those `getStandardLifecycles()` may
have excluded via `isEnabled()`), so the `maven.site.lifecycle.disabled` system
property no longer has any effect.
The old code explicitly called `isEnabled(name)` inside the loop to skip
disabled lifecycles. That guard needs to be preserved, e.g.:
```java
Map<String, Lifecycle> lifecycles = new LinkedHashMap<>();
this.lifecycles.forEach((name, lc) -> {
if (isEnabled(name)) {
lifecycles.put(name, lc);
}
});
```
The same issue applies symmetrically to
`DefaultLifecyclePluginAnalyzer.java` — the old Plexus-based filtering there
also served as a visibility gate; the new snapshot copies all mappings without
filtering.
##########
pom.xml:
##########
@@ -126,7 +126,7 @@ under the License.
<properties>
<javaVersion>8</javaVersion>
- <version.sisu-maven-plugin>1.1.0</version.sisu-maven-plugin>
+ <version.sisu-maven-plugin>1.1.1-SNAPSHOT</version.sisu-maven-plugin>
Review Comment:
⚠️ **SNAPSHOT dependency blocks merge.**
`sisu-maven-plugin 1.1.1-SNAPSHOT` is a pre-release artifact — it cannot be
used in a release or merged into a branch that may produce a release. Please
either wait for `1.1.1` to be released and use that, or restructure the PR so
the Sisu API used is available in the released `1.1.0`. The snapshot repository
blocks should also be removed before merge.
--
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]
