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 |
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.
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.
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.
@sdedic would you mind having another look?
| && 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.
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.
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.
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
jarif 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.
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.
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