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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 22, 2024
+129 −3
Merged

Maven: Improve dependency resolution (for example for annotation processors like lombok) #8057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions java/maven/src/org/netbeans/modules/maven/api/PluginPropertyUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Properties;
import java.util.Set;
import org.apache.maven.artifact.Artifact;
Expand All @@ -39,6 +41,7 @@
import org.apache.maven.artifact.versioning.InvalidVersionSpecificationException;
import org.apache.maven.artifact.versioning.VersionRange;
import org.apache.maven.model.Dependency;
import org.apache.maven.model.DependencyManagement;
import org.apache.maven.model.InputLocation;
import org.apache.maven.model.Plugin;
import org.apache.maven.model.PluginExecution;
Expand Down Expand Up @@ -613,8 +616,10 @@ static class DependencyListBuilder implements ConfigurationBuilder<List<Dependen

private final String multiPropertyName;
private final String filterType;
private final DependencyManagement dependencyManagement;

public DependencyListBuilder(String multiPropertyName, String filterType) {
public DependencyListBuilder(DependencyManagement dependencyManagement, String multiPropertyName, String filterType) {
this.dependencyManagement = dependencyManagement;
this.multiPropertyName = multiPropertyName;
this.filterType = filterType;
}
Expand Down Expand Up @@ -668,6 +673,22 @@ public List<Dependency> build(Xpp3Dom configRoot, ExpressionEvaluator eval) {
item.setScope(s.getValue());
item.setLocation(PROP_SCOPE, (InputLocation)s.getInputLocation());
}
if (item.getVersion() == null && 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(), d.getType());
Copy link
Member

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.

Copy link
Contributor Author

@matthiasblaesing matthiasblaesing Dec 21, 2024 •
edited
Loading

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.

Copy link
Member

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 the dependency (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 ?

Copy link
Contributor Author

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:

grafik

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.

Copy link
Member

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

})
.findFirst()
.ifPresent(d -> {
item.setVersion(d.getVersion());
item.setLocation(PROP_VERSION, d.getLocation(PROP_VERSION));
});
}
coords.add(item);
}
return coords;
Expand Down Expand Up @@ -793,7 +814,7 @@ public String getDefaultScope() {
}

MavenProject mavenProject = projectImpl.getOriginalMavenProject();
DependencyListBuilder bld = new DependencyListBuilder(query.getPathProperty(), query.getArtifactType());
DependencyListBuilder bld = new DependencyListBuilder(mavenProject.getDependencyManagement(), query.getPathProperty(), query.getArtifactType());
List<Dependency> coordinates = PluginPropertyUtils.getPluginPropertyBuildable(mavenProject, query.getPluginGroupId(), query.getPluginArtifactId(), query.getGoal(), bld);
if (coordinates == null) {
return null;
Expand Down Expand Up @@ -833,7 +854,7 @@ public String getDefaultScope() {
artifact = new DefaultArtifact(
coord.getGroupId(),
coord.getArtifactId(),
"",
(VersionRange) null,
coord.getScope() == null ? query.getDefaultScope() : coord.getScope(),
coord.getType(),
coord.getClassifier(),
Expand Down
105 changes: 105 additions & 0 deletions java/maven/test/unit/src/org/netbeans/modules/maven/api/PluginPropertyUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,17 @@
package org.netbeans.modules.maven.api;

import java.io.File;
import java.io.IOException;
import java.io.StringReader;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import org.apache.maven.artifact.Artifact;
import org.apache.maven.artifact.resolver.ArtifactResolutionException;
import org.apache.maven.model.Dependency;
import org.codehaus.plexus.util.xml.Xpp3Dom;
import org.codehaus.plexus.util.xml.Xpp3DomBuilder;
import org.netbeans.api.project.Project;
import org.netbeans.api.project.ProjectManager;
import org.netbeans.junit.NbTestCase;
import org.openide.filesystems.FileObject;
Expand Down Expand Up @@ -202,6 +207,7 @@ public void testDependencyListBuilder() throws Exception {

// Matching filter for propertyItemName should yield correct result
PluginPropertyUtils.DependencyListBuilder bld = new PluginPropertyUtils.DependencyListBuilder(
null,
"annotationProcessorPaths",
null
);
Expand Down Expand Up @@ -266,10 +272,109 @@ public void testDependencyListBuilder() throws Exception {
// Filter with null value for propertyItemName should yield full list
Xpp3Dom configRoot2 = Xpp3DomBuilder.build(new StringReader(testPom2)).getChild("build").getChild("plugins").getChildren()[0].getChild("configuration");
PluginPropertyUtils.DependencyListBuilder bld2 = new PluginPropertyUtils.DependencyListBuilder(
null,
"annotationProcessorPaths",
null
);
List<Dependency> dependencies3 = bld2.build(configRoot2, PluginPropertyUtils.DUMMY_EVALUATOR);
assertEquals(2, dependencies3.size());
}

public void testDependencyBuilderWithDependencyManagement() throws IOException {
TestFileUtils.writeFile(d, "pom.xml",
"""
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<groupId>let.me.reproduce</groupId>
<artifactId>annotation-processor-netbeans-reproducer</artifactId>
<version>1.0-SNAPSHOT</version>
<packaging>jar</packaging>
<dependencyManagement>
<dependencies>
<dependency>
<groupId>org.projectlombok</groupId>
<artifactId>lombok</artifactId>
<version>1.18.36</version>
</dependency>
</dependencies>
</dependencyManagement>
<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<version>3.13.0</version>
<configuration>
<annotationProcessorPaths>
<path>
<groupId>org.projectlombok</groupId>
<artifactId>lombok</artifactId>
<type>jar</type>
</path>
</annotationProcessorPaths>
</configuration>
</plugin>
</plugins>
</build>
</project>
"""
);
Project project = ProjectManager.getDefault().findProject(d);
assert project != null;
PluginPropertyUtils.PluginConfigPathParams query = new PluginPropertyUtils.PluginConfigPathParams("org.apache.maven.plugins", "maven-compiler-plugin", "annotationProcessorPaths");
query.setDefaultScope(Artifact.SCOPE_RUNTIME);
query.setGoal("runtime");
List<ArtifactResolutionException> errorList = new ArrayList<>();
List<Artifact> artifacts = PluginPropertyUtils.getPluginPathProperty(project, query, true, errorList);
assertNotNull(artifacts);
assert artifacts != null;
assertEquals(1, artifacts.size());
assertEquals("org.projectlombok", artifacts.get(0).getGroupId());
assertEquals("lombok", artifacts.get(0).getArtifactId());
assertEquals("1.18.36", artifacts.get(0).getVersion());
assertNull(artifacts.get(0).getClassifier());
}

public void testDependencyBuilderWithoutVersion() throws IOException {
TestFileUtils.writeFile(d, "pom.xml",
"""
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<groupId>let.me.reproduce</groupId>
<artifactId>annotation-processor-netbeans-reproducer</artifactId>
<version>1.0-SNAPSHOT</version>
<packaging>jar</packaging>
<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<version>3.13.0</version>
<configuration>
<annotationProcessorPaths>
<path>
<groupId>org.projectlombok</groupId>
<artifactId>lombok</artifactId>
</path>
</annotationProcessorPaths>
</configuration>
</plugin>
</plugins>
</build>
</project>
"""
);
Project project = ProjectManager.getDefault().findProject(d);
assert project != null;
PluginPropertyUtils.PluginConfigPathParams query = new PluginPropertyUtils.PluginConfigPathParams("org.apache.maven.plugins", "maven-compiler-plugin", "annotationProcessorPaths");
query.setDefaultScope(Artifact.SCOPE_RUNTIME);
query.setGoal("runtime");
List<ArtifactResolutionException> errorList = new ArrayList<>();
List<Artifact> artifacts = PluginPropertyUtils.getPluginPathProperty(project, query, true, errorList);
assertNotNull(artifacts);
assert artifacts != null;
assertEquals(0, artifacts.size());
}
}
Loading