Howdy, :beer: (or any other award you'd like, :wine:?)
Thanks for spotting and reporting this. Yes, there is a problem. Created issue https://issues.apache.org/jira/browse/MRESOLVER-357 In short, the problem lies here: https://github.com/apache/maven-resolver/blob/maven-resolver-1.9.9/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/ConflictResolver.java#L344-L366 The comment even "promises" that it will "leave only 1 loser" but the code does not fulfil this promise. Thanks T On Fri, Apr 28, 2023 at 9:59 AM Alexey Venderov <avende...@gmail.com> wrote: > Tamás, > > Thank you for the detailed explanation! > > One more question regarding the verbose mode in the old implementation vs. > the new implementation. I have this simple project > https://github.com/c00ler/simple-maven-project/tree/verbose-graph (all the > relevant code in the verbose-graph branch) where I override the version of > the junit-jupiter-params dependency > > https://github.com/c00ler/simple-maven-project/blob/verbose-graph/pom.xml#L42 > . > When I check its dependency tree using Maven 3.9.1 and Maven 4.0.0-SNAPSHOT > in the verbose mode I get slightly different results. > > Maven 3.9.1 (Maven 4.0.0-alpha-5 produces the same result) gives: > > 09:18:20 ➜ ./mvnw dependency:tree -D*aether.conflictResolver.verbose=true* > [INFO] Scanning for projects... > [INFO] > [INFO] -------------< com.github.avenderov:simple-maven-project > >-------------- > [INFO] Building simple-maven-project 1.0-SNAPSHOT > [INFO] from pom.xml > [INFO] --------------------------------[ jar > ]--------------------------------- > [INFO] > [INFO] --- dependency:3.5.0:tree (default-cli) @ simple-maven-project --- > [INFO] com.github.avenderov:simple-maven-project:jar:1.0-SNAPSHOT > [INFO] +- org.junit.jupiter:junit-jupiter:jar:5.9.1:test > [INFO] | +- org.junit.jupiter:junit-jupiter-api:jar:5.9.1:test > [INFO] | | +- org.opentest4j:opentest4j:jar:1.2.0:test > [INFO] | | +- org.junit.platform:junit-platform-commons:jar:1.9.1:test > [INFO] | | | \- org.apiguardian:apiguardian-api:jar:1.1.2:test > [INFO] | | \- org.apiguardian:apiguardian-api:jar:1.1.2:test > [INFO] | +- *org.junit.jupiter:junit-jupiter-params:jar:5.9.1:test* > [INFO] | \- org.junit.jupiter:junit-jupiter-engine:jar:5.9.1:test > [INFO] | +- org.junit.platform:junit-platform-engine:jar:1.9.1:test > [INFO] | | +- org.opentest4j:opentest4j:jar:1.2.0:test > [INFO] | | +- org.junit.platform:junit-platform-commons:jar:1.9.1:test > [INFO] | | \- org.apiguardian:apiguardian-api:jar:1.1.2:test > [INFO] | +- org.junit.jupiter:junit-jupiter-api:jar:5.9.1:test > [INFO] | \- org.apiguardian:apiguardian-api:jar:1.1.2:test > [INFO] \- *org.junit.jupiter:junit-jupiter-params:jar:5.9.3:test* > [INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.9.1:test > [INFO] \- org.apiguardian:apiguardian-api:jar:1.1.2:test > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 0.531 s > [INFO] Finished at: 2023-04-28T09:18:38+02:00 > [INFO] > ------------------------------------------------------------------------ > > Maven 4.0.0-SNAPSHOT gives: > > 09:18:38 ➜ > /Users/avenderov/tmp/maven/dist/apache-maven-4.0.0-SNAPSHOT/bin/mvn > dependency:tree -D*aether.conflictResolver.verbose=true* > -Dmaven.plugin.validation=disabled > [INFO] Scanning for projects... > [INFO] > [INFO] --------------------------------------< > com.github.avenderov:simple-maven-project > >--------------------------------------- > [INFO] Building simple-maven-project 1.0-SNAPSHOT > [INFO] from pom.xml > [INFO] ---------------------------------------------------------[ jar > ]---------------------------------------------------------- > [INFO] > [INFO] --- dependency:3.5.0:tree (default-cli) @ simple-maven-project --- > [INFO] com.github.avenderov:simple-maven-project:jar:1.0-SNAPSHOT > [INFO] +- org.junit.jupiter:junit-jupiter:jar:5.9.1:test > [INFO] | +- org.junit.jupiter:junit-jupiter-api:jar:5.9.1:test > [INFO] | | +- org.opentest4j:opentest4j:jar:1.2.0:test > [INFO] | | +- org.junit.platform:junit-platform-commons:jar:1.9.1:test > [INFO] | | | \- org.apiguardian:apiguardian-api:jar:1.1.2:test > [INFO] | | \- org.apiguardian:apiguardian-api:jar:1.1.2:test > [INFO] | \- org.junit.jupiter:junit-jupiter-engine:jar:5.9.1:test > [INFO] | +- org.junit.platform:junit-platform-engine:jar:1.9.1:test > [INFO] | | +- org.opentest4j:opentest4j:jar:1.2.0:test > [INFO] | | +- org.junit.platform:junit-platform-commons:jar:1.9.1:test > [INFO] | | \- org.apiguardian:apiguardian-api:jar:1.1.2:test > [INFO] | +- org.junit.jupiter:junit-jupiter-api:jar:5.9.1:test > [INFO] | \- org.apiguardian:apiguardian-api:jar:1.1.2:test > [INFO] \- *org.junit.jupiter:junit-jupiter-params:jar:5.9.3:test* > [INFO] +- org.junit.jupiter:junit-jupiter-api:jar:5.9.1:test > [INFO] \- org.apiguardian:apiguardian-api:jar:1.1.2:test > [INFO] > > -------------------------------------------------------------------------------------------------------------------------- > [INFO] BUILD SUCCESS > [INFO] > > -------------------------------------------------------------------------------------------------------------------------- > [INFO] Total time: 0.625 s > [INFO] Finished at: 2023-04-28T09:19:10+02:00 > [INFO] > > -------------------------------------------------------------------------------------------------------------------------- > [WARNING] > [WARNING] Plugin validation issues were detected in 1 plugin(s) > [WARNING] > > As can be seen, the output from the Maven 4.0.0-SNAPSHOT does not include > the node org.junit.jupiter:junit-jupiter-params:jar:5.9.1:test. This is why > I assumed the new STANDARD mode doesn't fully match the pre-1.9.8 true > mode. > > Best regards, > Alexey Venderov > mailto: avende...@gmail.com > > > On Tue, Apr 25, 2023 at 4:11 PM Tamás Cservenák <ta...@cservenak.net> > wrote: > > > Sorry, series of lapsues: > > so, verbose modes are: NONE, STANDARD and FULL. > > > > Wherever I wrote "DEFAULT" I meant "STANDARD" instead. > > > > The enum in question: > > > > /** > > * The enum representing verbosity levels of conflict resolver. > > * > > * @since 1.9.8 > > */ > > public enum Verbosity { > > /** > > * Verbosity level to be used in all "common" resolving use cases > > (ie. dependencies to build class path). The > > * {@link ConflictResolver} in this mode will trim down the graph > > to the barest minimum: will not leave > > * any conflicting node in place, hence no conflicts will be > > present in transformed graph either. > > */ > > NONE, > > > > /** > > * Verbosity level to be used in "analyze" resolving use cases > (ie. > > dependency convergence calculations). The > > * {@link ConflictResolver} in this mode will remove any > redundant > > collected nodes, in turn it will leave one > > * with recorded conflicting information. This mode corresponds > to > > "classic verbose" mode when > > * {@link #CONFIG_PROP_VERBOSE} was set to {@code true}. > Obviously, > > the resulting dependency tree is not > > * suitable for artifact resolution unless a filter is employed > to > > exclude the duplicate dependencies. > > */ > > STANDARD, > > > > /** > > * Verbosity level to be used in "analyze" resolving use cases > (ie. > > dependency convergence calculations). The > > * {@link ConflictResolver} in this mode will not remove any > > collected node, in turn it will record on all > > * eliminated nodes the conflicting information. Obviously, the > > resulting dependency tree is not suitable > > * for artifact resolution unless a filter is employed to exclude > > the duplicate dependencies. > > */ > > FULL > > } > > > > On Tue, Apr 25, 2023 at 4:04 PM Tamás Cservenák <ta...@cservenak.net> > > wrote: > > > > > Howdy, > > > > > > 1. Historically (and today) users (and client code needing verbose > tree, > > > like enforcer convergence rule) sets > > > > > > `aether.conflictResolver.verbose=true` > > > > > > in config properties of resolver session. Again, as this may happen by > > > user on CLI, or in code, like plugins, this part is unchanged. > > > And as you say, `aether.conflictResolver.verbose=true` means > > > Verbosity.DEFAULT, so "DEFAULT" is what in pre 1.9.8 releaser was > > > `aether.conflictResolver.verbose=true`. No change here. > > > > > > 2. I did not want to change/alter the "surface", so even the doco > > > https://maven.apache.org/resolver/configuration.html still states: > > > "aether.conflictResolver.verbose | boolean | Flag controlling the > > conflict > > > resolver's verbose mode." > > > > > > 3. The difference between Verbosity.DEFAULT and Verbosity.FULL really > > > matters ONLY when you have version ranges downstream (in the transitive > > > hull). > > > Otherwise the output of two Verbosity modes does not differ. > > > Modes are: > > > (pre 1.9.8 mode) NONE -- conflicted nodes are removed > > > (pre 1.9.8 mode) DEFAULT -- GA[V1-Vn]s are made "unique" (one left, > > others > > > removed), but GA[V1-Vn] may happen only IF there is version range > > > downstream (and version ranges are currently implemented in collector > > where > > > "one node is exploded into discovered/versionFiltered nodes" -> hence > you > > > end up with GA[V1-Vn]). > > > (new mode in 1.9.8) FULL -- nothing is removed from "ditty tree" (but > ALL > > > conflicted nodes are marked as "conflicted") > > > > > > Hopefully javadoc explains it > > > > > > https://github.com/apache/maven-resolver/blob/master/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/ConflictResolver.java#L70 > > > > > > 4. To be frank, the "FULL" mode was handy while fixing > > > https://issues.apache.org/jira/browse/MRESOLVER-345 > > > but in general I did not mean it as some "new feature" thing (hence the > > > doco did not change either). I just did not want to "throw away" all > this > > > execution path that did come handy to fix MRESOLVER-345 and may come > > handy > > > later for some other bug as well. > > > > > > 5. Finally, as I could not inspect ALL the code out there asking for > > > "verbose" graph, I just went on safe side, and did not want to > introduce > > > any (potential breaking) change, that existing code could choke on: for > > > example, existing code so far got one GAV, I could not guarantee it > would > > > not choke if the code would get GA[V1-Vn] instead, hence, the "default > > > verbose mode" is "STANDARD" -- same behaviour as it was before the > > change. > > > Basically the commit did not introduce any change, resolver still > behaves > > > same as pre-1.9.8 versions regarding `aether.conflictResolver.verbose` > > > configuration. > > > > > > HTH > > > Tamas > > > > > > On Tue, Apr 25, 2023 at 9:52 AM Alexey Venderov <avende...@gmail.com> > > > wrote: > > > > > >> Hi, > > >> > > >> I have a couple of questions regarding the recent changes to how > > >> *aether.conflictResolver.verbose* property is set: > > >> > > >> > > > https://github.com/apache/maven-resolver/blob/master/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/ConflictResolver.java#L106 > > >> . > > >> > > >> 1. To be able to set the verbosity level to *Verbosity.FULL* the > client > > >> code needs to have the maven-resolver 1.9.8 in their classpath, to > have > > an > > >> access to the Verbosity enum. Also, that means that it's not possible > to > > >> use system properties to set the verbosity level to *Verbosity.FULL*. > > >> Wouldn't it be better to check if the string property value contains > any > > >> Verbosity enum constant? Something along these lines: > > >> > > >> private static Verbosity getVerbosity(RepositorySystemSession > > >> session) { > > >> final Object verbosityValue = > > >> session.getConfigProperties().get(CONFIG_PROP_VERBOSE); > > >> if (verbosityValue instanceof Boolean) { > > >> return (Boolean) verbosityValue ? Verbosity.STANDARD : > > >> Verbosity.NONE; > > >> } else if (verbosityValue instanceof String) { > > >> String verbosityStringValue = verbosityValue.toString(); > > >> for (Verbosity verbosity : Verbosity.values()) { > > >> if (verbosity.name > > >> ().equalsIgnoreCase(verbosityStringValue)) > > >> { > > >> return verbosity; > > >> } > > >> } > > >> return Boolean.parseBoolean(verbosityStringValue) ? > > >> Verbosity.STANDARD : Verbosity.NONE; > > >> } else if (verbosityValue != null) { > > >> throw new IllegalArgumentException("Unsupported Verbosity > > >> configuration: " + verbosityValue); > > >> } > > >> return Verbosity.NONE; > > >> } > > >> > > >> 2. I'm not sure that I fully understand why the old *true* value is > > mapped > > >> to the new *Verbosity.STANDARD* value and not *Verbosity.FULL*. Isn't > > it a > > >> backward incompatible change? When verbosity was set to *true* the old > > >> implementation was not doing any deletions of the nodes, while the new > > >> implementation is doing some cleaning of the graph when verbosity is > set > > >> to > > >> *Verbosity.STANDARD*. > > >> > > >> Thank you! > > >> > > >> Best regards, > > >> Alexey Venderov > > >> mailto: avende...@gmail.com > > >> > > > > > >
