You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "psiroky (via GitHub)" <gi...@apache.org> on 2023/02/15 21:33:25 UTC
[GitHub] [maven-compiler-plugin] psiroky opened a new pull request, #180: [MCOMPILER-391] Get annotation processor version from dependencyManagement
psiroky opened a new pull request, #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180
Creating this as a draft for now since the code is pretty rough and I would like to get some feedback on the approach itself before cleaning things up.
I did not find a way to submit a resolve request, without actually filling in the versions of the direct dependencies of the imaginary root the `maven-resolver` uses (as described in https://github.com/apache/maven-resolver/blob/maven-resolver-1.9.4/maven-resolver-api/src/main/java/org/eclipse/aether/collection/CollectRequest.java#L96). One of the options is to simply try to find the versions "manually", in the compiler code. This is easy enough and the only concern I have is performance, since `dependencyManagement` sections can get quite large (e.g. thousands of deps), going through them for every single sub-module could be costly. If I take a somewhat extreme case of Quarkus, its `dependencyManagement` section contains ~2k dependencies, and about 500 of the modules in its build are using annotation processors. In the worst case that's going 500 times over an array list with 2k items. Not the end of the world of course, but potentially something to improve via e.g. caching or so
.
This is only the first part of the issue related to re-using `dependencyManagement` for annotation processor dependencies. It uses the managed versions of the top-level entries, not their transitive dependencies (that would potentially come later as that is a change with potentially larger consequences to the users).
Following this checklist to help us incorporate your
contribution quickly and easily:
- [x] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MCOMPILER) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [x] Each commit in the pull request should have a meaningful subject line and body.
- [x] Format the pull request title like `[MCOMPILER-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `MCOMPILER-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
- [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [x] You have run the integration tests successfully (`mvn -Prun-its clean verify`).
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [x] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [x] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] cstamas commented on a diff in pull request #180: [MCOMPILER-391] Get annotation processor version from dependencyManagement
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#discussion_r1107757707
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1604,22 +1606,39 @@ private List<String> resolveProcessorPathEntries() throws MojoExecutionException
}
}
- private List<Dependency> convertToDependencies(List<DependencyCoordinate> annotationProcessorPaths) {
+ private List<Dependency> convertToDependencies(List<DependencyCoordinate> annotationProcessorPaths)
+ throws MojoExecutionException {
List<Dependency> dependencies = new ArrayList<>();
for (DependencyCoordinate annotationProcessorPath : annotationProcessorPaths) {
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler(annotationProcessorPath.getType());
+ String version = annotationProcessorPath.getVersion();
+ if (version == null) {
+ version = findManagedVersion(annotationProcessorPath)
+ .orElseThrow(
+ () -> new MojoExecutionException("Cannot find version for " + annotationProcessorPath));
+ }
Review Comment:
Well, even in case of dependencies, it is maven that does this (by calculating effective POM), not resolver :smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps [maven-compiler-plugin]
Posted by "famod (via GitHub)" <gi...@apache.org>.
famod commented on PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#issuecomment-1876921475
Just a heads up: This implementation does not seem to respect relocations, e.g. I was still using https://mvnrepository.com/artifact/org.hibernate/hibernate-jpamodelgen in my plugin config but the Quarkus BOM contains (only) the new and proper group id `org.hibernate.orm` and not the old one ``org.hibernate.` and so the annotation processor path version lookup logic didn't find it. Took me a while to figure out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] psiroky commented on a diff in pull request #180: [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps
Posted by "psiroky (via GitHub)" <gi...@apache.org>.
psiroky commented on code in PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#discussion_r1121558571
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -330,6 +335,22 @@ public abstract class AbstractCompilerMojo extends AbstractMojo {
@Parameter
private List<DependencyCoordinate> annotationProcessorPaths;
+ /**
+ * <p>
+ * Whether to use the Maven dependency management section when resolving transitive dependencies of annotation
+ * processor paths.
+ * </p>
+ * <p>
+ * This flag does not enable / disable the ability to get the version of annotation processor paths
+ * (the top-level paths) from dependency management section. It only influences the resolution of
+ * transitive dependencies of those top-level paths.
+ * </p>
+ *
+ * @since 3.12.0
Review Comment:
New feature, so I think this should go into the next minor version (not patch).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] cstamas commented on a diff in pull request #180: [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#discussion_r1121549459
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -330,6 +335,22 @@ public abstract class AbstractCompilerMojo extends AbstractMojo {
@Parameter
private List<DependencyCoordinate> annotationProcessorPaths;
+ /**
+ * <p>
+ * Whether to use the Maven dependency management section when resolving transitive dependencies of annotation
+ * processor paths.
+ * </p>
+ * <p>
+ * This flag does not enable / disable the ability to get the version of annotation processor paths
+ * (the top-level paths) from dependency management section. It only influences the resolution of
+ * transitive dependencies of those top-level paths.
+ * </p>
+ *
+ * @since 3.12.0
+ */
+ @Parameter(defaultValue = "false")
Review Comment:
:beer: for this: new feature, if alters behavior, should be disabled by def
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] psiroky commented on a diff in pull request #180: [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps
Posted by "psiroky (via GitHub)" <gi...@apache.org>.
psiroky commented on code in PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#discussion_r1121557872
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -330,6 +335,22 @@ public abstract class AbstractCompilerMojo extends AbstractMojo {
@Parameter
private List<DependencyCoordinate> annotationProcessorPaths;
+ /**
+ * <p>
+ * Whether to use the Maven dependency management section when resolving transitive dependencies of annotation
+ * processor paths.
+ * </p>
+ * <p>
+ * This flag does not enable / disable the ability to get the version of annotation processor paths
+ * (the top-level paths) from dependency management section. It only influences the resolution of
+ * transitive dependencies of those top-level paths.
+ * </p>
+ *
+ * @since 3.12.0
+ */
+ @Parameter(defaultValue = "false")
+ private boolean annotationProcessorPathsUseDepMgmt;
Review Comment:
I am not the biggest fan of this long name, but I could not find a better one at this point. Reason I choose this one is that the prefix `annotationProcessorPaths` means it will be rendered/shown together with the paths option, meaning users are much more likely to see this when browsing the docs (and making the connection that is used for annotation processor paths). We could use something like `useDepMgmt` or so, but then you need to know exactly what you are looking for, since this would be rendered very far away from the `annotationProcessorPaths`. Also something like `useDepMgmt` may be too generic?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] slawekjaranowski commented on a diff in pull request #180: [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on code in PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#discussion_r1142681687
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1584,42 +1605,91 @@ private List<String> resolveProcessorPathEntries() throws MojoExecutionException
return null;
}
- Set<String> elements = new LinkedHashSet<>();
try {
- List<Dependency> dependencies = convertToDependencies(annotationProcessorPaths);
+ List<org.eclipse.aether.graph.Dependency> dependencies = convertToDependencies(annotationProcessorPaths);
+ List<org.eclipse.aether.graph.Dependency> managedDependencies =
+ getManagedDependenciesForAnnotationProcessorPaths();
CollectRequest collectRequest =
- new CollectRequest(dependencies, Collections.emptyList(), project.getRemoteProjectRepositories());
+ new CollectRequest(dependencies, managedDependencies, project.getRemoteProjectRepositories());
DependencyRequest dependencyRequest = new DependencyRequest();
dependencyRequest.setCollectRequest(collectRequest);
DependencyResult dependencyResult =
repositorySystem.resolveDependencies(session.getRepositorySession(), dependencyRequest);
- for (ArtifactResult resolved : dependencyResult.getArtifactResults()) {
- elements.add(resolved.getArtifact().getFile().getAbsolutePath());
- }
- return new ArrayList<>(elements);
+ return dependencyResult.getArtifactResults().stream()
+ .map(resolved -> resolved.getArtifact().getFile().getAbsolutePath())
+ .collect(Collectors.toList());
} catch (Exception e) {
throw new MojoExecutionException(
"Resolution of annotationProcessorPath dependencies failed: " + e.getLocalizedMessage(), e);
}
}
- private List<Dependency> convertToDependencies(List<DependencyCoordinate> annotationProcessorPaths) {
- List<Dependency> dependencies = new ArrayList<>();
+ private List<org.eclipse.aether.graph.Dependency> convertToDependencies(
+ List<DependencyCoordinate> annotationProcessorPaths) throws MojoExecutionException {
+ List<org.eclipse.aether.graph.Dependency> dependencies = new ArrayList<>();
for (DependencyCoordinate annotationProcessorPath : annotationProcessorPaths) {
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler(annotationProcessorPath.getType());
+ String version = getAnnotationProcessorPathVersion(annotationProcessorPath);
Artifact artifact = new DefaultArtifact(
annotationProcessorPath.getGroupId(),
annotationProcessorPath.getArtifactId(),
annotationProcessorPath.getClassifier(),
handler.getExtension(),
- annotationProcessorPath.getVersion());
+ version);
Set<Exclusion> exclusions = convertToAetherExclusions(annotationProcessorPath.getExclusions());
- dependencies.add(new Dependency(artifact, JavaScopes.RUNTIME, false, exclusions));
+ dependencies.add(new org.eclipse.aether.graph.Dependency(artifact, JavaScopes.RUNTIME, false, exclusions));
}
return dependencies;
}
+ private String getAnnotationProcessorPathVersion(DependencyCoordinate annotationProcessorPath)
+ throws MojoExecutionException {
+ String configuredVersion = annotationProcessorPath.getVersion();
+ if (configuredVersion != null) {
+ return configuredVersion;
+ } else {
+ List<Dependency> managedDependencies = getProjectManagedDependencies();
+ return findManagedVersion(annotationProcessorPath, managedDependencies)
+ .orElseThrow(() -> new MojoExecutionException(String.format(
+ "Cannot find version for annotation processor path '%s'. The version needs to be either"
+ + " provided directly in the plugin configuration or via dependency management.",
+ annotationProcessorPath)));
+ }
+ }
+
+ private Optional<String> findManagedVersion(
+ DependencyCoordinate dependencyCoordinate, List<Dependency> managedDependencies) {
+ return managedDependencies.stream()
+ .filter(dep -> Objects.equals(dep.getGroupId(), dependencyCoordinate.getGroupId())
+ && Objects.equals(dep.getArtifactId(), dependencyCoordinate.getArtifactId())
+ && Objects.equals(dep.getClassifier(), dependencyCoordinate.getClassifier())
+ && Objects.equals(dep.getType(), dependencyCoordinate.getType()))
+ .findAny()
+ .map(org.apache.maven.model.Dependency::getVersion);
+ }
+
+ private List<org.eclipse.aether.graph.Dependency> getManagedDependenciesForAnnotationProcessorPaths() {
+ if (!annotationProcessorPathsUseDepMgmt) {
+ return Collections.emptyList();
+ }
+ List<Dependency> projectManagedDependencies = getProjectManagedDependencies();
+ ArtifactTypeRegistry artifactTypeRegistry =
+ session.getRepositorySession().getArtifactTypeRegistry();
+
+ return projectManagedDependencies.stream()
+ .map(dep -> RepositoryUtils.toDependency(dep, artifactTypeRegistry))
Review Comment:
If we need caching, it should be introduced in higher level not in plugin.
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1584,42 +1605,91 @@ private List<String> resolveProcessorPathEntries() throws MojoExecutionException
return null;
}
- Set<String> elements = new LinkedHashSet<>();
try {
- List<Dependency> dependencies = convertToDependencies(annotationProcessorPaths);
+ List<org.eclipse.aether.graph.Dependency> dependencies = convertToDependencies(annotationProcessorPaths);
+ List<org.eclipse.aether.graph.Dependency> managedDependencies =
+ getManagedDependenciesForAnnotationProcessorPaths();
CollectRequest collectRequest =
- new CollectRequest(dependencies, Collections.emptyList(), project.getRemoteProjectRepositories());
+ new CollectRequest(dependencies, managedDependencies, project.getRemoteProjectRepositories());
DependencyRequest dependencyRequest = new DependencyRequest();
dependencyRequest.setCollectRequest(collectRequest);
DependencyResult dependencyResult =
repositorySystem.resolveDependencies(session.getRepositorySession(), dependencyRequest);
- for (ArtifactResult resolved : dependencyResult.getArtifactResults()) {
- elements.add(resolved.getArtifact().getFile().getAbsolutePath());
- }
- return new ArrayList<>(elements);
+ return dependencyResult.getArtifactResults().stream()
+ .map(resolved -> resolved.getArtifact().getFile().getAbsolutePath())
+ .collect(Collectors.toList());
} catch (Exception e) {
throw new MojoExecutionException(
"Resolution of annotationProcessorPath dependencies failed: " + e.getLocalizedMessage(), e);
}
}
- private List<Dependency> convertToDependencies(List<DependencyCoordinate> annotationProcessorPaths) {
- List<Dependency> dependencies = new ArrayList<>();
+ private List<org.eclipse.aether.graph.Dependency> convertToDependencies(
+ List<DependencyCoordinate> annotationProcessorPaths) throws MojoExecutionException {
+ List<org.eclipse.aether.graph.Dependency> dependencies = new ArrayList<>();
for (DependencyCoordinate annotationProcessorPath : annotationProcessorPaths) {
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler(annotationProcessorPath.getType());
+ String version = getAnnotationProcessorPathVersion(annotationProcessorPath);
Artifact artifact = new DefaultArtifact(
annotationProcessorPath.getGroupId(),
annotationProcessorPath.getArtifactId(),
annotationProcessorPath.getClassifier(),
handler.getExtension(),
- annotationProcessorPath.getVersion());
+ version);
Set<Exclusion> exclusions = convertToAetherExclusions(annotationProcessorPath.getExclusions());
- dependencies.add(new Dependency(artifact, JavaScopes.RUNTIME, false, exclusions));
+ dependencies.add(new org.eclipse.aether.graph.Dependency(artifact, JavaScopes.RUNTIME, false, exclusions));
}
return dependencies;
}
+ private String getAnnotationProcessorPathVersion(DependencyCoordinate annotationProcessorPath)
+ throws MojoExecutionException {
+ String configuredVersion = annotationProcessorPath.getVersion();
+ if (configuredVersion != null) {
+ return configuredVersion;
+ } else {
+ List<Dependency> managedDependencies = getProjectManagedDependencies();
+ return findManagedVersion(annotationProcessorPath, managedDependencies)
+ .orElseThrow(() -> new MojoExecutionException(String.format(
+ "Cannot find version for annotation processor path '%s'. The version needs to be either"
+ + " provided directly in the plugin configuration or via dependency management.",
+ annotationProcessorPath)));
+ }
+ }
+
+ private Optional<String> findManagedVersion(
+ DependencyCoordinate dependencyCoordinate, List<Dependency> managedDependencies) {
+ return managedDependencies.stream()
+ .filter(dep -> Objects.equals(dep.getGroupId(), dependencyCoordinate.getGroupId())
+ && Objects.equals(dep.getArtifactId(), dependencyCoordinate.getArtifactId())
+ && Objects.equals(dep.getClassifier(), dependencyCoordinate.getClassifier())
+ && Objects.equals(dep.getType(), dependencyCoordinate.getType()))
+ .findAny()
+ .map(org.apache.maven.model.Dependency::getVersion);
+ }
Review Comment:
We can mention such case at annotationProcessorPaths documentation, that missing version can impact on performance with big dependency management.
We can also consider use of `parallelStream`, there is no finish collector so should be safe
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] psiroky commented on pull request #180: [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps
Posted by "psiroky (via GitHub)" <gi...@apache.org>.
psiroky commented on PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#issuecomment-1450781593
I ran a quick performance test using the Quarkus repo (1k modules, where 500 of them are using annotation processors) and I could not really spot any noticeable difference in the overall build time. I am sure there is more "work" being done (e.g. search those dependency management lists), but it seems it gets lost in the rest of the work being done as part of the build. I ran the following command:
```
mvn387 clean install -DskipTests -Dversion.enforcer.plugin=3.2.1 -Dversion.compiler.plugin=3.11.1-SNAPSHOT -T8
```
with three different configurations:
* default (no dependency management usage) - option 1) above
* using dep mgmt to only get the top-level versions - option 3) above
* using dep mgmt to get the top-level versions and also to resolve transitive dependencies - option 4)
In all cases I got build times ranging from 01:49min to 01:52min. No noticeable difference overall.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] psiroky commented on a diff in pull request #180: [MCOMPILER-391] Get annotation processor version from dependencyManagement
Posted by "psiroky (via GitHub)" <gi...@apache.org>.
psiroky commented on code in PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#discussion_r1107752394
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1604,22 +1606,39 @@ private List<String> resolveProcessorPathEntries() throws MojoExecutionException
}
}
- private List<Dependency> convertToDependencies(List<DependencyCoordinate> annotationProcessorPaths) {
+ private List<Dependency> convertToDependencies(List<DependencyCoordinate> annotationProcessorPaths)
+ throws MojoExecutionException {
List<Dependency> dependencies = new ArrayList<>();
for (DependencyCoordinate annotationProcessorPath : annotationProcessorPaths) {
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler(annotationProcessorPath.getType());
+ String version = annotationProcessorPath.getVersion();
+ if (version == null) {
+ version = findManagedVersion(annotationProcessorPath)
+ .orElseThrow(
+ () -> new MojoExecutionException("Cannot find version for " + annotationProcessorPath));
+ }
Review Comment:
It would be ideal if this wasn't needed and we could just rely on `maven-resolver` to simply figure out the version (if it's null) from the supplied list of managed versions. However, I did not find a way to create a `CollectRequest` which would enable this. The resolving always fails with
```
Caused by: java.lang.IllegalArgumentException: version can neither be null, empty nor blank
at org.apache.commons.lang3.Validate.notBlank(Validate.java:454)
at org.apache.maven.artifact.ArtifactUtils.notBlank(ArtifactUtils.java:107)
at org.apache.maven.artifact.ArtifactUtils.key(ArtifactUtils.java:97)
at org.apache.maven.ReactorReader.findArtifact(ReactorReader.java:101)
at org.eclipse.aether.internal.impl.DefaultArtifactResolver.resolve(DefaultArtifactResolver.java:306)
at org.eclipse.aether.internal.impl.DefaultArtifactResolver.resolveArtifacts(DefaultArtifactResolver.java:229)
at org.eclipse.aether.internal.impl.DefaultArtifactResolver.resolveArtifact(DefaultArtifactResolver.java:207)
at org.apache.maven.repository.internal.DefaultArtifactDescriptorReader.loadPom(DefaultArtifactDescriptorReader.java:240)
at org.apache.maven.repository.internal.DefaultArtifactDescriptorReader.readArtifactDescriptor(DefaultArtifactDescriptorReader.java:171)
at org.eclipse.aether.internal.impl.collect.DefaultDependencyCollector.resolveCachedArtifactDescriptor(DefaultDependencyCollector.java:538)
at org.eclipse.aether.internal.impl.collect.DefaultDependencyCollector.getArtifactDescriptorResult(DefaultDependencyCollector.java:523)
at org.eclipse.aether.internal.impl.collect.DefaultDependencyCollector.processDependency(DefaultDependencyCollector.java:410)
at org.eclipse.aether.internal.impl.collect.DefaultDependencyCollector.processDependency(DefaultDependencyCollector.java:362)
at org.eclipse.aether.internal.impl.collect.DefaultDependencyCollector.process(DefaultDependencyCollector.java:349)
at org.eclipse.aether.internal.impl.collect.DefaultDependencyCollector.collectDependencies(DefaultDependencyCollector.java:254)
at org.eclipse.aether.internal.impl.DefaultRepositorySystem.resolveDependencies(DefaultRepositorySystem.java:309)
at org.apache.maven.plugin.compiler.AbstractCompilerMojo.resolveProcessorPathEntries(AbstractCompilerMojo.java:1595)
```
I may have missed something, so any suggestions are more than welcome.
Note: the code is a first draft and could definitely use some improvements, but first I would like to know if this is even a viable way to go.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] psiroky commented on a diff in pull request #180: [MCOMPILER-391] Get annotation processor version from dependencyManagement
Posted by "psiroky (via GitHub)" <gi...@apache.org>.
psiroky commented on code in PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#discussion_r1107752394
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1604,22 +1606,39 @@ private List<String> resolveProcessorPathEntries() throws MojoExecutionException
}
}
- private List<Dependency> convertToDependencies(List<DependencyCoordinate> annotationProcessorPaths) {
+ private List<Dependency> convertToDependencies(List<DependencyCoordinate> annotationProcessorPaths)
+ throws MojoExecutionException {
List<Dependency> dependencies = new ArrayList<>();
for (DependencyCoordinate annotationProcessorPath : annotationProcessorPaths) {
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler(annotationProcessorPath.getType());
+ String version = annotationProcessorPath.getVersion();
+ if (version == null) {
+ version = findManagedVersion(annotationProcessorPath)
+ .orElseThrow(
+ () -> new MojoExecutionException("Cannot find version for " + annotationProcessorPath));
+ }
Review Comment:
It would be ideal if this wasn't needed and we could just rely on `maven-resolver` to simply figure out the version (if it's null) from the supplied list of managed versions. However, I did not find a way to create a `CollectRequest` which would enable this. The resolving always fails with
```
Caused by: java.lang.IllegalArgumentException: version can neither be null, empty nor blank
at org.apache.commons.lang3.Validate.notBlank(Validate.java:454)
at org.apache.maven.artifact.ArtifactUtils.notBlank(ArtifactUtils.java:107)
at org.apache.maven.artifact.ArtifactUtils.key(ArtifactUtils.java:97)
at org.apache.maven.ReactorReader.findArtifact(ReactorReader.java:101)
at org.eclipse.aether.internal.impl.DefaultArtifactResolver.resolve(DefaultArtifactResolver.java:306)
at org.eclipse.aether.internal.impl.DefaultArtifactResolver.resolveArtifacts(DefaultArtifactResolver.java:229)
at org.eclipse.aether.internal.impl.DefaultArtifactResolver.resolveArtifact(DefaultArtifactResolver.java:207)
at org.apache.maven.repository.internal.DefaultArtifactDescriptorReader.loadPom(DefaultArtifactDescriptorReader.java:240)
at org.apache.maven.repository.internal.DefaultArtifactDescriptorReader.readArtifactDescriptor(DefaultArtifactDescriptorReader.java:171)
at org.eclipse.aether.internal.impl.collect.DefaultDependencyCollector.resolveCachedArtifactDescriptor(DefaultDependencyCollector.java:538)
at org.eclipse.aether.internal.impl.collect.DefaultDependencyCollector.getArtifactDescriptorResult(DefaultDependencyCollector.java:523)
at org.eclipse.aether.internal.impl.collect.DefaultDependencyCollector.processDependency(DefaultDependencyCollector.java:410)
at org.eclipse.aether.internal.impl.collect.DefaultDependencyCollector.processDependency(DefaultDependencyCollector.java:362)
at org.eclipse.aether.internal.impl.collect.DefaultDependencyCollector.process(DefaultDependencyCollector.java:349)
at org.eclipse.aether.internal.impl.collect.DefaultDependencyCollector.collectDependencies(DefaultDependencyCollector.java:254)
at org.eclipse.aether.internal.impl.DefaultRepositorySystem.resolveDependencies(DefaultRepositorySystem.java:309)
at org.apache.maven.plugin.compiler.AbstractCompilerMojo.resolveProcessorPathEntries(AbstractCompilerMojo.java:1595)
```
I may have missed something, so any suggestions are more than welcome.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] psiroky commented on a diff in pull request #180: [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps
Posted by "psiroky (via GitHub)" <gi...@apache.org>.
psiroky commented on code in PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#discussion_r1121568151
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -330,6 +335,22 @@ public abstract class AbstractCompilerMojo extends AbstractMojo {
@Parameter
private List<DependencyCoordinate> annotationProcessorPaths;
+ /**
+ * <p>
+ * Whether to use the Maven dependency management section when resolving transitive dependencies of annotation
+ * processor paths.
+ * </p>
+ * <p>
+ * This flag does not enable / disable the ability to get the version of annotation processor paths
+ * (the top-level paths) from dependency management section. It only influences the resolution of
+ * transitive dependencies of those top-level paths.
+ * </p>
+ *
+ * @since 3.12.0
+ */
+ @Parameter(defaultValue = "false")
Review Comment:
It may definitely change the resulting `processorpath`, thus changing the behavior. So it should be opt-in.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] slawekjaranowski commented on a diff in pull request #180: [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on code in PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#discussion_r1172841557
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1584,42 +1605,91 @@ private List<String> resolveProcessorPathEntries() throws MojoExecutionException
return null;
}
- Set<String> elements = new LinkedHashSet<>();
try {
- List<Dependency> dependencies = convertToDependencies(annotationProcessorPaths);
+ List<org.eclipse.aether.graph.Dependency> dependencies = convertToDependencies(annotationProcessorPaths);
+ List<org.eclipse.aether.graph.Dependency> managedDependencies =
+ getManagedDependenciesForAnnotationProcessorPaths();
CollectRequest collectRequest =
- new CollectRequest(dependencies, Collections.emptyList(), project.getRemoteProjectRepositories());
+ new CollectRequest(dependencies, managedDependencies, project.getRemoteProjectRepositories());
DependencyRequest dependencyRequest = new DependencyRequest();
dependencyRequest.setCollectRequest(collectRequest);
DependencyResult dependencyResult =
repositorySystem.resolveDependencies(session.getRepositorySession(), dependencyRequest);
- for (ArtifactResult resolved : dependencyResult.getArtifactResults()) {
- elements.add(resolved.getArtifact().getFile().getAbsolutePath());
- }
- return new ArrayList<>(elements);
+ return dependencyResult.getArtifactResults().stream()
+ .map(resolved -> resolved.getArtifact().getFile().getAbsolutePath())
+ .collect(Collectors.toList());
} catch (Exception e) {
throw new MojoExecutionException(
"Resolution of annotationProcessorPath dependencies failed: " + e.getLocalizedMessage(), e);
}
}
- private List<Dependency> convertToDependencies(List<DependencyCoordinate> annotationProcessorPaths) {
- List<Dependency> dependencies = new ArrayList<>();
+ private List<org.eclipse.aether.graph.Dependency> convertToDependencies(
+ List<DependencyCoordinate> annotationProcessorPaths) throws MojoExecutionException {
+ List<org.eclipse.aether.graph.Dependency> dependencies = new ArrayList<>();
for (DependencyCoordinate annotationProcessorPath : annotationProcessorPaths) {
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler(annotationProcessorPath.getType());
+ String version = getAnnotationProcessorPathVersion(annotationProcessorPath);
Artifact artifact = new DefaultArtifact(
annotationProcessorPath.getGroupId(),
annotationProcessorPath.getArtifactId(),
annotationProcessorPath.getClassifier(),
handler.getExtension(),
- annotationProcessorPath.getVersion());
+ version);
Set<Exclusion> exclusions = convertToAetherExclusions(annotationProcessorPath.getExclusions());
- dependencies.add(new Dependency(artifact, JavaScopes.RUNTIME, false, exclusions));
+ dependencies.add(new org.eclipse.aether.graph.Dependency(artifact, JavaScopes.RUNTIME, false, exclusions));
}
return dependencies;
}
+ private String getAnnotationProcessorPathVersion(DependencyCoordinate annotationProcessorPath)
+ throws MojoExecutionException {
+ String configuredVersion = annotationProcessorPath.getVersion();
+ if (configuredVersion != null) {
+ return configuredVersion;
+ } else {
+ List<Dependency> managedDependencies = getProjectManagedDependencies();
+ return findManagedVersion(annotationProcessorPath, managedDependencies)
+ .orElseThrow(() -> new MojoExecutionException(String.format(
+ "Cannot find version for annotation processor path '%s'. The version needs to be either"
+ + " provided directly in the plugin configuration or via dependency management.",
+ annotationProcessorPath)));
+ }
+ }
+
+ private Optional<String> findManagedVersion(
+ DependencyCoordinate dependencyCoordinate, List<Dependency> managedDependencies) {
+ return managedDependencies.stream()
+ .filter(dep -> Objects.equals(dep.getGroupId(), dependencyCoordinate.getGroupId())
+ && Objects.equals(dep.getArtifactId(), dependencyCoordinate.getArtifactId())
+ && Objects.equals(dep.getClassifier(), dependencyCoordinate.getClassifier())
+ && Objects.equals(dep.getType(), dependencyCoordinate.getType()))
+ .findAny()
+ .map(org.apache.maven.model.Dependency::getVersion);
+ }
Review Comment:
ok, we can go as is - if we need we can tray with `parallelStream` next time 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps [maven-compiler-plugin]
Posted by "famod (via GitHub)" <gi...@apache.org>.
famod commented on PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#issuecomment-1876946809
@psiroky sure:
> Are you relying on the new dependency management
Yes, that's what I tried and this doesn't work (old groupId):
```xml
<configuration>
<annotationProcessorPaths>
<path>
<groupId>org.hibernate</groupId>
<artifactId>hibernate-jpamodelgen</artifactId>
</path>
```
but this does work (new, relocated groupId):
```
```
<configuration>
<annotationProcessorPaths>
<path>
<groupId>org.hibernate.orm</groupId>
<artifactId>hibernate-jpamodelgen</artifactId>
</path>
```
The Quarkus BOM I'm importing has this entry (new groupId only!):
https://github.com/quarkusio/quarkus/blob/3.6.4/bom/application/pom.xml#L5012-L5014
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] psiroky commented on pull request #180: [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps
Posted by "psiroky (via GitHub)" <gi...@apache.org>.
psiroky commented on PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#issuecomment-1449955799
I decided to proceed with the suggested solution. The PR now contains changes for both use cases:
1. using dependency management to get just the top-level annotation processor path version
2. using dependency management when resolving the transitive dependencies via maven-resolver
There is also a new flag that enables / disables the usage of dependency management for the second use case.
I added some inline comments for things that I am not 100% sure about or things that potentially need more eyes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] psiroky commented on a diff in pull request #180: [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps
Posted by "psiroky (via GitHub)" <gi...@apache.org>.
psiroky commented on code in PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#discussion_r1142993319
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1584,42 +1605,91 @@ private List<String> resolveProcessorPathEntries() throws MojoExecutionException
return null;
}
- Set<String> elements = new LinkedHashSet<>();
try {
- List<Dependency> dependencies = convertToDependencies(annotationProcessorPaths);
+ List<org.eclipse.aether.graph.Dependency> dependencies = convertToDependencies(annotationProcessorPaths);
+ List<org.eclipse.aether.graph.Dependency> managedDependencies =
+ getManagedDependenciesForAnnotationProcessorPaths();
CollectRequest collectRequest =
- new CollectRequest(dependencies, Collections.emptyList(), project.getRemoteProjectRepositories());
+ new CollectRequest(dependencies, managedDependencies, project.getRemoteProjectRepositories());
DependencyRequest dependencyRequest = new DependencyRequest();
dependencyRequest.setCollectRequest(collectRequest);
DependencyResult dependencyResult =
repositorySystem.resolveDependencies(session.getRepositorySession(), dependencyRequest);
- for (ArtifactResult resolved : dependencyResult.getArtifactResults()) {
- elements.add(resolved.getArtifact().getFile().getAbsolutePath());
- }
- return new ArrayList<>(elements);
+ return dependencyResult.getArtifactResults().stream()
+ .map(resolved -> resolved.getArtifact().getFile().getAbsolutePath())
+ .collect(Collectors.toList());
} catch (Exception e) {
throw new MojoExecutionException(
"Resolution of annotationProcessorPath dependencies failed: " + e.getLocalizedMessage(), e);
}
}
- private List<Dependency> convertToDependencies(List<DependencyCoordinate> annotationProcessorPaths) {
- List<Dependency> dependencies = new ArrayList<>();
+ private List<org.eclipse.aether.graph.Dependency> convertToDependencies(
+ List<DependencyCoordinate> annotationProcessorPaths) throws MojoExecutionException {
+ List<org.eclipse.aether.graph.Dependency> dependencies = new ArrayList<>();
for (DependencyCoordinate annotationProcessorPath : annotationProcessorPaths) {
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler(annotationProcessorPath.getType());
+ String version = getAnnotationProcessorPathVersion(annotationProcessorPath);
Artifact artifact = new DefaultArtifact(
annotationProcessorPath.getGroupId(),
annotationProcessorPath.getArtifactId(),
annotationProcessorPath.getClassifier(),
handler.getExtension(),
- annotationProcessorPath.getVersion());
+ version);
Set<Exclusion> exclusions = convertToAetherExclusions(annotationProcessorPath.getExclusions());
- dependencies.add(new Dependency(artifact, JavaScopes.RUNTIME, false, exclusions));
+ dependencies.add(new org.eclipse.aether.graph.Dependency(artifact, JavaScopes.RUNTIME, false, exclusions));
}
return dependencies;
}
+ private String getAnnotationProcessorPathVersion(DependencyCoordinate annotationProcessorPath)
+ throws MojoExecutionException {
+ String configuredVersion = annotationProcessorPath.getVersion();
+ if (configuredVersion != null) {
+ return configuredVersion;
+ } else {
+ List<Dependency> managedDependencies = getProjectManagedDependencies();
+ return findManagedVersion(annotationProcessorPath, managedDependencies)
+ .orElseThrow(() -> new MojoExecutionException(String.format(
+ "Cannot find version for annotation processor path '%s'. The version needs to be either"
+ + " provided directly in the plugin configuration or via dependency management.",
+ annotationProcessorPath)));
+ }
+ }
+
+ private Optional<String> findManagedVersion(
+ DependencyCoordinate dependencyCoordinate, List<Dependency> managedDependencies) {
+ return managedDependencies.stream()
+ .filter(dep -> Objects.equals(dep.getGroupId(), dependencyCoordinate.getGroupId())
+ && Objects.equals(dep.getArtifactId(), dependencyCoordinate.getArtifactId())
+ && Objects.equals(dep.getClassifier(), dependencyCoordinate.getClassifier())
+ && Objects.equals(dep.getType(), dependencyCoordinate.getType()))
+ .findAny()
+ .map(org.apache.maven.model.Dependency::getVersion);
+ }
Review Comment:
I can add a note to the docs.
Not sure about `parallelStream` (especially in combination with -T), but I could try and test to see if there is any improvement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps [maven-compiler-plugin]
Posted by "psiroky (via GitHub)" <gi...@apache.org>.
psiroky commented on PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#issuecomment-1877354179
Ah, I see. In that case you are right, the relocation message should be there. I'll see if I can find some time to take a look at this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] psiroky commented on a diff in pull request #180: [MCOMPILER-391] Get annotation processor version from dependencyManagement
Posted by "psiroky (via GitHub)" <gi...@apache.org>.
psiroky commented on code in PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#discussion_r1107776766
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1604,22 +1606,39 @@ private List<String> resolveProcessorPathEntries() throws MojoExecutionException
}
}
- private List<Dependency> convertToDependencies(List<DependencyCoordinate> annotationProcessorPaths) {
+ private List<Dependency> convertToDependencies(List<DependencyCoordinate> annotationProcessorPaths)
+ throws MojoExecutionException {
List<Dependency> dependencies = new ArrayList<>();
for (DependencyCoordinate annotationProcessorPath : annotationProcessorPaths) {
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler(annotationProcessorPath.getType());
+ String version = annotationProcessorPath.getVersion();
+ if (version == null) {
+ version = findManagedVersion(annotationProcessorPath)
+ .orElseThrow(
+ () -> new MojoExecutionException("Cannot find version for " + annotationProcessorPath));
+ }
Review Comment:
Sorry, I am not sure I quite got that.
I was thinking that resolver could check if the version is null and if so, then try to find the version from the supplied managed deps (somewhere inside `org.eclipse.aether.internal.impl.DefaultRepositorySystem.resolveDependencies`). But maybe that's not really what resolver should be doing anyway. And I assume that even if this was added, the plugin could not rely on this (yet), because the older Maven versions (with older resolvers) would not support that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] psiroky commented on a diff in pull request #180: [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps
Posted by "psiroky (via GitHub)" <gi...@apache.org>.
psiroky commented on code in PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#discussion_r1121566199
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1584,42 +1605,91 @@ private List<String> resolveProcessorPathEntries() throws MojoExecutionException
return null;
}
- Set<String> elements = new LinkedHashSet<>();
try {
- List<Dependency> dependencies = convertToDependencies(annotationProcessorPaths);
+ List<org.eclipse.aether.graph.Dependency> dependencies = convertToDependencies(annotationProcessorPaths);
+ List<org.eclipse.aether.graph.Dependency> managedDependencies =
+ getManagedDependenciesForAnnotationProcessorPaths();
CollectRequest collectRequest =
- new CollectRequest(dependencies, Collections.emptyList(), project.getRemoteProjectRepositories());
+ new CollectRequest(dependencies, managedDependencies, project.getRemoteProjectRepositories());
DependencyRequest dependencyRequest = new DependencyRequest();
dependencyRequest.setCollectRequest(collectRequest);
DependencyResult dependencyResult =
repositorySystem.resolveDependencies(session.getRepositorySession(), dependencyRequest);
- for (ArtifactResult resolved : dependencyResult.getArtifactResults()) {
- elements.add(resolved.getArtifact().getFile().getAbsolutePath());
- }
- return new ArrayList<>(elements);
+ return dependencyResult.getArtifactResults().stream()
+ .map(resolved -> resolved.getArtifact().getFile().getAbsolutePath())
+ .collect(Collectors.toList());
} catch (Exception e) {
throw new MojoExecutionException(
"Resolution of annotationProcessorPath dependencies failed: " + e.getLocalizedMessage(), e);
}
}
- private List<Dependency> convertToDependencies(List<DependencyCoordinate> annotationProcessorPaths) {
- List<Dependency> dependencies = new ArrayList<>();
+ private List<org.eclipse.aether.graph.Dependency> convertToDependencies(
+ List<DependencyCoordinate> annotationProcessorPaths) throws MojoExecutionException {
+ List<org.eclipse.aether.graph.Dependency> dependencies = new ArrayList<>();
for (DependencyCoordinate annotationProcessorPath : annotationProcessorPaths) {
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler(annotationProcessorPath.getType());
+ String version = getAnnotationProcessorPathVersion(annotationProcessorPath);
Artifact artifact = new DefaultArtifact(
annotationProcessorPath.getGroupId(),
annotationProcessorPath.getArtifactId(),
annotationProcessorPath.getClassifier(),
handler.getExtension(),
- annotationProcessorPath.getVersion());
+ version);
Set<Exclusion> exclusions = convertToAetherExclusions(annotationProcessorPath.getExclusions());
- dependencies.add(new Dependency(artifact, JavaScopes.RUNTIME, false, exclusions));
+ dependencies.add(new org.eclipse.aether.graph.Dependency(artifact, JavaScopes.RUNTIME, false, exclusions));
}
return dependencies;
}
+ private String getAnnotationProcessorPathVersion(DependencyCoordinate annotationProcessorPath)
+ throws MojoExecutionException {
+ String configuredVersion = annotationProcessorPath.getVersion();
+ if (configuredVersion != null) {
+ return configuredVersion;
+ } else {
+ List<Dependency> managedDependencies = getProjectManagedDependencies();
+ return findManagedVersion(annotationProcessorPath, managedDependencies)
+ .orElseThrow(() -> new MojoExecutionException(String.format(
+ "Cannot find version for annotation processor path '%s'. The version needs to be either"
+ + " provided directly in the plugin configuration or via dependency management.",
+ annotationProcessorPath)));
+ }
+ }
+
+ private Optional<String> findManagedVersion(
+ DependencyCoordinate dependencyCoordinate, List<Dependency> managedDependencies) {
+ return managedDependencies.stream()
+ .filter(dep -> Objects.equals(dep.getGroupId(), dependencyCoordinate.getGroupId())
+ && Objects.equals(dep.getArtifactId(), dependencyCoordinate.getArtifactId())
+ && Objects.equals(dep.getClassifier(), dependencyCoordinate.getClassifier())
+ && Objects.equals(dep.getType(), dependencyCoordinate.getType()))
+ .findAny()
+ .map(org.apache.maven.model.Dependency::getVersion);
+ }
+
+ private List<org.eclipse.aether.graph.Dependency> getManagedDependenciesForAnnotationProcessorPaths() {
+ if (!annotationProcessorPathsUseDepMgmt) {
+ return Collections.emptyList();
+ }
+ List<Dependency> projectManagedDependencies = getProjectManagedDependencies();
+ ArtifactTypeRegistry artifactTypeRegistry =
+ session.getRepositorySession().getArtifactTypeRegistry();
+
+ return projectManagedDependencies.stream()
+ .map(dep -> RepositoryUtils.toDependency(dep, artifactTypeRegistry))
Review Comment:
This is also not great, since it re-creates the dependency list for every sub-module of the Maven build, and usually the dependency management section is 99% same for all modules.
Taking example of Quarkus. It has ~500 modules using annotation processing and the dependency management section has ~2k dependencies. That 1 million of new objects created during the build. Possible not the biggest deal (since the build already creates tens of millions of other objects). Again, at this point I am not seeing a simple alternative. Suggestions more than welcome.
Maybe some caching could be done here. But caching is usually double-edge sword, so I wanted to avoid that for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] slawekjaranowski merged pull request #180: [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski merged PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps [maven-compiler-plugin]
Posted by "famod (via GitHub)" <gi...@apache.org>.
famod commented on PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#issuecomment-1877280262
> I think the missing relocation warning has the same cause - the plugin (or more precisely the dependency resolution mechanism the plugin uses) simply does not know that the artifact was relocated, because without version, it cannot fetch the relocation config.
>
> I believe the relocation warning gets printed in case you depend on the old artifact, but the version must be known, explicitly or via depMgmt: e.g:
>
> ```
> <dependency>
> <groupId>org.hibernate</groupId>
> <artifactId>hibernate-jpamodelgen</artifactId>
> <verison>6.0.0</version>
> ```
>
> In that case, the POM is fetched, relocation config found and the warning can be printed.
I guess I should have been more precise:
I had exactly that (only a version property instead of the version literal, but that shouldn't make any difference) and still no warning. Maven 3.9.6, btw.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] graemerocher commented on pull request #180: [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps
Posted by "graemerocher (via GitHub)" <gi...@apache.org>.
graemerocher commented on PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#issuecomment-1516094076
could we kindly ask for an increase in priority on this issue. The fact that Maven is unable to correctly manage annotation processor dependencies like Gradle can is a major disadvantage. It is issues like this that imagine lead to any tool chain that relies on annotation processors moving away from Maven (see Android)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] psiroky commented on pull request #180: [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps
Posted by "psiroky (via GitHub)" <gi...@apache.org>.
psiroky commented on PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#issuecomment-1516621512
I can update the docs, based on the comments above. Besides that, the PR should be ready from my point of view. Please let me know if there is anything else that would need changing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps [maven-compiler-plugin]
Posted by "famod (via GitHub)" <gi...@apache.org>.
famod commented on PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#issuecomment-1877127524
@psiroky
Yeah, I think you have a very valid point: There is no explicit version for that old groupId and so there is no precise way to find the exact pom with the relocation info (=> new groupId).
As a human you could just check the latest version for the old groupId and get the relocation info from that but I doubt there is a nice way of finding the latest version via Maven builtin features (?).
Not worth the hassle.
For extra user-friendliness this plugin could check (if no version is found) for _similar_ deps, e.g. same artifactId but differen groupId and print put all those candidates, but relocations are not limited to groupIds (AFAIK), so...
It probably boils down to the question why there are/were no relocations warnings like you get for "normal" dependencies. For general plugin dependencies that's clearly something that Maven should cover.
`annotationProcessorPaths` is probably a different story.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps [maven-compiler-plugin]
Posted by "psiroky (via GitHub)" <gi...@apache.org>.
psiroky commented on PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#issuecomment-1876984635
@famod thanks. In this case, I am not quite sure this is supported from Maven itself.
I assume there is no version anywhere in the project depMgmt for `org.hibernate:hibernate-jpamodelgen` and so there is also no information which could map it to the new artifact `org.hibernate.orm:hibernate-jpamodelgen`. That mapping information is part of the old / original POM, but since the plugin does not have the version for this old artifact, it cannot lookup the POM with the relocation config.
Does that make sense? Or am I overlooking something in the setup?
Or this perhaps something that is working for you outside of the compiler plugin configuration with standard project dependencies?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] psiroky commented on a diff in pull request #180: [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps
Posted by "psiroky (via GitHub)" <gi...@apache.org>.
psiroky commented on code in PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#discussion_r1121557872
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -330,6 +335,22 @@ public abstract class AbstractCompilerMojo extends AbstractMojo {
@Parameter
private List<DependencyCoordinate> annotationProcessorPaths;
+ /**
+ * <p>
+ * Whether to use the Maven dependency management section when resolving transitive dependencies of annotation
+ * processor paths.
+ * </p>
+ * <p>
+ * This flag does not enable / disable the ability to get the version of annotation processor paths
+ * (the top-level paths) from dependency management section. It only influences the resolution of
+ * transitive dependencies of those top-level paths.
+ * </p>
+ *
+ * @since 3.12.0
+ */
+ @Parameter(defaultValue = "false")
+ private boolean annotationProcessorPathsUseDepMgmt;
Review Comment:
I am not the biggest fan of this long name, but I could not find a better one at this point. Reason I choose this one is that the prefix `annotationProcessorPaths` means it will be rendered/shown together with the paths option, meaning users are much more likely to see this when browsing the docs (and making the connection that this should be used together with annotation processor paths). We could use something shorter like `useDepMgmt` or so, but then you need to know exactly what you are looking for, since this would be rendered very far away from the `annotationProcessorPaths`. I may also be too generic of a name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] psiroky commented on a diff in pull request #180: [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps
Posted by "psiroky (via GitHub)" <gi...@apache.org>.
psiroky commented on code in PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#discussion_r1121560964
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1584,42 +1605,91 @@ private List<String> resolveProcessorPathEntries() throws MojoExecutionException
return null;
}
- Set<String> elements = new LinkedHashSet<>();
try {
- List<Dependency> dependencies = convertToDependencies(annotationProcessorPaths);
+ List<org.eclipse.aether.graph.Dependency> dependencies = convertToDependencies(annotationProcessorPaths);
+ List<org.eclipse.aether.graph.Dependency> managedDependencies =
+ getManagedDependenciesForAnnotationProcessorPaths();
CollectRequest collectRequest =
- new CollectRequest(dependencies, Collections.emptyList(), project.getRemoteProjectRepositories());
+ new CollectRequest(dependencies, managedDependencies, project.getRemoteProjectRepositories());
DependencyRequest dependencyRequest = new DependencyRequest();
dependencyRequest.setCollectRequest(collectRequest);
DependencyResult dependencyResult =
repositorySystem.resolveDependencies(session.getRepositorySession(), dependencyRequest);
- for (ArtifactResult resolved : dependencyResult.getArtifactResults()) {
- elements.add(resolved.getArtifact().getFile().getAbsolutePath());
- }
- return new ArrayList<>(elements);
+ return dependencyResult.getArtifactResults().stream()
+ .map(resolved -> resolved.getArtifact().getFile().getAbsolutePath())
+ .collect(Collectors.toList());
} catch (Exception e) {
throw new MojoExecutionException(
"Resolution of annotationProcessorPath dependencies failed: " + e.getLocalizedMessage(), e);
}
}
- private List<Dependency> convertToDependencies(List<DependencyCoordinate> annotationProcessorPaths) {
- List<Dependency> dependencies = new ArrayList<>();
+ private List<org.eclipse.aether.graph.Dependency> convertToDependencies(
+ List<DependencyCoordinate> annotationProcessorPaths) throws MojoExecutionException {
+ List<org.eclipse.aether.graph.Dependency> dependencies = new ArrayList<>();
for (DependencyCoordinate annotationProcessorPath : annotationProcessorPaths) {
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler(annotationProcessorPath.getType());
+ String version = getAnnotationProcessorPathVersion(annotationProcessorPath);
Artifact artifact = new DefaultArtifact(
annotationProcessorPath.getGroupId(),
annotationProcessorPath.getArtifactId(),
annotationProcessorPath.getClassifier(),
handler.getExtension(),
- annotationProcessorPath.getVersion());
+ version);
Set<Exclusion> exclusions = convertToAetherExclusions(annotationProcessorPath.getExclusions());
- dependencies.add(new Dependency(artifact, JavaScopes.RUNTIME, false, exclusions));
+ dependencies.add(new org.eclipse.aether.graph.Dependency(artifact, JavaScopes.RUNTIME, false, exclusions));
}
return dependencies;
}
+ private String getAnnotationProcessorPathVersion(DependencyCoordinate annotationProcessorPath)
+ throws MojoExecutionException {
+ String configuredVersion = annotationProcessorPath.getVersion();
+ if (configuredVersion != null) {
+ return configuredVersion;
+ } else {
+ List<Dependency> managedDependencies = getProjectManagedDependencies();
+ return findManagedVersion(annotationProcessorPath, managedDependencies)
+ .orElseThrow(() -> new MojoExecutionException(String.format(
+ "Cannot find version for annotation processor path '%s'. The version needs to be either"
+ + " provided directly in the plugin configuration or via dependency management.",
+ annotationProcessorPath)));
+ }
+ }
+
+ private Optional<String> findManagedVersion(
+ DependencyCoordinate dependencyCoordinate, List<Dependency> managedDependencies) {
+ return managedDependencies.stream()
+ .filter(dep -> Objects.equals(dep.getGroupId(), dependencyCoordinate.getGroupId())
+ && Objects.equals(dep.getArtifactId(), dependencyCoordinate.getArtifactId())
+ && Objects.equals(dep.getClassifier(), dependencyCoordinate.getClassifier())
+ && Objects.equals(dep.getType(), dependencyCoordinate.getType()))
+ .findAny()
+ .map(org.apache.maven.model.Dependency::getVersion);
+ }
Review Comment:
This could be somewhat problematic from performance point of view, in case the dependency management section is big (e.g. thousands of dependencies, which is not that uncommon I think -- thinking about SpringBoot or Quarkus BOMs). On the other hand, I have not found a better way at this point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-compiler-plugin] psiroky commented on a diff in pull request #180: [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps
Posted by "psiroky (via GitHub)" <gi...@apache.org>.
psiroky commented on code in PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#discussion_r1121595369
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1604,22 +1606,39 @@ private List<String> resolveProcessorPathEntries() throws MojoExecutionException
}
}
- private List<Dependency> convertToDependencies(List<DependencyCoordinate> annotationProcessorPaths) {
+ private List<Dependency> convertToDependencies(List<DependencyCoordinate> annotationProcessorPaths)
+ throws MojoExecutionException {
List<Dependency> dependencies = new ArrayList<>();
for (DependencyCoordinate annotationProcessorPath : annotationProcessorPaths) {
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler(annotationProcessorPath.getType());
+ String version = annotationProcessorPath.getVersion();
+ if (version == null) {
+ version = findManagedVersion(annotationProcessorPath)
+ .orElseThrow(
+ () -> new MojoExecutionException("Cannot find version for " + annotationProcessorPath));
+ }
Review Comment:
Thinking about this again, this would probably mean we could not distinguish between the 2 difference cases: version for the top-level path, and managed versions for the transitive dependencies. Marking this as resolved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps [maven-compiler-plugin]
Posted by "psiroky (via GitHub)" <gi...@apache.org>.
psiroky commented on PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#issuecomment-1876936773
@famod could you please provide a bit more details, maybe with an example configuration of the plugin? Are you relying on the new dependency management, or specifying the version manually as before?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps [maven-compiler-plugin]
Posted by "psiroky (via GitHub)" <gi...@apache.org>.
psiroky commented on PR #180:
URL: https://github.com/apache/maven-compiler-plugin/pull/180#issuecomment-1877222225
Yeah, there could possibly be some sort of heuristics to try to detect these relocations. Not sure if this would be something that should be done directly in the plugin or rather left to the dependency resolver.
I think the missing relocation warning has the same cause - the plugin (or more precisely the dependency resolution mechanism the plugin uses) simply does not know that the artifact was relocated, because without version, it cannot fetch the relocation config.
I believe the relocation warning gets printed in case you depend on the old artifact, but the version must be known, explicitly or via depMgmt: e.g:
```
<dependency>
<groupId>org.hibernate</groupId>
<artifactId>hibernate-jpamodelgen</artifactId>
<verison>6.0.0</version>
```
In that case, the POM is fetched, relocation config found and the warning can be printed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org