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

Rewrite dependencies resolution in AbstractResolveDependencies. #303

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wilx
Copy link
Contributor

@wilx wilx commented Jun 28, 2024

Use repositorySystem.resolveDependencies() to fold collection and resolution steps into one.

Use filters for resolveDependencies() call. Do not pre-filter dependencies.

Fixes #302.

wilx added 2 commits June 28, 2024 21:21
Use repositorySystem.resolveDependencies() to fold collection and resolution steps
into one.

Use filters for resolveDependencies() call. Do not pre-filter dependencies.

Fixes mojohaus#302.
Allow slight variation in expected messages.
@slawekjaranowski
Copy link
Member

Thanks for PR.
I will check it, but afraid that we should filter artifacts by scope before resolving/collecting it.

dependency:tree has a bug for such cases https://issues.apache.org/jira/browse/MDEP-909

@cstamas please also look

@slawekjaranowski slawekjaranowski self-assigned this Jun 29, 2024
@wilx
Copy link
Contributor Author

wilx commented Jun 29, 2024

I will check it, but afraid that we should filter artifacts by scope before resolving/collecting it.

Why? The filter does it for us during the collection. For my test case, the provided dependency gets filtered out correctly.

@wilx
Copy link
Contributor Author

wilx commented Jun 29, 2024

When I was looking into this, I looked around the other sources. I saw org.apache.maven.project.DefaultProjectDependenciesResolver#resolve. It looks like the original code was trying to do what the DefaultProjectDependenciesResolver#resolve does, but the original code lacks few of the hacks that the DefaultProjectDependenciesResolver#resolve has. I was wondering, if that is why it was returning different results.

@cstamas
Copy link

cstamas commented Jun 29, 2024

This looks correct in Maven3 universe. Due being in "maven3 universe" it suffers from MNG-8041, but is acceptable.

@slawekjaranowski
Copy link
Member

@cstamas we should be consequently - we did filtering before resolving in m-assembly-p
apache/maven-assembly-plugin#166

So how plugins should resolve dependencies tree with given scope ...?

another issue with the similar problem https://issues.apache.org/jira/browse/MINVOKER-368

@cstamas
Copy link

cstamas commented Jun 29, 2024

Right. This all evolves around MNG-8041 where maven3 provides "wrong runtime" classpath/tree. Reason is that at the "start" in includes unwanted scopes like test scope, that "eclipses" possible runtime dependencies lower in tree, and finally dep filter removes "test" scope as well (and the losers are removed by conflict resolution before that) and classpath ends up being incomplete, as explained here: https://maven.apache.org/resolver-archives/resolver-2.0.0-alpha-11/common-misconceptions.html

So, I do agree with @slawekjaranowski we should align these plugin scattered "fixes", until Maven4 API generally provides fixed and one-shop solution for all.

@slachiewicz slachiewicz marked this pull request as draft December 22, 2024 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

banDuplicateClasses incorrectly finds duplicates
4 participants