✨ feat: Add Maven build extensions support to lockfile#1529
✨ feat: Add Maven build extensions support to lockfile#1529tkubas-rh wants to merge 11 commits intochains-project:mainfrom
Conversation
263ffc8 to
89017a9
Compare
maven_plugin/src/main/java/io/github/chains_project/maven_lockfile/data/LockFile.java
Outdated
Show resolved
Hide resolved
maven_plugin/src/main/java/io/github/chains_project/maven_lockfile/data/LockFile.java
Show resolved
Hide resolved
...lugin/src/main/java/io/github/chains_project/maven_lockfile/data/AbstractMavenComponent.java
Outdated
Show resolved
Hide resolved
...lugin/src/main/java/io/github/chains_project/maven_lockfile/data/AbstractMavenComponent.java
Show resolved
Hide resolved
3154344 to
e7602f2
Compare
e7602f2 to
008fc34
Compare
- Create AbstractMavenComponent base class for plugins and extensions - Add MavenExtension data class with dependencies support - Update LockFile to include mavenExtensions field - Update LockFileDifference to diff extensions
- Include extension diffing in lockfile validation - Show missing extensions in validation error output
- Add RepositorySystem injection to AbstractLockfileMojo - Implement getAllExtensions() using project.getBuildExtensions() - Use batch resolution with Aether's CollectRequest/DependencyRequest - Rename resolvePluginDependencies to resolveComponentDependencies - Resolve extension dependencies using compile+runtime scopes - Pass repositorySystem to generateLockFileFromProject() Assisted-by: Claude Code
- buildExtensionsSimple: single extension with dependencies - buildExtensionsMultiple: multiple extensions - buildExtensionsValidationFail: tampered checksum detection 🐛 fix: Respect includeEnvironment config in ValidateChecksumMojo - Only generate environment metadata when config.isIncludeEnvironment() is true - Read lockfile config before generating environment metadata - Fixes false validation failures when lockfile has includeEnvironment=false 🐛 fix: Add checksum tiebreakers in AbstractMavenComponent.compareTo() - Add checksumAlgorithm and checksum comparison after version - Ensures consistent ordering and equals()/compareTo() contract - Fixes MavenPluginTest comparison test failures Assisted-by: Claude Code
- Verify extension dependencies are not empty - Verify all dependencies have valid scopes - Ensure TEST scope dependencies are excluded - Match thoroughness of plugin tests Assisted-by: Claude Code
- Update buildExtensionsValidationFail lockfile POM checksum - Allows test to reach extension validation instead of failing on POM
Signed-off-by: Timotej Kubas <tkubas@redhat.com>
Replace CRTP pattern with simple runtime typechecks. This simplifies the code while maintaining type isolation between MavenPlugin and MavenExtension. Changes: - Remove <T extends AbstractMavenComponent<T>> type parameter - Fix equals() to check exact type (this.getClass() != obj.getClass()) - Add type-first ordering in compareTo() - Add cross-type comparison tests to MavenPluginTest - Add MavenExtensionTest with comprehensive test coverage Signed-off-by: Timotej Kubas <tkubas@redhat.com>
Remove redundant buildExtensionsSimple test as buildExtensionsMultiple provides sufficient coverage for build extensions functionality. Changes: - Remove buildExtensionsSimple test directory - Remove buildExtensionsSimple test method from IntegrationTestsIT Signed-off-by: Timotej Kubas <tkubas@redhat.com>
Signed-off-by: Timotej Kubas <tkubas@redhat.com>
|
@algomaster99 Can we also get a review on this one? |
Signed-off-by: Timotej Kubas <tkubas@redhat.com>
a78b484 to
79cb8a1
Compare
|
@brunoapimentel sure! I can go through it over the weekend. Personally, I have not directly used build extensions so I will also need to spend some time understanding its usecase :) |
algomaster99
left a comment
There was a problem hiding this comment.
Overall, looks good. I left some minor comments below.
| && Objects.equals(resolved, other.resolved) | ||
| && Objects.equals(repositoryId, other.repositoryId) |
There was a problem hiding this comment.
resolved and repositoryId were not used to compare MavenPlugin before, but now they would be. I believe that is correct behaviour. Just flagging it here in case.
| transitiveDeps)); | ||
| } | ||
| } catch (DependencyResolutionException e) { | ||
| PluginLogManager.getLog().warn("Failed to resolve extension dependencies", e); |
There was a problem hiding this comment.
I guess a follow up PR could be to have an option to enforce failure here and I also think you would want this option in Konflux otherwise hermetic builds won't succeed.
| <extensions> | ||
| <extension> | ||
| <groupId>kr.motd.maven</groupId> | ||
| <artifactId>os-maven-plugin</artifactId> | ||
| <version>1.7.1</version> | ||
| </extension> | ||
| <extension> | ||
| <groupId>org.apache.maven.wagon</groupId> | ||
| <artifactId>wagon-ssh</artifactId> | ||
| <version>3.5.3</version> | ||
| </extension> | ||
| </extensions> |
There was a problem hiding this comment.
This is a not a question related to the PR, but more to enhance my knowledge. What behaviour difference to the build one will observe where these two extensions are configured? I also don't understand how extensions are invoked. Plugins can be invoked using certain goals from example so I am trying to find an analogy.
| assertThat(lockFile.getMavenExtensions()) | ||
| .anyMatch(ext -> ext.getGroupId().getValue().equals("kr.motd.maven") | ||
| && ext.getArtifactId().getValue().equals("os-maven-plugin")); | ||
| assertThat(lockFile.getMavenExtensions()) | ||
| .anyMatch(ext -> ext.getGroupId().getValue().equals("org.apache.maven.wagon") | ||
| && ext.getArtifactId().getValue().equals("wagon-ssh")); |
There was a problem hiding this comment.
Typecasting them to list and then comparing would also assert the order implicitly although we know TreeSet is ordered.
| public void buildExtensionsValidationFail(MavenExecutionResult result) throws Exception { | ||
| // contract: if an extension checksum in the lockfile doesn't match the actual extension, | ||
| // validation should fail | ||
| System.out.println("Running 'buildExtensionsValidationFail' integration test."); |
There was a problem hiding this comment.
| System.out.println("Running 'buildExtensionsValidationFail' integration test."); |
Probably a debugging statement?
| @SuppressWarnings("null") | ||
| public void buildExtensionsMultiple(MavenExecutionResult result) throws Exception { | ||
| // contract: if a project uses multiple build extensions, all should be recorded in the lockfile | ||
| System.out.println("Running 'buildExtensionsMultiple' integration test."); |
There was a problem hiding this comment.
| System.out.println("Running 'buildExtensionsMultiple' integration test."); |
|
@tkubas-rh can you please run spotless:apply to fix |
|
I'll have to investigate why |
Summary
Adds support for tracking Maven build extensions in lockfiles. Build extensions modify Maven's core behavior and are loaded before the build starts, making them critical for supply chain security and build reproducibility.
Implementation
The algorithm resolves build extensions through the following process:
project.getBuildExtensions()(non-deprecated API) to get extension model objectsRepositorySystemwith batch resolution viaCollectRequest/DependencyRequestresolveComponentDependencies()(renamed fromresolvePluginDependencies) for transitive dependency resolutionmavenExtensionskey in the lockfileChanges
AbstractMavenComponentbase classMavenExtensiondata class with dependencies supportLockFileto includemavenExtensionsfieldLockFileDifferenceto diff extensionsLockFileFacadeValidateChecksumMojoBug Fixes
ValidateChecksumMojoto respectincludeEnvironmentconfig flagCloses #1513