Add support for build artifacts locking using `lockBuild` parameter #85

smaarn · 2025-04-22T17:26:25Z

Objective: locking plugins (and their dependencies) as well as extensions being used in a maven project.

This involved several refactorings:

Extract Dependency from Artifact model
Introduce LockableEntity base class (and corresponding subclasses)

Fixes #84

Depends upon #86 (which basically updates the XSD)

…dencies)

vandmo

Looks really good!
Added some comment and have a closer look later.

src/it/pom/check-build/.dependency-lock/pom.xml

vandmo · 2025-04-23T06:41:04Z

src/main/java/se/vandmo/dependencylock/maven/mojos/AbstractDependencyLockMojo.java

+  private MavenSession mavenSession;
+
+  @Parameter(property = "dependencyLock.alsoLockBuild")
+  private boolean alsoLockBuild;


I think lockBuild would be enough? And maybe it should be moved to LockMojo.java ?
I think that the check goal should check the build lock if it exists in the lock file.

Indeed.
Reworked that in two phases:

Renaming the parameter (991ae44)

Pull down the parameter and make sure the check logic automatically detects if it needs to check build logics (in 9a417a4)

src/main/java/se/vandmo/dependencylock/maven/mojos/CheckMojo.java

src/main/resources/se/vandmo/dependencylock/maven/pom/pom-with-plugins.ftlx

…ion and withIntegrity

…e level of the lock (and have the build check logic be done only if build intel is provided in lock file)

…ly artifacts shared between plugins

…ontents of the file at once)

smaarn · 2025-04-23T13:18:00Z

I believe this is pretty much ready to be reviewed.

I recon there are a few classes for which unit testing could be helpful (specifically thinking of lock file formatting / parsing logic which may be tricky to maintain and where errors are quickly made), but would rather have feedbacks first to see if this is the right implementation target.

vandmo · 2025-04-24T05:41:02Z

I have had a closer look now.

I think the code looks good but I am unsure about the new structure of the dependency-lock.json. I like the idea but it would break the build if somebody manages to do a lock with a new version of the plugin. It is opt-in in a way since lockBuild needs to be set but maybe it should be an additional flag. Is the lock file a lot smaller then it would be if it would behave as the pom lock file?

The XML schema would need to be updated as well (https://vandmo.github.io/dependency-lock-maven-plugin/maven-v4_0_0_ext.xsd and https://vandmo.github.io/dependency-lock-maven-plugin/dependencylock.xsd)

There are some tests that should problaby be added that I noticed, but those could be done separately from this MR. I can write them myself unless you really want to :)

A test with <extension>...</extension> in the pom.
Make sure that the generated pom validates against the schema at https://vandmo.github.io/dependency-lock-maven-plugin/dependencylock.xsd

smaarn · 2025-04-24T09:15:42Z

Will add a test for the support of "extension".

One question I have though: the mentioned xsd needs to be altered from what I see (or a newer version generated), would there be any objection to having it included in the build process ?

I see three ways of implementing this:

Having a copy of the xsd in the build process and have it "delivered" / "checked" in the release process (if we are to version the XSD)
Having a copy of the xsd in the build process and have it updated in the release process (if we are to not version the XSD)
Manually uploading the xsds and ensuring in the build process that those actually are compliant with generated resources (there are some plugins out there to "fetch them")

What would be your preference here ?

vandmo · 2025-04-24T09:33:39Z

I think it would be great if there could be a build step that commits the file in the gh-pages branch.

smaarn · 2025-04-24T15:44:22Z

Added:

Integration tests for extension cases (turns out that there was a bug so... good thing :) )
New XSD usage

This means that this PR will depend on #86 being merged to be fully operational in terms of XSD compliance

I can propose to do the automatic publishing to gh-pages but I'd rather do it in a separate PR as it involves a lot more discussion (notably because it requires adding rights for the process to contribute to github branches)

vandmo · 2025-04-24T17:48:25Z

I am pondering wheter it is a good idea to make the JSON lock file incompatible with older versions of the plugin.
Pros:

Size reduction I would assume, but unsure how much
The structure makes it impossible to specify different checksums for the same artifact
Older versions of the plugin will fail which requires the user to bump it to a version that supports build locking which is wanted by the lock file
Cons:
Older versions of the plugin will fail with a bad error message if it encounters the lock file

Am I missing something?

smaarn · 2025-04-24T19:48:11Z

I am pondering wheter it is a good idea to make the JSON lock file incompatible with older versions of the plugin.
Pros:

Size reduction I would assume, but unsure how much

The structure makes it impossible to specify different checksums for the same artifact

Older versions of the plugin will fail which requires the user to bump it to a version that supports build locking which is wanted by the lock file
Cons:

Older versions of the plugin will fail with a bad error message if it encounters the lock file

Am I missing something?

I believe you're not missing anything indeed. The "space gain" is not even guaranteed to be a huge one IMHO.

My only question here would be: is it reasonable to expect different versions of the plugin being used ? (e.g. locking with a new version and checking with an old version) Rolling back this is a one liner so do tell me if you'd rather keep it "backward compatible" (in the same manner as for the POM it turns out).

smaarn · 2025-04-24T20:14:58Z

Fixed the JSON format change in 0f7b954. WDYT ?

vandmo · 2025-04-25T06:20:06Z

Yeah, lets go with this version. It is consistent with how the pom is as well and if an artifact gets locked with different checksums for some reason then the check would fail for one of those. Probably very low risk of that happening though, somebody would need to edit the lock file manually I guess.
I will look closer during the day and merge it hopefully. Is it ok if I do "Squash and Merge" or do you want to keep the individual commits?

smaarn · 2025-04-25T07:54:23Z

Yeah, lets go with this version. It is consistent with how the pom is as well and if an artifact gets locked with different checksums for some reason then the check would fail for one of those. Probably very low risk of that happening though, somebody would need to edit the lock file manually I guess. I will look closer during the day and merge it hopefully. Is it ok if I do "Squash and Merge" or do you want to keep the individual commits?

By all means, do "Squash and Merge" ! The initial commits separation was to ease the review process and the refactoring logic, it loses any interests to keep as is "on master".

vandmo · 2025-04-25T11:51:12Z

Could you change back to the initial structure of the JSON lock file?
I have though about it now and I think that makes most sense.

It could be worth having a version: 2 in the JSON file and something similar in the XML file as well, and then make the plugin fail on any newer versions with a message like "Format version X not recognised, try bumping the plugin version". That would make it possible to do similar changes in the future which would result in better error messages for at least some people. That is separate PR/change though.

Maybe it could be worth having an option to use the new format even if the build is not locked, but that is also a separate thing.

This reverts commit 0f7b954.

…riminate when reading)

smaarn · 2025-04-25T16:53:20Z

Could you change back to the initial structure of the JSON lock file? I have though about it now and I think that makes most sense.

It could be worth having a version: 2 in the JSON file and something similar in the XML file as well, and then make the plugin fail on any newer versions with a message like "Format version X not recognised, try bumping the plugin version". That would make it possible to do similar changes in the future which would result in better error messages for at least some people. That is separate PR/change though.

Maybe it could be worth having an option to use the new format even if the build is not locked, but that is also a separate thing.

Rolled back to the former proposal (e.g. "dependencies" field using "artifact" indirection) and added version field (used it as way to identify the file format).

Regarding the POM situation there are two options:

Using a versioned XSD instead (e.g. dedicated path)
Adding a "lockVersion" (the "version" field is already used, as well as "modelVersion")

Not too sure about either options TBH. What would be your take here ?

vandmo · 2025-04-25T21:50:22Z

Looks really good! I merged it and will create separate issues for remaining things we have talked about. Would be good to get the version in to the the XML somehow before creating a release.
I can always create a 0.0.0- version if you want to try it out before that.

smaarn added 11 commits April 21, 2025 22:25

Rename DependenciesLockFileAccessor to LockFileAccessor

a779b07

Differentiate Artifact from Dependency (and rename Artifacts to Depen…

a4874b9

…dencies)

Extract VersionUtils class

9fa44b6

Extract LockableEntity(ies) class hierarchy

a240197

Extract DiffHelper class

e5bf731

Extract DiffReport class

43402ff

Introduce Plugin and corresponding Locked entities

939b451

Introduce Extension and corresponding Locked entities

be70ec4

Introduce Project and corresponding Locked entities

6452484

Introduce Lockfile and corresponding JSON and POM formatters

516a15e

Add support for alsoLockBuild parameter

53ed213

smaarn force-pushed the feat/add-plugins-support branch from c7b5c83 to 53ed213 Compare April 22, 2025 17:58

vandmo reviewed Apr 23, 2025

View reviewed changes

smaarn added 6 commits April 23, 2025 10:41

Extract LockedEntities class hierarchy

e377a93

Make sure LockableEntity classes all appropriately implement withVers…

d9152b9

…ion and withIntegrity

PR feedback: align indenting

ad1c38b

PR feedback: renamed alsoLockBuild to lockBuild

991ae44

PR feedback: make the "buildLock" parameter only something done at th…

9a417a4

…e level of the lock (and have the build check logic be done only if build intel is provided in lock file)

Perf optim: don't hold all plugin realms at the same time

7e1d30e

smaarn changed the title ~~Add support for build artifacts locking using alsoLockBuild parameter~~ Add support for build artifacts locking using lockBuild parameter Apr 23, 2025

smaarn added 5 commits April 23, 2025 14:57

Perf optim: introduce artifact cache to avoid recomputing unnecessari…

8d8edd1

…ly artifacts shared between plugins

Perf optim: use buffer to compute hash (instead of reading the full c…

97060f1

…ontents of the file at once)

Fix compilation warning (about untyped generic)

78710a1

Add test on optimized code

574478e

Fixed error handling for malformed XML content

ff508f4

smaarn marked this pull request as ready for review April 23, 2025 13:18

smaarn requested a review from vandmo April 23, 2025 13:18

smaarn added 2 commits April 24, 2025 11:44

PR feedback: add IT tests with extensions

b9b40a0

Fixed: extensions artifact are handled as "maven-plugin" artifacts

74a2aed

smaarn added 2 commits April 24, 2025 17:44

Depend on new XSD version

a4ae53d

Update lock file

a0c59d2

smaarn added 2 commits April 24, 2025 22:11

Fix formatting

cf1523d

PR feedback: keep "dependencies" contents' format

0f7b954

Account for XSD path change

55e65b6

smaarn added 2 commits April 25, 2025 18:33

Revert "PR feedback: keep "dependencies" contents' format"

c60b1d4

This reverts commit 0f7b954.

PR feedback: introduce version field in JSON file (and use it to disc…

d061492

…riminate when reading)

vandmo merged commit 34b1528 into vandmo:master Apr 25, 2025
2 checks passed

smaarn deleted the feat/add-plugins-support branch May 24, 2025 19:08

Add support for build artifacts locking using lockBuild parameter #85

Add support for build artifacts locking using lockBuild parameter #85

Uh oh!

Conversation

smaarn commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vandmo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vandmo Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

smaarn Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

smaarn commented Apr 23, 2025

Uh oh!

vandmo commented Apr 24, 2025

Uh oh!

smaarn commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vandmo commented Apr 24, 2025

Uh oh!

smaarn commented Apr 24, 2025

Uh oh!

vandmo commented Apr 24, 2025

Uh oh!

smaarn commented Apr 24, 2025

Uh oh!

smaarn commented Apr 24, 2025

Uh oh!

vandmo commented Apr 25, 2025

Uh oh!

smaarn commented Apr 25, 2025

Uh oh!

vandmo commented Apr 25, 2025

Uh oh!

smaarn commented Apr 25, 2025

Uh oh!

Uh oh!

vandmo commented Apr 25, 2025

Uh oh!

Uh oh!

Add support for build artifacts locking using `lockBuild` parameter #85

Add support for build artifacts locking using `lockBuild` parameter #85

smaarn commented Apr 22, 2025 •

edited

Loading

smaarn commented Apr 24, 2025 •

edited

Loading