Fix #375: Capture plugin properties at validation time to eliminate timing mismatch - #395
Fix #375: Capture plugin properties at validation time to eliminate timing mismatch#395cowwoc wants to merge 10 commits into
Conversation
57a76d8 to
d2c8071
Compare
elharo
left a comment
There was a problem hiding this comment.
need to run mvn spotless:apply
- Upgrade spotless-maven-plugin from 2.44.5 to 3.0.0 - Upgrade palantir-java-format from 2.56.0 to 2.81.0 for Java 25 support - Run mvn spotless:apply to format code Addresses review feedback on PR apache#395.
|
@elharo Done. Try now. |
- Split run-on sentence in AssertionError message - Use HashMap import instead of fully qualified name - Move Mojo variable declaration into try block (nested try-finally) - Return defensive copy from getValidationTimeEvents() for encapsulation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Split run-on sentence in AssertionError message - Use HashMap import instead of fully qualified name - Move Mojo variable declaration into try block (nested try-finally) - Return defensive copy from getValidationTimeEvents() for encapsulation
43445c6 to
fda621a
Compare
- Split run-on sentence in AssertionError message - Use HashMap import instead of fully qualified name - Move Mojo variable declaration into try block (nested try-finally) - Make getValidationTimeEvents() package-private
fda621a to
6cf3a36
Compare
- Upgrade spotless-maven-plugin from 2.44.5 to 3.0.0 - Upgrade palantir-java-format from 2.56.0 to 2.81.0 for Java 25 support - Run mvn spotless:apply to format code Addresses review feedback on PR apache#395.
- Split run-on sentence in AssertionError message - Use HashMap import instead of fully qualified name - Move Mojo variable declaration into try block (nested try-finally) - Make getValidationTimeEvents() package-private
6cf3a36 to
04b6246
Compare
|
@elharo I've pushed a commit that should fix the latest build failures. Try now. |
c4c7a6c to
86c7750
Compare
|
It looks like the latest build errors are not related to this PR. Can it be merged? Do you need to do something else to fix the build? |
|
Absolutely nothing should be merged on a failing build. A failing build blocks everything. |
There was a problem hiding this comment.
Pull request overview
This PR addresses #375 by eliminating a lifecycle timing mismatch in Maven 4 where --module-version ${project.version} is injected during execution, but cache validation reads plugin parameters earlier. The proposed approach captures and stores plugin properties consistently at validation time so that subsequent cache validations compare like-for-like values.
Changes:
- Extend
CacheResultto carry validation-timeMojoExecutionEvents. - Capture validation-time configured mojo instances in
BuildCacheMojosExecutionStrategyand use them for cache storage. - Add new integration-test projects and ITS coverage for Maven 4 JPMS, explicit module version, non-JPMS, and multi-module mixed JPMS/non-JPMS scenarios.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/org/apache/maven/buildcache/BuildCacheMojosExecutionStrategy.java | Captures validation-time mojo configuration and uses it during cache save to avoid Maven 4 execution-time injection mismatches. |
| src/main/java/org/apache/maven/buildcache/CacheResult.java | Adds storage for validation-time events so save can use consistent lifecycle-point data. |
| src/test/java/org/apache/maven/buildcache/its/Maven4JpmsModuleTest.java | ITS coverage for Maven 4 JPMS module auto-injection scenario (regression for #375). |
| src/test/java/org/apache/maven/buildcache/its/ExplicitModuleVersionTest.java | ITS coverage for explicit --module-version configuration present at validation time. |
| src/test/java/org/apache/maven/buildcache/its/NonJpmsProjectTest.java | ITS coverage ensuring no regression for regular (non-JPMS) Java projects. |
| src/test/java/org/apache/maven/buildcache/its/MultiModuleJpmsTest.java | ITS coverage for mixed JPMS/non-JPMS multi-module reactor builds. |
| src/test/projects/maven4-jpms-module/pom.xml | Maven 4 JPMS sample project used by ITS. |
| src/test/projects/maven4-jpms-module/src/main/java/org/apache/maven/caching/test/maven4/HelloMaven4.java | Sample source for Maven 4 JPMS project. |
| src/test/projects/maven4-jpms-module/src/main/java/module-info.java | JPMS descriptor to trigger module compilation behavior. |
| src/test/projects/maven4-jpms-module/.mvn/extensions.xml | Enables build cache extension for the Maven 4 JPMS test project. |
| src/test/projects/maven4-jpms-module/.mvn/maven-build-cache-config.xml | Enables cache in the Maven 4 JPMS test project. |
| src/test/projects/explicit-module-version/pom.xml | Sample project explicitly configuring --module-version. |
| src/test/projects/explicit-module-version/src/main/java/org/apache/maven/caching/test/explicit/ExplicitVersionModule.java | Sample source for explicit-module-version project. |
| src/test/projects/explicit-module-version/src/main/java/module-info.java | JPMS descriptor for explicit-module-version project. |
| src/test/projects/explicit-module-version/.mvn/extensions.xml | Enables build cache extension for explicit-module-version project. |
| src/test/projects/explicit-module-version/.mvn/maven-build-cache-config.xml | Enables cache in explicit-module-version project. |
| src/test/projects/non-jpms-project/pom.xml | Regular Java (non-JPMS) sample project for regression coverage. |
| src/test/projects/non-jpms-project/src/main/java/org/apache/maven/caching/test/nonjpms/RegularJavaClass.java | Sample source for non-JPMS regression project. |
| src/test/projects/non-jpms-project/.mvn/extensions.xml | Enables build cache extension for non-JPMS regression project. |
| src/test/projects/non-jpms-project/.mvn/maven-build-cache-config.xml | Enables cache in non-JPMS regression project. |
| src/test/projects/multi-module-jpms/pom.xml | Parent POM for mixed JPMS/non-JPMS multi-module test project. |
| src/test/projects/multi-module-jpms/module-a/pom.xml | JPMS module POM in mixed multi-module test. |
| src/test/projects/multi-module-jpms/module-a/src/main/java/org/apache/maven/caching/test/multi/modulea/ModuleA.java | Sample JPMS module source. |
| src/test/projects/multi-module-jpms/module-a/src/main/java/module-info.java | JPMS descriptor for module-a. |
| src/test/projects/multi-module-jpms/module-b/pom.xml | Non-JPMS module POM in mixed multi-module test. |
| src/test/projects/multi-module-jpms/module-b/src/main/java/org/apache/maven/caching/test/multi/moduleb/ModuleB.java | Sample non-JPMS module source. |
| src/test/projects/multi-module-jpms/.mvn/extensions.xml | Enables build cache extension for multi-module test project. |
| src/test/projects/multi-module-jpms/.mvn/maven-build-cache-config.xml | Enables cache in multi-module test project. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a2bd63c to
f04d1cc
Compare
|
@cstamas Comment addressed. Please re-run the check. |
|
@cstamas Round 2 complete :) Please re-check. |
|
@cstamas Please trigger another review. |
|
@cstamas The failure seems to be a transient network issue. Can you please trigger another build? |
|
Please rebase against latest master. |
…nate timing mismatch Maven 4 automatically injects --module-version into compiler arguments during execution, but cache validation happens before execution. This creates a timing mismatch that causes cache invalidation. This fix captures properties at validation time for ALL builds and stores ONLY validation-time values in the cache, ensuring consistent reading at the same lifecycle point. Changes: - Modified CacheResult to store validation-time mojo events - Added captureValidationTimeProperties() to BuildCacheMojosExecutionStrategy - Modified execute() to capture properties after findCachedBuild() - Modified save() to store ONLY validation-time events (AssertionError if missing) - Added comprehensive integration tests (Maven4Jpms, ExplicitModuleVersion, NonJpms, MultiModule) Benefits: - No configuration required (vs ignorePattern in PR apache#391) - Fixes root cause (timing mismatch) not symptom (value mismatch) - Works for ALL Maven 4 auto-injected properties - Preserves execution-time logging for debugging Alternative to PR apache#391
- Upgrade spotless-maven-plugin from 2.44.5 to 3.0.0 - Upgrade palantir-java-format from 2.56.0 to 2.81.0 for Java 25 support - Run mvn spotless:apply to format code Addresses review feedback on PR apache#395.
- Split run-on sentence in AssertionError message - Use HashMap import instead of fully qualified name - Move Mojo variable declaration into try block (nested try-finally) - Make getValidationTimeEvents() package-private
- Fix invalid XML in pom.xml comment (-- not allowed in XML comments) - Fix cache config XML validation errors by simplifying config - Change tests to use 'package' goal instead of 'compile' since cache extension stores artifacts, not intermediate output directories - Fix AssertionError when running clean-only builds by gracefully skipping cache storage when no cacheable mojos executed
Put LOGGER.debug() call on single line to satisfy Spotless formatting rules.
Add missing Apache Software Foundation license headers to all test project files as required by Apache conventions: - explicit-module-version: pom.xml, extensions.xml, module-info.java, ExplicitVersionModule.java - maven4-jpms-module: pom.xml, extensions.xml, module-info.java, HelloMaven4.java - multi-module-jpms: parent and module pom.xml files, extensions.xml, module-info.java, ModuleA.java, ModuleB.java - non-jpms-project: pom.xml, extensions.xml, RegularJavaClass.java maven-build-cache-config.xml files already had correct license headers.
f4a1cf2 to
c538272
Compare
|
@cstamas Done. Try now. |
Problem
Maven 4 automatically injects
--module-version ${project.version}into compiler arguments during execution, but the build cache validates properties before execution. This creates a timing mismatch that causes cache invalidation:Root Cause Analysis
The cache extension currently reads plugin properties at different lifecycle points for different builds:
The problem: validation reads BEFORE injection, storage reads AFTER injection.
Solution
Capture properties at validation time for ALL builds (even when no cache exists). Store ONLY validation-time values in the cache. This ensures both validation and storage read at the same lifecycle point.
Implementation
CacheResultto store validation-time mojo eventsBuildCacheMojosExecutionStrategyto capture properties immediately afterfindCachedBuild()save()to store ONLY validation-time events (fails withAssertionErrorif missing)Key Code Changes
Note: Execution-time values are still captured by
MojoParametersListenerfor logging/debugging during the build, but are NOT stored in the cache.New Timeline (Both Builds)
What Gets Stored vs. Logged
This approach ensures cache consistency while preserving debugging information in build logs.
Benefits Over PR #391
ignorePattern)Testing
Created comprehensive integration tests:
--module-versionmoduleVersionconfigurationAll tests verify:
ignorePatternconfiguration requiredBackward Compatibility
ignorePatternstill works but is no longer necessary for Maven 4 injectionAlternative Approaches Considered
Related Issues
Migration
No migration needed. This change is transparent to users. Existing projects will benefit automatically.