Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow dependencies to appear in graph multiple times #187

Merged
merged 3 commits into from May 23, 2022
Merged

Allow dependencies to appear in graph multiple times #187

merged 3 commits into from May 23, 2022

Conversation

ThomGeG
Copy link
Contributor

@ThomGeG ThomGeG commented May 18, 2022

Resolves #116 by updating the CycloneDX MOJOs to use the same mechanism mvn dependency:tree -Dverbose does when building the dependency graph.

This PR also moves to the latest version of maven-dependency-tree to address a bug the previous version contained that would otherwise result in this change breaking the graphs for WARs.

The existing POM has been updated with a different set of dependencies 
that reproduce the problem, however don't cause errors in the console

Signed-off-by: Thomas Gaskell <tgegaskell@gmail.com>
The DependencyCollectorBuilder is what maven uses when in a verbose 
mode, so it has more information. Most noteably the duplicate nodes in 
the tree when something is a depenedncy of multiple parents.

Signed-off-by: Thomas Gaskell <tgegaskell@gmail.com>
Addresses MSHARED-994, which prevents building the graph for WARs

Signed-off-by: Thomas Gaskell <tgegaskell@gmail.com>
@fmarot
Copy link

fmarot commented May 18, 2022

Hello, I just want to warn that to my knowledge dependency:tree -Dverbose has been deprecated for a long time and it is stated to "may give wrong results when used with Maven 3". See the comment in the code: https://github.com/apache/maven-dependency-plugin/blob/master/src/main/java/org/apache/maven/plugins/dependency/tree/TreeMojo.java#L160

Sorry I may be off topic but I thought I better add this warning.

@ThomGeG
Copy link
Contributor Author

ThomGeG commented May 18, 2022

Cheers @fmarot

Looking into some of the history, it seems this has come full circle.

apache/maven-dependency-plugin@1852329

The warning in the JavaDoc there links to https://issues.apache.org/jira/browse/MDEP-443, which specifies the inconsistency with the move to Maven 3. In the current version of the dependency however, all the actual console warnings have been removed.

https://issues.apache.org/jira/browse/MDEP-644 then seems to imply the issue has been fixed and just needed to be released, which is actually the ticket Steve was previously talking about in the original issue #116.

It looks like whatever the issue was may have been fixed, but the JavaDoc was never updated. I'll have to look into it more tomorrow unless anybody else knows more

@ThomGeG
Copy link
Contributor Author

ThomGeG commented May 19, 2022

I'm back on a new morn, and it looks like my hunch was right.

The -Dverbose option was added back to the maven-dependency-plugin in MDEP-644, however they didn't also remove the warning in the JavaDoc on the actual parameter itself. The gargantuan commit that re-implements it instead just removes the placeholder logs and adds the necessary logic: apache/maven-dependency-plugin@10398dd#diff-fff95abc51baf25ef97506144eaf80d4c07ec4f598eb0110491897850504ac1aL233-L238

Interestingly, that incarnation of the builder is different from what is currently used: apache/maven-dependency-plugin@d9d34c6#diff-fff95abc51baf25ef97506144eaf80d4c07ec4f598eb0110491897850504ac1aR272

The latest logic uses the DependencyCollectorBuilder from maven-dependency-tree, which has an implementation specifically made for Maven 3 and another for Maven 3.1+: https://github.com/apache/maven-dependency-tree/blob/c001873c14b15be7468e4dc6e44ec0d73f4f9b37/src/main/java/org/apache/maven/shared/dependency/graph/internal/Maven31DependencyCollectorBuilder.java#L138

The degraph-maven-plugin uses a very slimmed-down version of much of the same code to generate its dependency graphs:
https://github.com/ferstl/depgraph-maven-plugin/blob/5671441d39b94337ceed74bd37852251a970e29c/src/main/java/com/github/ferstl/depgraph/dependency/MavenGraphAdapter.java#L67


It seems to me like breaking the -Dverbose param was just a transitionary thing from the jump between Maven 2 and Maven 3 (hence why the warning in the JavaDoc says it uses a Maven 2 algorithm) however when this was addressed and native Maven 3 & Maven 3.1+ equivalents were added, nobody noticed that the JavaDoc warning would need updating.

@fmarot
Copy link

fmarot commented May 19, 2022

Thanks you very much for this deep investigation. I have been stuck thinking I should not use -Dverbose for a looooong time. If you do not submit a PR to correct the javadoc, I'll do it.

@fmarot
Copy link

fmarot commented May 23, 2022

PR submitted to correct the javadoc: fmarot/maven-dependency-plugin#1

@stevespringett
Copy link
Member

Thank you so much for the investigation and PR for this @ThomGeG 🎉

@stevespringett stevespringett merged commit 46579e7 into CycloneDX:master May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependency Graph missing transitive dependencies when dependency has multiple sources
3 participants