gnodet-bot commented on code in PR #13041:
URL: https://github.com/apache/maven/pull/13041#discussion_r4087797041
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultLifecycleBindingsInjector.java:
##########
@@ -182,6 +202,49 @@ protected void mergePluginContainer_Plugins(
}
}
+ private Plugin mergePluginManagement(Plugin lifecyclePlugin, Plugin
managedPlugin, boolean sourceDominant) {
+ List<PluginExecution> filtered =
managedPlugin.getExecutions().stream()
+ .filter(execution -> {
+ if (isFromSameLifecycle(lifecyclePlugin, execution)) {
+ return true;
+ }
+ if (problems != null) {
+ problems.add(
+ Severity.WARNING,
+ Version.BASE,
+ "Managed execution '" + execution.getId()
+ "' of plugin '"
+ + managedPlugin.getGroupId() + ":"
+ managedPlugin.getArtifactId()
+ + "' is bound to phase '" +
execution.getPhase()
+ + "' which belongs to a different
lifecycle than the one currently"
+ + " executing. The execution will
not run because the plugin is"
+ + " introduced only by lifecycle
bindings."
+ + " Declare the plugin in
<build><plugins> to apply all its"
+ + " managed executions
unconditionally, or set"
+ + "
-Dmaven.warn.crossLifecycleManagedExecution=false to suppress"
+ + " this warning.",
+ (InputLocation) null);
Review Comment:
📝 **Missing source location in warning:** The legacy
`LifecycleBindingsMerger` passes `execution.getLocation("")` so the warning
includes the exact POM file and line number where the filtered execution is
declared. Here, `(InputLocation) null` throws that context away.
`InputLocation` is already imported and `execution` is in scope — use the
location:
```suggestion
execution.getLocation(""));
```
##########
impl/maven-core/src/test/java/org/apache/maven/model/plugin/DefaultLifecycleBindingsInjectorTest.java:
##########
@@ -0,0 +1,244 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.model.plugin;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import org.apache.maven.api.Lifecycle;
+import org.apache.maven.api.services.LifecycleRegistry;
+import org.apache.maven.api.services.Lookup;
+import org.apache.maven.lifecycle.DefaultLifecycles;
+import org.apache.maven.lifecycle.LifeCyclePluginAnalyzer;
+import org.apache.maven.model.Build;
+import org.apache.maven.model.Dependency;
+import org.apache.maven.model.InputLocation;
+import org.apache.maven.model.InputSource;
+import org.apache.maven.model.Model;
+import org.apache.maven.model.Plugin;
+import org.apache.maven.model.PluginExecution;
+import org.apache.maven.model.PluginManagement;
+import org.codehaus.plexus.util.xml.Xpp3Dom;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+class DefaultLifecycleBindingsInjectorTest {
+
+ @Test
+ void mergePluginManagementOnlyActivatesExecutionsFromTheSameLifecycle() {
+ InputSource lifecycleSource = inputSource("lifecycle");
+ InputSource managementSource = inputSource("plugin-management");
+
+ PluginExecution lifecycleExecution = execution("default-clean",
"clean", lifecycleSource);
+ Plugin lifecyclePlugin = plugin("lifecycle-version",
lifecycleExecution, lifecycleSource);
+ lifecyclePlugin.setConfiguration(configuration("shared", "lifecycle",
"lifecycle", "default"));
+
+ PluginExecution sameLifecycleExecution = execution("managed-clean",
"clean", managementSource);
+ PluginExecution crossLifecycleExecution =
execution("managed-initialize", "initialize", managementSource);
+ PluginExecution defaultPhaseExecution =
execution("managed-default-phase", null, managementSource);
+ Plugin managedPlugin = plugin(
+ "managed-version",
+ managementSource,
+ sameLifecycleExecution,
+ crossLifecycleExecution,
+ defaultPhaseExecution);
+ managedPlugin.setConfiguration(configuration("shared", "managed",
"managed", "configured"));
+ managedPlugin.setExtensions(true);
+ managedPlugin.setInherited(false);
+ managedPlugin.addDependency(dependency("managed-dependency"));
+
+ Model target = modelWithPluginManagement(managedPlugin);
+ Model source = modelWithPlugin(lifecyclePlugin);
+
+ new DefaultLifecycleBindingsInjector.LifecycleBindingsMerger(
+ Map.of("clean", "clean", "initialize", "default"),
null)
Review Comment:
💡 **Warning emission is untested:** This test (and every other test in both
model implementations) passes `null` as the `problems` collector, so the
warning branch inside `mergePluginManagement` is never reached. Add a test that
supplies a capturing collector and verifies:
- A warning IS emitted for each cross-lifecycle filtered execution.
- The warning message contains the execution ID, plugin coordinates, and
filtered phase.
- No warning is emitted for same-lifecycle or null-phase executions.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Constants.java:
##########
@@ -575,6 +575,20 @@ public final class Constants {
@Config(type = "java.lang.Boolean", defaultValue = "false")
public static final String MAVEN_MAVEN3_PERSONALITY =
"maven.maven3Personality";
+ /**
+ * User property to suppress warnings about pluginManagement executions
that are filtered out because they are
+ * bound to a lifecycle phase that belongs to a different lifecycle than
the one being executed. Such executions
+ * are silently dropped when the plugin is introduced only by lifecycle
bindings (not explicitly declared in
+ * {@code <build><plugins>}). Set to {@code false} to suppress the warning
once you have reviewed and
Review Comment:
🔍 **Nit:** "once you have reviewed and accepted the behavior" is editorial —
the Javadoc should describe the semantics, not the user's decision process.
Suggest:
```suggestion
* User property to suppress warnings about pluginManagement executions
that are filtered out because they are
* bound to a lifecycle phase that belongs to a different lifecycle than
the one being executed. Such executions
* are silently dropped when the plugin is introduced only by lifecycle
bindings (not explicitly declared in
* {@code <build><plugins>}). Set to {@code false} to suppress the
warning,
```
--
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]
