gnodet-bot commented on code in PR #13647:
URL: https://github.com/apache/maven/pull/13647#discussion_r4324046973
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/LifecycleModuleBuilder.java:
##########
@@ -103,23 +107,34 @@ public void buildProject(
MavenExecutionPlan executionPlan =
builderCommon.resolveBuildPlan(session, currentProject,
taskSegment, new HashSet<>());
- List<MojoExecution> mojoExecutions =
executionPlan.getMojoExecutions();
+ mojoExecutions = executionPlan.getMojoExecutions();
projectExecutionListener.beforeProjectLifecycleExecution(
new ProjectExecutionEvent(session, currentProject,
mojoExecutions));
+ LifecycleParticipantDispatcher.beforeProjectExecution(session,
currentProject, mojoExecutions);
mojoExecutor.execute(session, mojoExecutions);
Instant buildEndTime = MonotonicClock.now();
projectExecutionListener.afterProjectExecutionSuccess(
new ProjectExecutionEvent(session, currentProject,
mojoExecutions));
+ participantAfterCalled = true;
+ LifecycleParticipantDispatcher.afterProjectExecution(session,
currentProject, mojoExecutions, null);
reactorContext
.getResult()
.addBuildSummary(new BuildSuccess(currentProject,
Duration.between(buildStartTime, buildEndTime)));
eventCatapult.fire(ExecutionEvent.Type.ProjectSucceeded, session,
null);
} catch (Throwable t) {
+ if (!participantAfterCalled) {
+ participantAfterCalled = true;
+ try {
+
LifecycleParticipantDispatcher.afterProjectExecution(session, currentProject,
mojoExecutions, t);
+ } catch (LifecycleExecutionException e) {
+ t.addSuppressed(e);
+ }
+ }
builderCommon.handleBuildError(reactorContext, rootSession,
session, currentProject, t, buildStartTime);
projectExecutionListener.afterProjectExecutionFailure(
Review Comment:
⚠️ **Inconsistent `ProjectExecutionListener` event sequence when participant
fails on the success path (high).**
When `LifecycleParticipantDispatcher.afterProjectExecution(…, null)` at line
122 throws a `LifecycleExecutionException`, execution falls into `catch
(Throwable t)`. Because `participantAfterCalled` is set to `true` at line 121
(before the dispatcher call), the guard at line 130 skips re-dispatching —
correct. But `afterProjectExecutionFailure` here is called unconditionally,
even though `afterProjectExecutionSuccess` was already called at lines 119–120.
`ProjectExecutionListener` implementations receive: `beforeProjectExecution
→ afterProjectExecutionSuccess → afterProjectExecutionFailure`. That sequence
is invalid — these callbacks are meant to be mutually exclusive.
`BuildPlanExecutor` avoids this correctly by catching the
`LifecycleExecutionException` inline and calling `handleBuildError` + `return`,
so `afterProjectExecutionFailure` is never reached.
Apply the same guard here:
```java
if (!participantAfterCalled) {
projectExecutionListener.afterProjectExecutionFailure(
new ProjectExecutionEvent(session, currentProject, t));
}
```
##########
api/maven-api-spi/src/main/java/org/apache/maven/api/spi/LifecycleParticipantException.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.api.spi;
+
+import org.apache.maven.api.annotations.Experimental;
+
+/**
+ * Signals that a {@link LifecycleParticipant} callback could not complete.
+ *
+ * @since 4.1.0
+ */
+@Experimental
+public class LifecycleParticipantException extends Exception {
+
+ public LifecycleParticipantException(String message) {
+ super(message);
+ }
Review Comment:
💡 **Public API missing Javadoc on constructors.** This is a public SPI class
— extension authors will read these constructors. All three lack `@param`
documentation.
```suggestion
/**
* Creates a new exception with the given message.
*
* @param message the detail message
*/
public LifecycleParticipantException(String message) {
super(message);
}
```
##########
api/maven-api-spi/src/main/java/org/apache/maven/api/spi/LifecycleParticipantException.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.api.spi;
+
+import org.apache.maven.api.annotations.Experimental;
+
+/**
+ * Signals that a {@link LifecycleParticipant} callback could not complete.
+ *
+ * @since 4.1.0
+ */
+@Experimental
+public class LifecycleParticipantException extends Exception {
+
+ public LifecycleParticipantException(String message) {
+ super(message);
+ }
+
+ public LifecycleParticipantException(String message, Throwable cause) {
+ super(message, cause);
+ }
+
+ public LifecycleParticipantException(Throwable cause) {
+ super(cause);
+ }
Review Comment:
💡 **Missing `@param` documentation on third constructor.**
```suggestion
/**
* Creates a new exception with the given cause.
*
* @param cause the root cause
*/
public LifecycleParticipantException(Throwable cause) {
super(cause);
}
```
##########
impl/maven-core/src/test/java/org/apache/maven/MavenLifecycleParticipantTest.java:
##########
@@ -100,6 +106,53 @@ public void afterProjectsRead(MavenSession session) {
}
}
+ public static class InjectReactorDependencyV4 implements
LifecycleParticipant {
+ @Override
+ public void afterProjectsRead(Session session) {
+ ProjectManager projectManager =
session.getService(ProjectManager.class);
+ Project target = session.getProjects().stream()
+ .filter(project -> "module-a".equals(project.artifactId()))
+ .findFirst()
+ .orElseThrow();
+ Project dependency = session.getProjects().stream()
+ .filter(project -> "module-b".equals(project.artifactId()))
+ .findFirst()
+ .orElseThrow();
+
+ List<org.apache.maven.api.DependencyCoordinates> dependencies =
new ArrayList<>(target.dependencies());
+
dependencies.add(session.createDependencyCoordinates(session.createArtifactCoordinates(
+ dependency.groupId(), dependency.artifactId(),
dependency.version(), "", "jar", "jar")));
+ projectManager.setDependencies(target, dependencies);
+ }
+ }
+
+ public static class RecordingLifecycleParticipant implements
LifecycleParticipant {
+ private static final List<String> CALLBACKS = new
CopyOnWriteArrayList<>();
+
+ @Override
+ public void afterProjectsRead(Session session) {
+ CALLBACKS.add("afterProjectsRead:" + session.getProjects().size());
+ }
+
+ @Override
+ public void beforeProjectExecution(ProjectExecutionEvent event) {
+ CALLBACKS.add("before:" + event.project().artifactId() + ':'
+ + event.executionPlan().get(0).goal());
Review Comment:
💡 **`executionPlan().get(0)` throws `IndexOutOfBoundsException` if the plan
is empty.** While the specific test project always has a mojo, this makes
`RecordingLifecycleParticipant` fragile. Use a null-safe form:
```suggestion
+ event.executionPlan().get(0).goal());
```
Prefer: `event.executionPlan().isEmpty() ? "(no plan)" :
event.executionPlan().get(0).goal()`
##########
api/maven-api-spi/src/main/java/org/apache/maven/api/spi/LifecycleParticipantException.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.api.spi;
+
+import org.apache.maven.api.annotations.Experimental;
+
+/**
+ * Signals that a {@link LifecycleParticipant} callback could not complete.
+ *
+ * @since 4.1.0
+ */
+@Experimental
+public class LifecycleParticipantException extends Exception {
+
+ public LifecycleParticipantException(String message) {
+ super(message);
+ }
+
+ public LifecycleParticipantException(String message, Throwable cause) {
+ super(message, cause);
+ }
Review Comment:
💡 **Missing `@param` documentation on second constructor.**
```suggestion
/**
* Creates a new exception with the given message and cause.
*
* @param message the detail message
* @param cause the root cause
*/
public LifecycleParticipantException(String message, Throwable cause) {
super(message, cause);
}
```
##########
impl/maven-core/src/test/java/org/apache/maven/MavenLifecycleParticipantTest.java:
##########
@@ -100,6 +106,53 @@ public void afterProjectsRead(MavenSession session) {
}
}
+ public static class InjectReactorDependencyV4 implements
LifecycleParticipant {
+ @Override
+ public void afterProjectsRead(Session session) {
+ ProjectManager projectManager =
session.getService(ProjectManager.class);
+ Project target = session.getProjects().stream()
+ .filter(project -> "module-a".equals(project.artifactId()))
+ .findFirst()
+ .orElseThrow();
+ Project dependency = session.getProjects().stream()
+ .filter(project -> "module-b".equals(project.artifactId()))
+ .findFirst()
+ .orElseThrow();
+
+ List<org.apache.maven.api.DependencyCoordinates> dependencies =
new ArrayList<>(target.dependencies());
+
dependencies.add(session.createDependencyCoordinates(session.createArtifactCoordinates(
+ dependency.groupId(), dependency.artifactId(),
dependency.version(), "", "jar", "jar")));
+ projectManager.setDependencies(target, dependencies);
+ }
+ }
+
+ public static class RecordingLifecycleParticipant implements
LifecycleParticipant {
+ private static final List<String> CALLBACKS = new
CopyOnWriteArrayList<>();
+
+ @Override
+ public void afterProjectsRead(Session session) {
+ CALLBACKS.add("afterProjectsRead:" + session.getProjects().size());
+ }
+
+ @Override
+ public void beforeProjectExecution(ProjectExecutionEvent event) {
+ CALLBACKS.add("before:" + event.project().artifactId() + ':'
+ + event.executionPlan().get(0).goal());
+ }
+
+ @Override
+ public void afterProjectExecution(ProjectExecutionEvent event) {
+ CALLBACKS.add("after:" + event.project().artifactId() + ':'
+ + event.executionPlan().get(0).goal() + ':'
Review Comment:
💡 Same `executionPlan().get(0)` issue — will throw if the plan is empty.
--
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]
