[MNG-8295] Dependency Manager Transitivity (now default) handles dependency management inconsistently

[gnodet]

IT PR: apache/maven-integration-testing#384
JIRA issue: MNG-8295

From @DidierLoiseau

Actually use TransitiveDependencyManager when maven.resolver.dependencyManagerTransitivity=true, instead of ClassicDependencyManager(true, …) as per #1357.

This fixes MNG-8295 and the integration tests proposed in apache/maven-integration-testing#384.

I thought that, since the change is trivial, it doesn’t cost much to create a PR for it already – even if the consensus turns out to be something else in the end.

---

Supersedes #1785 to align branch names

[gnodet]

Two failing ITs:

• MavenITmng2196ParentResolutionTest
• MavenITmng7836AlternativePomSyntaxTest

[DidierLoiseau]

I see you have now disabled MavenITmng2196ParentResolutionTest so I guess it is not relevant anymore.

Regarding MavenITmng7836AlternativePomSyntaxTest, it succeeds when running the integration tests… with maven built with this patch. I also had this issue at some point, and after running the integration tests like that, it was gone, even when running under 3.9.7. However I now realize that it’s because the dependencies were in `pwd`/repo after the run under the patched Maven 4. If the integration tests still have to run under Maven 3, then I suppose it would be good to understand what changes, and maybe something needs to be added to the bootstrap.txt.

The thing is, pom.hocon uses parent maven-extensions:41, which has maven-parent:41 as parent. The latter runs spotless-maven-plugin:2.40.0:check, which appears to need palantir-java-format:2.38.0 which is in the repository despite not appearing in spotless’ dependency tree. Palantir, however, depends on error_prone_annotations:2.19.1 and checker-qual:3.35.0, which are not in the repository. This seems a bit weird, but maybe there is a good reason for that?

Changing the test to run with -X -Dorg.slf4j.simpleLogger.showLogName=true, you can see the following:

[DEBUG] org.eclipse.aether.internal.impl.DefaultArtifactResolver - Resolving artifact com.google.errorprone:error_prone_annotations:pom:2.19.1 from [central (file:target/null, default, releases+snapshots), apache.snapshots (https://repository.apache.org/snapshots, default, snapshots)]
[DEBUG] org.eclipse.aether.internal.impl.DefaultArtifactResolver - Resolving artifact org.checkerframework:checker-qual:pom:3.35.0 from [central (file:target/null, default, releases+snapshots), apache.snapshots (https://repository.apache.org/snapshots, default, snapshots)]
[DEBUG] org.eclipse.aether.internal.impl.DefaultArtifactResolver - Resolving artifact com.google.errorprone:error_prone_annotations:pom:2.19.1 from [central (file:target/null, default, releases+snapshots), apache.snapshots (https://repository.apache.org/snapshots, default, snapshots)]
[DEBUG] org.eclipse.aether.internal.impl.DefaultArtifactResolver - Resolving artifact org.checkerframework:checker-qual:pom:3.35.0 from [central (file:target/null, default, releases+snapshots), apache.snapshots (https://repository.apache.org/snapshots, default, snapshots)]
[WARNING] org.apache.maven.internal.aether.DefaultRepositorySystemSessionFactory - The POM for org.checkerframework:checker-qual:jar:3.35.0 is missing, no dependency information available
[WARNING] org.apache.maven.internal.aether.DefaultRepositorySystemSessionFactory - The POM for com.google.errorprone:error_prone_annotations:jar:2.19.1 is missing, no dependency information available

For some reason however, this does not cause the build to fail on 4.0.0-beta-4 or the current master, but it does with the TransitiveDependencyManager.

It’s getting late so I’ll stop my investigations for today, but I put the test ouptut (with full stacktrace at the end) as a gist here.

[DidierLoiseau]

With a fresh mind, the answer is quite simple: 😅

• palantir-java-format:2.38.0 has <dependencyManagement> for error_prone_annotations:2.19.1 and checker-qual:3.35.0 – previously ignored
• it also depends on guava:32.1.2-jre, which has a direct dependency on error_prone_annotations:2.18.0 and checker-qual:3.33.0

So this change is perfectly expected – and I think it makes sense that, if Spotless requests palantir-java-format:2.38.0, it also gets the managed dependency versions.

I wanted to propose to add error_prone_annotations:2.18.0 and checker-qual:3.33.0 to bootstrap.txt but then… Spotless also loads google-java-format:1.17.0 (for <removeUnusedImports />), which depends on error_prone_annotations:2.16.0 and checker-qual:3.21.2! Spotless loads it in a separate call to DefaultRepositorySystem#resolveDependencies(), so the resolution is done independently.

With or without the change, BfDependencyCollector#doRecurse() will take google-java-format:1.17.0’s dependencies from the descriptorResult and put them as context.managedDependencies, except that the ClassicDependencyManager will ignore them whereas the TransitiveDependencyManager now keeps track of them. The trick is that google-java-format declares them both as <optional>, so they are not added to the dependencyProcessingQueue!

Back in the while loop of BfDependencyCollector#doCollectDependencies(), the dependencyProcessingQueue now contains just 1 entry: the guava:31.1-jre, also a dependency of google-java-format:1.17.0! It depends on error_prone_annotations:2.11.0 and checker-qual:3.12.0, so ClassicDependencyManager will happily use those. However, TransitiveDependencyManager remembers the versions specified by google-java-format:1.17.0, and will use them instead!

Basically, GJF says “those dependencies are optional” (this is kinda incorrect, actually, since it has a strong dependency on Guava, so they are actually mandatory), but if you include them (which will thus always be the case), I need these versions”. Here again, in my opinion, TransitiveDependencyManager seems right (although I hadn’t really thought about how <optional> dependencies influence the version selection until now).

Neither dependency manager takes the same versions of the 2 offending dependencies when doing resolution for Palantir and GJF, so I don’t think we should try to fix that now – it might actually be a bug in Spotless if the classpath of the two is merged.

The nasty trick is that Spotless 2.40 runs as part of Core ITs build, before any integration test runs. This causes the Spotless dependencies decided by ClassicDependencyManager to be cached without needing to add them in the bootstrap.txt. I suppose it works as long as Spotless 2.40 isn’t upgraded in the Core ITs, at which point you’ll need to align the version for the pom.hocon.

TL;DR

To make it work with TransitiveDependencyManager, I suppose you you could either disable Spotless in this test, disable Palantir and GJF for Spotless in this test, or add the following lines in bootstrap.txt:

com.google.errorprone:error_prone_annotations:2.16
com.google.errorprone:error_prone_annotations:2.19.1
org.checkerframework:checker-qual:3.21.2
org.checkerframework:checker-qual:3.35.0

As a side note, the error you get when a plugin dependency fails to download such dynamic dependencies is particularly unclear, as it never says which DependencyRequest it was actually trying to resolve (here Palantir and GJF) despite the very long stracktraces – but I guess that’s an issue for Maven Resolver.