-
Notifications
You must be signed in to change notification settings - Fork 861
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
Maven: Improve dependency resolution (for example for annotation processors like lombok) #8057
Maven: Improve dependency resolution (for example for annotation processors like lombok) #8057
Conversation
Looks good to me to resolve at this point. This whole thing will need some review with Maven 4 as Also not sure if the existing code takes into account the value of |
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.
makes sense!
java/maven/src/org/netbeans/modules/maven/api/PluginPropertyUtils.java
Outdated
Show resolved
Hide resolved
} | ||
if (c != null) { | ||
item.setClassifier(c.getValue()); | ||
item.setLocation(PROP_CLASSIFIER, (InputLocation)c.getInputLocation()); | ||
} else if (dependencyFromDependencyManagement != null) { |
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 am not sure about this: the classifier
tags a differnt flavour of an artifact. So IMHO the dependency mgmt tagged with a classifier does not apply/manage a project dependency without any classifier (empty classifier).
IMHO group:artifact:classifier:type of the dependency must match the dependency mgmt decl in order to apply the version from the dependency management.
So if the dependency mgmt declares a classifier and the artifact does not, or the dependency mgmt declares a type different from the assumed artifact type, the management does not apply.
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.
You are right. I had a different use-case in mind, but that is not covered:
This might be handled differently in other plugins, but for now I adjusted the code accordingly. Pushed an update.
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.
@sdedic would you mind having another look?
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.
Apologies for the delay.
&& Objects.equals(item.getArtifactId(), d.getArtifactId()); | ||
&& Objects.equals(item.getArtifactId(), d.getArtifactId()) | ||
&& Objects.equals(item.getClassifier(), d.getClassifier()) | ||
&& Objects.equals(item.getType(), d.getType()); |
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.
Note that the compiler plugin your comment refers to defaults to jar
type, if the item.getType()
is not defned.
The usage in maven-compiler suggests that each Plugin can choose whatever default it wants :( for its dependencies ... but as long as maven-compiler-plugin is used, any compile
scope type-less artifact has to be `jar' (otherwise compilation would fail).
Could be solved with an additional type
parameter that would default to jar
.
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 don't get what you mean. Do you mean the type filter should not be there? Could please give a sample?
Edit: What is wrong here, is that I use the newly created dependency to early. I'm doing a fix right now.
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.
Apologies for unclear wording; maybe code would help to understand the intent/difference:
if (item.getVersion() == null || item.getScope() == null) {
String defaultType = "jar"; // PENDING: this could be an optional parameter
if (dependencyManagement != null && dependencyManagement.getDependencies() != null) {
dependencyManagement
.getDependencies()
.stream()
.filter(d -> {
return Objects.equals(item.getGroupId(), d.getGroupId())
&& Objects.equals(item.getArtifactId(), d.getArtifactId())
&& Objects.equals(item.getClassifier(), d.getClassifier())
&& Objects.equals(item.getType() != null ? item.getType() : defaultType, d.getType());
})
.findFirst()
.ifPresent(d -> {
if (item.getVersion() == null) {
item.setVersion(d.getVersion());
item.setLocation(PROP_VERSION, d.getLocation(PROP_VERSION));
}
if (item.getScope() == null) {
item.setVersion(d.getScope());
item.setLocation(PROP_SCOPE, d.getLocation(PROP_SCOPE));
}
});
}
- in order for dependency-management to match, group:artifact:classifierOpt:typeOpt have to be the same
- type defaults to
jar
if not specified in thedependency
(assuming the dep mgmt has type resolved before this code executes ? not sure about this) - on match, missing version/scope is supplied from dependency managament
What do you think ?
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.
Thanks for the update, but I don't think it is necessary. Both sides of the comparison are instances of org.apache.maven.model.Dependency
. That class is generated from this:
https://github.com/apache/maven/blob/maven-3.9.9/maven-model/src/main/mdo/maven.mdo#L1096-L1109
and the decompiler gives me the expected:
The type is jar
unless overridden. So I don't see how getType
can return null, unless explicitly set from the java side and if not specificied will be jar
.
I moved the version update after the initialization from user input, so that we get the Dependency in a state as far initialized as the user did.
I would change/set the scope at least I don't see a use there. For dependency resolution I know that
- groupId
- artifactId
- version
- classifier
- type
take part, I never saw scope, so I would currently not mess with that.
So I think the now pushed version should be safe.
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.
Thanks for your explanation/patience
…essors like lombok) Resolution of processors also needs to take into account, that for example the version for an artifact could be provided by DependencyManagement. Closes: apache#8054 Closes: apache#8041
1131ee4
to
6b86a36
Compare
@sdedic @mbien @neilcsmith-net thank you all for checking/commenting I found the discussions points very helpful. |
Resolution of processors also needs to take into account, that for example the version for an artifact could be provided by DependencyManagement.
Closes: #8054
Closes: #8041