Copilot commented on code in PR #395:
URL:
https://github.com/apache/maven-build-cache-extension/pull/395#discussion_r3608962908
##########
src/main/java/org/apache/maven/buildcache/BuildCacheMojosExecutionStrategy.java:
##########
@@ -134,6 +135,19 @@ public void execute(
}
if (cacheState == INITIALIZED) {
result = cacheController.findCachedBuild(session, project,
mojoExecutions, skipCache);
+
+ // Capture validation-time properties for all mojos to
ensure consistent property reading
+ // at the same lifecycle point for all builds (eliminates
Maven 4 injection timing issues)
+ // Always capture when cacheState is INITIALIZED since we
may need to save
+ if (cacheState == INITIALIZED) {
+ Map<String, MojoExecutionEvent> validationTimeEvents =
+ captureValidationTimeProperties(session,
project, mojoExecutions);
+ result = CacheResult.rebuilded(result,
validationTimeEvents);
+ LOGGER.debug(
+ "Captured validation-time properties for {}
mojos in project {}",
+ validationTimeEvents.size(),
+ projectName);
+ }
Review Comment:
Redundant `if (cacheState == INITIALIZED)` nested inside an outer `if
(cacheState == INITIALIZED)` adds noise and makes the control-flow harder to
scan. The capture can run unconditionally within the existing outer block.
##########
src/main/java/org/apache/maven/buildcache/BuildCacheMojosExecutionStrategy.java:
##########
@@ -444,6 +466,78 @@ 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;
+ }
+
+ try {
+ mojoExecutionScope.enter();
Review Comment:
`mojoExecutionScope.enter()` is inside the `try`, but `exit()` runs in
`finally`. If `enter()` throws (e.g., due to an inconsistent scope state), the
`finally` will still attempt `exit()`, which can trigger a secondary failure
and obscure the root cause. This file’s other usage patterns call `enter()`
before the `try` to avoid exiting an un-entered scope.
##########
src/test/projects/explicit-module-version/pom.xml:
##########
@@ -0,0 +1,57 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+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.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0";
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
http://maven.apache.org/xsd/maven-4.0.0.xsd";>
+ <modelVersion>4.0.0</modelVersion>
+
+ <groupId>org.apache.maven.caching.test.explicit</groupId>
+ <artifactId>explicit-module-version</artifactId>
+ <version>1.0.0-SNAPSHOT</version>
+ <packaging>jar</packaging>
+
+ <name>Explicit Module Version Test</name>
+ <description>Test project for JPMS module with explicit moduleVersion
configuration</description>
+
+ <properties>
+ <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
+ <maven.compiler.source>11</maven.compiler.source>
+ <maven.compiler.target>11</maven.compiler.target>
+ </properties>
+
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-compiler-plugin</artifactId>
+ <version>3.13.0</version>
+ <configuration>
+ <!-- Explicit moduleVersion configuration - present at validation
time -->
+ <compilerArgs>
+ <arg>--module-version</arg>
+ <arg>${project.version}</arg>
+ <arg>-parameters</arg>
+ <arg>-g</arg>
+ </compilerArgs>
Review Comment:
This test project is described as having an explicit `moduleVersion`
configuration, but the POM actually hard-codes `--module-version` via
`compilerArgs`. That doesn’t exercise the plugin’s `moduleVersion` parameter
path described in the PR/issue and can mask the real timing behavior.
##########
src/main/java/org/apache/maven/buildcache/BuildCacheMojosExecutionStrategy.java:
##########
@@ -444,6 +466,78 @@ 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;
+ }
+
+ try {
+ mojoExecutionScope.enter();
+ 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);
+ }
+
+ } catch (PluginConfigurationException | PluginContainerException
e) {
+ throw new LifecycleExecutionException(
+ "Cannot capture validation-time properties for "
+ +
mojoExecution.getMojoDescriptor().getFullGoalName(),
+ e);
+ } finally {
+ try {
+ mojoExecutionScope.exit();
+ } catch (MojoExecutionException e) {
+ LOGGER.debug("Error exiting mojo execution scope: {}",
e.getMessage());
+ }
+ }
+ }
+
+ LOGGER.debug("Captured validation-time properties for {} mojos",
validationTimeEvents.size());
+ return validationTimeEvents;
+ }
+
+ private void validateValidationTimeEvents(
+ String projectName,
+ List<MojoExecution> mojoExecutions,
+ Map<String, MojoExecutionEvent> validationTimeEvents) {
+ for (MojoExecution mojoExecution : mojoExecutions) {
+ if (mojoExecution.getLifecyclePhase() != null
+ &&
lifecyclePhasesHelper.isLaterPhaseThanClean(mojoExecution.getLifecyclePhase())
+ && (validationTimeEvents == null
+ ||
!validationTimeEvents.containsKey(mojoExecutionKey(mojoExecution)))) {
+ throw new AssertionError("Validation-time properties not
captured for project " + projectName);
Review Comment:
The AssertionError thrown when validation-time events are missing doesn’t
indicate which mojo is missing, making failures hard to diagnose. Including the
execution key (or full goal name) would make the invariant actionable.
--
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]
