Code changes for plugin dependencies include test scope#1487
Open
akallian-eng wants to merge 3 commits intochains-project:mainfrom
Open
Code changes for plugin dependencies include test scope#1487akallian-eng wants to merge 3 commits intochains-project:mainfrom
test scope#1487akallian-eng wants to merge 3 commits intochains-project:mainfrom
Conversation
adambkaplan
suggested changes
Feb 2, 2026
Contributor
adambkaplan
left a comment
adambkaplan
left a comment
There was a problem hiding this comment.
Added my comments - it looks like there are other formatting issues that need to be checked by running mvn spotless:apply.
I'm a bit concerned that this is a "band-aid" that papers over a deeper bug. After all, we don't see standard compile-scoped dependencies bring in their test-scoped dependencies. Is the mechanism for building the dependency tree for plugins too broad in the first place?
| MetaData metadata) { | ||
| PluginLogManager.getLog().info(String.format("Generating lock file for project %s", project.getArtifactId())); | ||
| Set<MavenPlugin> plugins = new TreeSet<>(); | ||
| Set<MavenPlugin> plugins = new TreeSet<>(Comparator.comparing(MavenPlugin::getChecksum)); |
Contributor
There was a problem hiding this comment.
Is this change required/necessary?
| DependencyCollectorBuilder dependencyCollectorBuilder, | ||
| AbstractChecksumCalculator checksumCalculator) { | ||
| Set<MavenPlugin> plugins = new TreeSet<>(); | ||
| Set<MavenPlugin> plugins = new TreeSet<>(Comparator.comparing(MavenPlugin::getChecksum)); |
Contributor
There was a problem hiding this comment.
Ditto - is this required/necessary?
| String relativePath = project.getFile() == null | ||
| ? null | ||
| : initialProject | ||
| .getBasedir() |
Contributor
There was a problem hiding this comment.
Unnecessary formatting change, please revert.
| AbstractChecksumCalculator calc, | ||
| boolean isRoot, | ||
| boolean reduce) { | ||
| boolean reduce,boolean skipTestScope) { |
Contributor
There was a problem hiding this comment.
nit: format
Suggested change
| boolean reduce,boolean skipTestScope) { | |
| boolean reduce, | |
| boolean skipTestScope) { |
Comment on lines
93
to
97
| if (skipPluginTestDependency(node, isRoot, skipTestScope)) { | ||
| PluginLogManager.getLog().debug( | ||
| "Skipping plugin test-scope dependency: " + node.toNodeString()); | ||
| return Optional.empty(); | ||
| } |
Contributor
There was a problem hiding this comment.
Can't we simplify this and avoid a separate function call?
(pseudocode below)
if (skipTestScope && !isRoot && MavenScope.Test.equals(scope)) {
...
}|
|
||
| DependencyGraph dependencyGraph = DependencyGraph.of(graph, checksumCalculator, false); | ||
|
|
||
| DependencyGraph dependencyGraph = DependencyGraph.of(graph, checksumCalculator,false, true); |
Contributor
There was a problem hiding this comment.
Worth adding a comment here that we are excluding test scope.
Suggested change
| DependencyGraph dependencyGraph = DependencyGraph.of(graph, checksumCalculator,false, true); | |
| // Ignore test scoped dependencies for plugins. | |
| DependencyGraph dependencyGraph = DependencyGraph.of(graph, checksumCalculator,false, true); |
Author
|
Hi @adambkaplan,
Thanks for the feedback — this was helpful.
Based on your comments, I simplified the logic and removed the separate
helper method for skipping test dependencies. Instead, I now handle it
directly inside createDependencyNode with a simple inline check:
if (skipTestScope && !isRoot && scope == MavenScope.TEST) {
return Optional.empty();
}
This avoids an extra method call while keeping the behavior clear and easy
to follow.
In LockFileFacade.resolvePluginDependencies, I also explicitly enabled
test-scope skipping only for plugin dependency graphs:
DependencyGraph dependencyGraph =
DependencyGraph.of(graph, checksumCalculator, false, true);
and added a clarifying comment that we are intentionally excluding
test-scoped plugin dependencies.
Please let me know if you’d like this expressed differently or moved
elsewhere in the code. Happy to adjust.
Thanks!
…On Tue, Feb 3, 2026 at 1:51 AM Adam Kaplan ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Added my comments - it looks like there are other formatting issues that
need to be checked by running mvn spotless:apply.
I'm a bit concerned that this is a "band-aid" that papers over a deeper
bug. After all, we don't see standard compile-scoped dependencies bring
in their test-scoped dependencies. Is the mechanism for building the
dependency tree for plugins too broad in the first place?
------------------------------
In
maven_plugin/src/main/java/io/github/chains_project/maven_lockfile/LockFileFacade.java
<#1487 (comment)>
:
> @@ -96,7 +96,7 @@ public static LockFile generateLockFileFromProject(
AbstractChecksumCalculator checksumCalculator,
MetaData metadata) {
PluginLogManager.getLog().info(String.format("Generating lock file for project %s", project.getArtifactId()));
- Set<MavenPlugin> plugins = new TreeSet<>();
+ Set<MavenPlugin> plugins = new TreeSet<>(Comparator.comparing(MavenPlugin::getChecksum));
Is this change required/necessary?
------------------------------
In
maven_plugin/src/main/java/io/github/chains_project/maven_lockfile/LockFileFacade.java
<#1487 (comment)>
:
> @@ -127,7 +127,7 @@ private static Set<MavenPlugin> getAllPlugins(
MavenSession session,
DependencyCollectorBuilder dependencyCollectorBuilder,
AbstractChecksumCalculator checksumCalculator) {
- Set<MavenPlugin> plugins = new TreeSet<>();
+ Set<MavenPlugin> plugins = new TreeSet<>(Comparator.comparing(MavenPlugin::getChecksum));
Ditto - is this required/necessary?
------------------------------
In
maven_plugin/src/main/java/io/github/chains_project/maven_lockfile/LockFileFacade.java
<#1487 (comment)>
:
> @@ -364,10 +363,10 @@ private static Pom constructRecursivePom(
String relativePath = project.getFile() == null
? null
: initialProject
- .getBasedir()
Unnecessary formatting change, please revert.
------------------------------
In
maven_plugin/src/main/java/io/github/chains_project/maven_lockfile/graph/DependencyGraph.java
<#1487 (comment)>
:
> @@ -80,7 +80,7 @@ private static Optional<DependencyNode> createDependencyNode(
Graph<org.apache.maven.shared.dependency.graph.DependencyNode> graph,
AbstractChecksumCalculator calc,
boolean isRoot,
- boolean reduce) {
+ boolean reduce,boolean skipTestScope) {
nit: format
⬇️ Suggested change
- boolean reduce,boolean skipTestScope) {
+ boolean reduce,
+ boolean skipTestScope) {
------------------------------
In
maven_plugin/src/main/java/io/github/chains_project/maven_lockfile/graph/DependencyGraph.java
<#1487 (comment)>
:
> + if (skipPluginTestDependency(node, isRoot, skipTestScope)) {
+ PluginLogManager.getLog().debug(
+ "Skipping plugin test-scope dependency: " + node.toNodeString());
+ return Optional.empty();
+ }
Can't we simplify this and avoid a separate function call?
(pseudocode below)
if (skipTestScope && !isRoot && MavenScope.Test.equals(scope)) {
...
}
------------------------------
In
maven_plugin/src/main/java/io/github/chains_project/maven_lockfile/LockFileFacade.java
<#1487 (comment)>
:
> @@ -301,14 +301,13 @@ private static Set<io.github.chains_project.maven_lockfile.graph.DependencyNode>
"Built graph with %d nodes for plugin %s",
graph.nodes().size(), pluginArtifact));
- DependencyGraph dependencyGraph = DependencyGraph.of(graph, checksumCalculator, false);
-
+ DependencyGraph dependencyGraph = DependencyGraph.of(graph, checksumCalculator,false, true);
Worth adding a comment here that we are excluding test scope.
⬇️ Suggested change
- DependencyGraph dependencyGraph = DependencyGraph.of(graph, checksumCalculator,false, true);
+ // Ignore test scoped dependencies for plugins.
+ DependencyGraph dependencyGraph = DependencyGraph.of(graph, checksumCalculator,false, true);
—
Reply to this email directly, view it on GitHub
<#1487 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/B5QQXSUHHV7WVUFXIRRGZNT4J6WTPAVCNFSM6AAAAACTV5LLOOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTONBRGUZDQMZRGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.