-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Type derive (simpler) #11380
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
base: master
Are you sure you want to change the base?
Type derive (simpler) #11380
Conversation
Instead to "smear" this feature across Maven and Resolver classes, for start let's keep it "confined" with single class: the TypeDeriver. Later we can see where to go further with it. Supersedes apache#11373
| */ | ||
| @Deprecated(since = "4.0.0") | ||
| public class DefaultType implements Type, ArtifactType { | ||
| public class DefaultType implements Type { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the benefit of this decoupling? It does not seem necessary.
|
Thanks. Just tried with the |
|
@desruisseaux that is weird, as I have locally (master of m-compiler-p): Invoked as: |
|
Aha, I think I see it now: |
|
Yes, the integration test passes because the line that cause a failure is commented out. If you open the following file: then uncomment the second line below: // TODO: pending https://github.com/apache/maven/pull/11373
// dependency.AnnotationProcessorDependency.foo();then, we get the test failure. You can also check that if the value on |
|
Blocked by apache/maven-resolver#1648 |
|
@desruisseaux if this build passes OK, you will have a Maven build (available as GH CI artifact here: https://github.com/apache/maven/actions/runs/19105278503?pr=11380) w/ resolver fix incorporated (will use 2.0.14-SNAPSHOT resolver) |
|
Using that Maven along with latest Toolbox you can inspect the tree output from Maven and how types are derived. |
|
Thanks! Will try this weekend. No worry for the github actions, I always build Maven core locally anyway. |
|
Funnily, GH Actions revealed several "leaks" (UT and IT bubbled up to checkout root), so I am just using this as a good test to fix all those leaks 😄 |
desruisseaux
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot really evaluate this part of Maven code, but I tested with the Maven Compiler Plugin and confirm that it worked. I was able to re-enable a processor dependency test which was commented-out.
Instead to "smear" this feature across Maven and Resolver classes, for start let's keep it "confined" with single class: the TypeDeriver.
Later we can see where to go further with it.
This PR also includes bugfix, where Maven
DefaultTypeimplementsArtifactType, while in reality it does not (violates contract by returningnullwhen no classifier present).Supersedes #11373