gnodet-bot commented on code in PR #13798:
URL: https://github.com/apache/maven/pull/13798#discussion_r4450519007
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutor.java:
##########
@@ -681,6 +723,50 @@ private void plan() {
}
}
+ private boolean inheritedFromReactorAncestor(
+ MavenProject project, Plugin plugin, PluginExecution
execution, String goal, String phase) {
+ if (declaredInProject(project, plugin, execution, goal)) {
+ return false;
+ }
+ for (MavenProject parent = project.getParent(); parent != null;
parent = parent.getParent()) {
+ if (plan.getAllProjects().containsKey(parent)
+ && plan.step(parent, phase).filter(step ->
!step.skip.get()).isPresent()
+ && parent.getBuild().getPlugins().stream()
+ .filter(p ->
p.getKey().equals(plugin.getKey()))
+ .flatMap(p -> p.getExecutions().stream())
+ .anyMatch(e ->
e.getId().equals(execution.getId())
+ && e.getGoals().contains(goal)
+ &&
java.util.Objects.equals(e.getPhase(), execution.getPhase()))) {
Review Comment:
**Style:** Use a top-level import instead of the fully-qualified name. No
naming collision exists in this file — `Objects` is unambiguous.
```suggestion
&& Objects.equals(e.getPhase(),
execution.getPhase()))) {
```
Also add `import java.util.Objects;` to the import section.
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutor.java:
##########
@@ -681,6 +723,50 @@ private void plan() {
}
}
+ private boolean inheritedFromReactorAncestor(
+ MavenProject project, Plugin plugin, PluginExecution
execution, String goal, String phase) {
+ if (declaredInProject(project, plugin, execution, goal)) {
+ return false;
+ }
+ for (MavenProject parent = project.getParent(); parent != null;
parent = parent.getParent()) {
+ if (plan.getAllProjects().containsKey(parent)
+ && plan.step(parent, phase).filter(step ->
!step.skip.get()).isPresent()
+ && parent.getBuild().getPlugins().stream()
+ .filter(p ->
p.getKey().equals(plugin.getKey()))
+ .flatMap(p -> p.getExecutions().stream())
+ .anyMatch(e ->
e.getId().equals(execution.getId())
+ && e.getGoals().contains(goal)
+ &&
java.util.Objects.equals(e.getPhase(), execution.getPhase()))) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ private boolean declaredInProject(MavenProject project, Plugin plugin,
PluginExecution execution, String goal) {
+ var model = project.getOriginalModel();
+ if (model == null) {
+ return false;
+ }
+ Stream<Plugin> plugins = model.getBuild() == null
+ ? Stream.empty()
+ : model.getBuild().getPlugins().stream();
+ Set<String> activeProfiles = project.getActiveProfiles() == null
+ ? Set.of()
+ : project.getActiveProfiles().stream()
+ .map(org.apache.maven.model.Profile::getId)
Review Comment:
**Style:** Same issue — `org.apache.maven.model.Profile` should be a
top-level import. No `Profile` collision exists in this file.
```suggestion
.map(Profile::getId)
```
Also add `import org.apache.maven.model.Profile;` to the import section.
##########
its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng13763ConcurrentAggregatorTest.java:
##########
@@ -0,0 +1,134 @@
+/*
+ * 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.it;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+
+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.junit.jupiter.api.Assertions.assertThrows;
+
+public class MavenITmng13763ConcurrentAggregatorTest extends
AbstractMavenIntegrationTestCase {
+
+ @Test
+ public void testAggregatorWaitsForChildren() throws Exception {
+ Path testDir = extractResources("mng-13763");
+ installPlugin(testDir);
+
+ Verifier verifier = newCleanVerifier(testDir.resolve("reactor"), true);
+ verifier.addCliArgument("-b");
+ verifier.addCliArgument("concurrent");
+ verifier.addCliArgument("-T2");
+ verifier.addCliArgument("verify");
+ verifier.execute();
+ verifier.verifyErrorFreeLog();
+
+ assertEquals(
+ "a=true b=true aa=true",
+
Files.readString(testDir.resolve("reactor/target/aggregate-observed.txt")).trim());
+
assertFalse(Files.exists(testDir.resolve("reactor/a/target/aggregate-observed.txt")));
+
assertFalse(Files.exists(testDir.resolve("reactor/a/aa/target/aggregate-observed.txt")));
+
assertFalse(Files.exists(testDir.resolve("reactor/b/target/aggregate-observed.txt")));
+ }
+
+ @Test
+ public void testAggregatorDoesNotRunAfterChildFailure() throws Exception {
+ Path testDir = extractResources("mng-13763");
+ installPlugin(testDir);
+
+ Verifier verifier = newCleanVerifier(testDir.resolve("reactor"), true);
+ verifier.addCliArgument("-b");
+ verifier.addCliArgument("concurrent");
+ verifier.addCliArgument("-T2");
+ verifier.addCliArgument("--fail-at-end");
+ verifier.addCliArgument("-DfailModule=b");
+ verifier.addCliArgument("package");
+ assertThrows(VerificationException.class, verifier::execute);
+
+
assertFalse(Files.exists(testDir.resolve("reactor/target/aggregate-observed.txt")));
+ }
+
+ @Test
+ public void testChildProfileAggregatorIsPreserved() throws Exception {
+ Path testDir = extractResources("mng-13763");
+ installPlugin(testDir);
+
+ Verifier verifier = newCleanVerifier(testDir.resolve("reactor"), true);
+ verifier.addCliArgument("-b");
+ verifier.addCliArgument("concurrent");
+ verifier.addCliArgument("-T2");
+ verifier.addCliArgument("-Pchild-aggregate");
+ verifier.addCliArgument("-DexpectChildAggregator=true");
+ verifier.addCliArgument("package");
+ verifier.execute();
+ verifier.verifyErrorFreeLog();
+
+ assertEquals(
+ "a=true b=true aa=true",
+
Files.readString(testDir.resolve("reactor/target/aggregate-observed.txt")).trim());
+ assertEquals(
+ "a=false b=false aa=true",
+
Files.readString(testDir.resolve("reactor/a/target/aggregate-observed.txt")).trim());
+ }
+
+ @Test
+ public void testManagedAggregatorRunsInModules() throws Exception {
+ Path testDir = extractResources("mng-13763");
+ installPlugin(testDir);
+
+ Verifier verifier = newCleanVerifier(testDir.resolve("managed"),
false);
+ verifier.addCliArgument("-b");
+ verifier.addCliArgument("concurrent");
+ verifier.addCliArgument("-T2");
+ verifier.addCliArgument("-DrequireOwnTouch=true");
+ verifier.addCliArgument("package");
+ verifier.execute();
+ verifier.verifyErrorFreeLog();
+
+
assertFalse(Files.exists(testDir.resolve("managed/target/aggregate-observed.txt")));
+ assertEquals(
+ "a=false b=false aa=false",
+
Files.readString(testDir.resolve("managed/a/target/aggregate-observed.txt")).trim());
+ assertEquals(
+ "a=false b=false aa=false",
+
Files.readString(testDir.resolve("managed/b/target/aggregate-observed.txt")).trim());
+ }
+
+ private void installPlugin(Path testDir) throws Exception {
+ Verifier verifier = newVerifier(testDir.resolve("plugin"));
+ verifier.addCliArgument("install");
+ verifier.execute();
+ verifier.verifyErrorFreeLog();
+ }
+
+ private Verifier newCleanVerifier(Path reactorDir, boolean nested) throws
IOException, VerificationException {
+ Verifier verifier = newVerifier(reactorDir);
+ for (String path : new String[] {"target", "a/target", "b/target"}) {
+ verifier.deleteDirectory(path);
+ }
+ if (nested) {
+ verifier.deleteDirectory("a/aa/target");
+ }
+ return verifier;
+ }
+}
Review Comment:
**Missing test — partial build scenario.** The code has an explicit guard
for this case:
```java
// Only suppress inherited copies when their declaring ancestor is
// part of the reactor; partial builds still run the selected goal.
if (plan.getAllProjects().containsKey(parent))
```
The comment says partial builds are handled, but there is no IT verifying
it. A partial build like `-pl :a -b concurrent -T2` where the root has an
aggregator should preserve the aggregator in module `a`. Without a test, a
regression here would silently suppress aggregators in partial builds and go
undetected.
--
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]
