Copilot commented on code in PR #395:
URL:
https://github.com/apache/maven-build-cache-extension/pull/395#discussion_r3609477957
##########
src/main/java/org/apache/maven/buildcache/BuildCacheMojosExecutionStrategy.java:
##########
@@ -444,6 +464,81 @@ boolean isParamsMatched(
return true;
}
+ /**
+ * Captures plugin properties at validation time for all mojo executions.
+ * This ensures properties are read at the same lifecycle point for all
builds,
+ * eliminating timing mismatches caused by Maven 4's auto-injection of
properties
+ * like --module-version during execution.
+ *
+ * @param session Maven session
+ * @param project Current project
+ * @param mojoExecutions List of mojo executions to capture properties for
+ * @return Map of execution key to MojoExecutionEvent captured at
validation time
+ */
+ private Map<String, MojoExecutionEvent> captureValidationTimeProperties(
+ MavenSession session, MavenProject project, List<MojoExecution>
mojoExecutions)
+ throws LifecycleExecutionException {
+ Map<String, MojoExecutionEvent> validationTimeEvents = new HashMap<>();
+
+ for (MojoExecution mojoExecution : mojoExecutions) {
+ // Skip mojos that don't execute or are in clean phase
+ if (mojoExecution.getLifecyclePhase() == null
+ ||
!lifecyclePhasesHelper.isLaterPhaseThanClean(mojoExecution.getLifecyclePhase()))
{
+ continue;
+ }
+
+ mojoExecutionScope.enter();
+ try {
+ mojoExecutionScope.seed(MavenProject.class, project);
+ mojoExecutionScope.seed(MojoExecution.class, mojoExecution);
+
+ Mojo mojo = mavenPluginManager.getConfiguredMojo(Mojo.class,
session, mojoExecution);
+ try {
+ MojoExecutionEvent event = new MojoExecutionEvent(session,
project, mojoExecution, mojo);
+ validationTimeEvents.put(mojoExecutionKey(mojoExecution),
event);
+
+ LOGGER.debug(
+ "Captured validation-time properties for {}",
+
mojoExecution.getMojoDescriptor().getFullGoalName());
+ } finally {
+ mavenPluginManager.releaseMojo(mojo, mojoExecution);
+ }
Review Comment:
`captureValidationTimeProperties()` creates a configured Mojo instance,
stores it inside a `MojoExecutionEvent`, and then immediately calls
`mavenPluginManager.releaseMojo(mojo, mojoExecution)`. Later, `save()` passes
these events to `CacheControllerImpl.recordMojoProperties()`, which reflects
over `executionEvent.getMojo()` to read parameter values. Using a Mojo instance
after it has been released is unsafe (it may be disposed or reused), and can
lead to incorrect cached properties or hard-to-diagnose failures.
##########
src/main/java/org/apache/maven/buildcache/CacheResult.java:
##########
@@ -111,4 +156,8 @@ public RestoreStatus getStatus() {
public boolean isFinal() {
return build != null && build.getDto().is_final();
}
+
+ Map<String, MojoExecutionEvent> getValidationTimeEvents() {
+ return validationTimeEvents;
+ }
}
Review Comment:
`validationTimeEvents` can be provided via multiple public factory methods
(e.g., `empty(context, validationTimeEvents)` / `success(...,
validationTimeEvents)`), but there is no public accessor for this new state.
That makes the new API surface asymmetric and effectively unusable outside the
package, while all other CacheResult state has public getters. Consider making
the getter public (or alternatively, make the factory overloads
package-private/remove them).
--
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]
