-
Notifications
You must be signed in to change notification settings - Fork 55
Add default executionControl reconciliation for common Maven plugins #389
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
## Problem Without executionControl configuration, the build cache does not track critical plugin properties that are often specified via command-line arguments (e.g., -Dmaven.compiler.release). This leads to incorrect cache reuse when these properties change between builds. In multi-module JPMS projects, this manifests as compilation failures or bytecode version mismatches: 1. Build with `-Dmaven.compiler.release=17` → cache stores Java 17 bytecode (major version 61) 2. Build with `-Dmaven.compiler.release=21` → cache incorrectly reuses Java 17 bytecode 3. Result: Bytecode remains major version 61 instead of expected 65 This is particularly problematic for module-info.class files which are sensitive to Java version. ## Solution Implemented default reconciliation configs for common plugins when executionControl is not specified: - **maven-compiler-plugin** (compile & testCompile goals): - Tracks `source`, `target`, and `release` properties - Ensures cache invalidation when Java version changes - **maven-install-plugin** (install goal): - Tracked to ensure local repository is updated when needed ## Testing Verified with multi-module JPMS test project: **Before (broken):** ``` mvn clean verify -Dmaven.compiler.release=17 # Caches Java 17 bytecode mvn clean verify -Dmaven.compiler.release=21 # Incorrectly reuses Java 17 bytecode javap -v module-info.class | grep "major version" # Shows 61 (wrong!) ``` **After (fixed):** ``` mvn clean verify -Dmaven.compiler.release=17 # Caches Java 17 bytecode mvn clean verify -Dmaven.compiler.release=21 # Detects change, recompiles javap -v module-info.class | grep "major version" # Shows 65 (correct!) ``` ## Impact - Users no longer need to manually configure executionControl for basic scenarios - Prevents silent bytecode version mismatches in JPMS projects - Backward compatible: explicit executionControl config still takes precedence
70d91e7
to
e99c008
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve this change because it does not introduce any regressions and indeed, could fix certain scenarios. However, I would prefer the following enhancements:
-
An integration test that verifies the fix. This should include scenarios such as:
- No configuration set
- No plugin configuration set
- If plugin reconciliation set, the default is overridden (or merged)
-
Although this is definitely a step in the right direction, I would love to see some ground work to make this configuration extensible.
To achieve this, I suggest the following:
- For each plugin, export its all parameters to xml (per plugin).
- For the exported parameters, annotate each one as either "behavioral" or "functional." (further categorization might also help). For example, the number of compiler threads or log level would be considered behavioral parameters, whereas the target level would be functional.
The goal is to create a closed and matching set of parameters for default configurations for every plugin, every goal. The current implementation does not enforce the categorization of all parameters, which can lead to incorrect behavior if the plugin is modified (for instance, if a critical parameter is renamed).
Ideally, the default configuration should fail on encountering an unknown parameter or at least emit a warning.
src/main/java/org/apache/maven/buildcache/xml/CacheConfigImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/maven/buildcache/xml/CacheConfigImpl.java
Outdated
Show resolved
Hide resolved
Addresses reviewer feedback on PR apache#389: 1. Add integration tests for default reconciliation behavior: - Test with no executionControl configuration (uses defaults) - Test with executionControl for different plugin (verifies merging) 2. Fix reconciliation config resolution to merge defaults with explicit config: - Explicit configs take precedence when present - Default reconciliation configs apply when no explicit config exists - Ensures compiler defaults work even when other plugins are configured 3. Add null safety checks to prevent NPE in unit tests: - Check for null mojoExecution before accessing - Check for null plugin before matching Integration tests verify that: - Changing maven.compiler.release invalidates cache as expected - Changing maven.compiler.source/target invalidates cache - Default reconciliation still applies when executionControl configures other plugins
Addresses reviewer feedback on PR apache#389: 1. Add integration tests for default reconciliation behavior: - Test with no executionControl configuration (uses defaults) - Test with executionControl for different plugin (verifies merging) 2. Fix reconciliation config resolution to merge defaults with explicit config: - Explicit configs take precedence when present - Default reconciliation configs apply when no explicit config exists - Ensures compiler defaults work even when other plugins are configured 3. Add null safety checks to prevent NPE in unit tests: - Check for null mojoExecution before accessing - Check for null plugin before matching Integration tests verify that: - Changing maven.compiler.release invalidates cache as expected - Changing maven.compiler.source/target invalidates cache - Default reconciliation still applies when executionControl configures other plugins
0136c97
to
2619554
Compare
Addresses reviewer feedback on PR apache#389: 1. Add integration tests for default reconciliation behavior: - Test with no executionControl configuration (uses defaults) - Test with executionControl for different plugin (verifies merging) 2. Fix reconciliation config resolution to merge defaults with explicit config: - Explicit configs take precedence when present - Default reconciliation configs apply when no explicit config exists - Ensures compiler defaults work even when other plugins are configured 3. Add null safety checks to prevent NPE in unit tests: - Check for null mojoExecution before accessing - Check for null plugin before matching Integration tests verify that: - Changing maven.compiler.release invalidates cache as expected - Changing maven.compiler.source/target invalidates cache - Default reconciliation still applies when executionControl configures other plugins Add test for explicit config overriding defaults Completes integration test coverage requested by reviewer: 1. ✅ No configuration set - DefaultReconciliationTest 2. ✅ No plugin configuration set - DefaultReconciliationTest 3. ✅ Plugin reconciliation overrides defaults - DefaultReconciliationOverrideTest (NEW) 4. ✅ Plugin reconciliation merges with defaults - DefaultReconciliationWithOtherPluginTest The new test verifies that when executionControl explicitly configures maven-compiler-plugin, the explicit config completely OVERRIDES the defaults rather than merging: - Explicit config tracks only 'release' property - Changing 'source' DOES hit cache (proves 'source' not tracked) - Changing 'release' does NOT hit cache (proves 'release' is tracked) - If defaults merged, 'source' would also be tracked (but it's not) This confirms the implementation correctly handles both override and merge scenarios as requested. Implement plugin parameter export/categorization system Addresses reviewer suggestion to create extensible configuration system: **Architecture:** - Plugin parameter definitions stored as XML files in classpath - Each plugin has its own XML file (e.g., maven-compiler-plugin.xml) - Parameters categorized as "functional" or "behavioral" - Validation system checks reconciliation configs against definitions **Components:** 1. **XML Schema** (plugin-parameters.xsd): - Defines structure for parameter definitions - Enforces functional/behavioral categorization - Validates at build time 2. **Parameter Definitions**: - maven-compiler-plugin.xml: 30+ parameters for compile/testCompile goals - maven-install-plugin.xml: 12+ parameters for install/install-file goals - Comprehensive categorization based on whether parameter affects output 3. **Java Infrastructure**: - PluginParameterDefinition: Model classes for definitions - PluginParameterLoader: Loads XML from classpath - Validation logic in CacheConfigImpl 4. **Validation Features**: - ERROR on unknown parameters (may indicate plugin changes) - WARN on behavioral parameters in reconciliation config - WARN when no definition exists for plugin - Validates all default reconciliation configs on initialization **Benefits:** - Detects plugin parameter changes/renames automatically - Prevents incorrect behavioral parameters in reconciliation - Self-documenting: XML files serve as parameter catalog - Extensible: Add new plugins by creating XML files - Type-safe: Enforces functional/behavioral distinction **Testing:** - Unit tests verify parameter loading and categorization - Validates that default reconciliation params are functional - All 20 existing tests pass + 3 new validation tests This creates a "closed and matching set of parameters" as requested, ensuring the default configuration fails/warns on unknown parameters and preventing incorrect behavior from plugin modifications. Add user documentation for parameter validation system Updates documentation to explain the new parameter categorization and validation features: **how-to.md additions:** - New "Parameter Validation and Categorization" section - Explains functional vs behavioral parameter categories - Documents validation features (ERROR/WARN on unknown/misclassified params) - Provides example XML for adding new plugin definitions - Notes that defaults are overridden by explicit config per-plugin - Lists current coverage (maven-compiler-plugin, maven-install-plugin) **concepts.md additions:** - Reference to parameter validation system - Link to detailed how-to documentation - Context within correctness vs performance tradeoff discussion This documentation will appear on the Maven website, making the feature discoverable and providing clear guidance for: - Understanding why certain parameters are tracked - Adding validation for new plugins - Troubleshooting unknown parameter warnings - Extending the system as plugins evolve Fix integration test version placeholder Use ${projectVersion} instead of @project.version@ to match the resource filtering pattern used by other integration tests. Fix integration tests to check for reconciliation behavior - Fix XML config structure: executionControl is sibling of configuration, not child - Update test assertions to check for 'Plugin parameter mismatch found' message - Reconciliation happens at runtime after cache lookup, not during hash calculation - Tests now correctly verify that parameter changes trigger rebuild via reconciliation Add version-specific parameter definition support - Update XSD schema to support minVersion element - Implement version-aware parameter loading with best-match selection - Add version comparison logic handling SNAPSHOT qualifiers - Update validation to use plugin version at runtime - Add comprehensive tests for version-specific loading - Update documentation with version support examples Version matching selects the definition with highest minVersion <= plugin version. Multiple version-specific definitions can coexist in a single XML file. This allows accurate validation as plugin APIs evolve across versions. Load default reconciliation configs from XML instead of hardcoding - Create default-reconciliation.xsd schema for defaults - Add defaults.xml with maven-compiler-plugin and maven-install-plugin configs - Implement DefaultReconciliationLoader to load configs from XML - Replace hardcoded defaults in CacheConfigImpl with XML loading - Update documentation to reflect XML-based configuration This makes defaults extensible - projects can provide their own default-reconciliation/defaults.xml on the classpath.
7258f28
to
ed9332a
Compare
- Created integration tests for three scenarios: * No executionControl configuration (defaults apply automatically) * executionControl with other plugin (defaults still apply to unconfigured plugins) * Explicit plugin config (overrides defaults for that plugin only) - Implemented parameter export/categorization system: * Created plugin-parameters.xsd schema * Added maven-compiler-plugin.xml (57 parameters) * Added maven-install-plugin.xml (22 parameters) * Parameter validation with ERROR/WARN logging * Version-specific parameter support with best-match selection - Implemented XML-based default reconciliation: * Created default-reconciliation.xsd schema * Created defaults.xml with maven-compiler-plugin and maven-install-plugin * Implemented DefaultReconciliationLoader to replace hardcoded defaults - Updated documentation: * Clarified that defaults are built-in (not user-customizable) * Added example showing how to override defaults via .mvn/maven-build-cache-config.xml * Documented parameter validation and version-specific definitions
ed9332a
to
4678f01
Compare
@AlexanderAshitkin So if I understand you correctly, for the limited number of plugins that we supply defaults for, you want to provide enhanced validation to ensure that we log a warning if a user attempts to track changes of a non-existent or behavioral parameter. Is that correct? Take a look at the latest commit I pushed and let me know if this matches what you had in mind. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good improvement. Please recategorize skip parameters to functional
, and I’d like to get feedback on the possibility of direct reconciliation using the new plugin descriptors.
The 'skip' and 'skipMain' parameters directly affect build output: - skip=true: No bytecode produced (compilation skipped entirely) - skip=false: Bytecode produced normally This is a functional difference (affects artifacts), not behavioral (affects only how the build runs). Updated all three occurrences: - compile goal: skip parameter - compile goal: skipMain parameter - testCompile goal: skip parameter Addresses reviewer feedback on PR apache#389.
…nitions Replace static defaults.xml with dynamic auto-generation that tracks ALL functional parameters from plugin-parameters/*.xml files. Changes: - Remove defaults.xml (no longer needed - all plugins have XML definitions) - Remove DefaultReconciliationLoader class (replaced by auto-generation) - Add generateReconciliationFromParameters() method to CacheConfigImpl that automatically creates reconciliation configs by reading functional parameters from plugin parameter definitions - Auto-generation runs as final fallback after explicit config check Benefits: - Single source of truth (plugin-parameters/*.xml defines everything) - More comprehensive tracking (all functional params, not just 3) - Easier maintenance (no need to keep defaults.xml in sync) - Future-proof (new plugins with XML definitions get auto-tracking) For maven-compiler-plugin: - Before: Tracked 3 params (source, target, release) - After: Tracks ALL 15 functional params (source, target, release, encoding, debug, debuglevel, optimize, compilerArgs, etc.) All existing tests pass, including DefaultReconciliationTest which validates the auto-generation works correctly. Addresses reviewer feedback on PR apache#389.
Add comprehensive unit test to validate that the auto-generation from plugin XML definitions tracks ALL functional parameters, not just the 3 that were in the old defaults.xml. Tests verify: - maven-compiler-plugin tracks MORE than the original 3 parameters (source, target, release) and includes encoding, debug, compilerArgs, annotationProcessorPaths, etc. - maven-install-plugin tracks functional parameters (old defaults.xml had ZERO properties listed) - Behavioral parameters (verbose, fork) are NOT auto-tracked This test ensures the auto-generation is working as designed and provides more comprehensive tracking than the old defaults.xml system.
Problem
Without executionControl configuration, the build cache does not track critical plugin properties that are often specified via command-line arguments (e.g.,
-Dmaven.compiler.release
). This leads to incorrect cache reuse when these properties change between builds.In multi-module JPMS projects, this manifests as compilation failures or bytecode version mismatches:
-Dmaven.compiler.release=17
→ cache stores Java 17 bytecode (major version 61)-Dmaven.compiler.release=21
→ cache incorrectly reuses Java 17 bytecodeThis is particularly problematic for
module-info.class
files which are sensitive to Java version.Solution
Implemented default reconciliation configs for common plugins when executionControl is not specified:
maven-compiler-plugin (
compile
&testCompile
goals):source
,target
, andrelease
propertiesmaven-install-plugin (
install
goal):Users can still provide explicit
executionControl
configuration which will override these defaults.Testing
Verified with multi-module JPMS test project. See commit message for detailed test results.
Before (broken):
After (fixed):
Impact
Related Issues
This PR addresses multiple user-reported issues: