You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2023/01/19 11:31:34 UTC
[GitHub] [maven-compiler-plugin] psiroky opened a new pull request, #169: [MCOMPILER-522] Use maven-resolver to resolve 'annotationProcessorPaths' dependencies
psiroky opened a new pull request, #169:
URL: https://github.com/apache/maven-compiler-plugin/pull/169
* heavily based on work by @cstamas (PR #151)
* added IT to verify the build fails if one the dependencies cannot be resolved (e.g. does not exist)
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.
- [x] 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] psiroky commented on a diff in pull request #169: [MCOMPILER-522] Use maven-resolver to resolve 'annotationProcessorPaths' dependencies
Posted by GitBox <gi...@apache.org>.
psiroky commented on code in PR #169:
URL: https://github.com/apache/maven-compiler-plugin/pull/169#discussion_r1081657329
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1814,29 +1810,26 @@ private List<String> resolveProcessorPathEntries()
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler( coord.getType() );
Artifact artifact = new DefaultArtifact(
- coord.getGroupId(),
- coord.getArtifactId(),
- VersionRange.createFromVersionSpec( coord.getVersion() ),
- Artifact.SCOPE_RUNTIME,
- coord.getType(),
- coord.getClassifier(),
- handler,
- false );
-
- ArtifactResolutionRequest request = new ArtifactResolutionRequest()
- .setArtifact( artifact )
- .setResolveRoot( true )
- .setResolveTransitively( true )
- .setLocalRepository( session.getLocalRepository() )
- .setRemoteRepositories( project.getRemoteArtifactRepositories() );
-
- ArtifactResolutionResult resolutionResult = repositorySystem.resolve( request );
-
- resolutionErrorHandler.throwErrors( request, resolutionResult );
-
- for ( Artifact resolved : resolutionResult.getArtifacts() )
+ coord.getGroupId(),
+ coord.getArtifactId(),
+ coord.getClassifier(),
+ handler.getExtension(),
+ coord.getVersion()
+ );
+
+ CollectRequest collectRequest = new CollectRequest( new Dependency( artifact, JavaScopes.RUNTIME ),
+ project.getRemoteProjectRepositories() );
+ DependencyRequest dependencyRequest = new DependencyRequest();
+ dependencyRequest.setCollectRequest( collectRequest );
+ DependencyResult dependencyResult = repositorySystem.resolveDependencies(
+ session.getRepositorySession(), dependencyRequest );
+
+ for ( ArtifactResult resolved : dependencyResult.getArtifactResults() )
{
- elements.add( resolved.getFile().getAbsolutePath() );
+ if ( resolved.getArtifact() != null && resolved.getArtifact().getFile() != null )
+ {
+ elements.add( resolved.getArtifact().getFile().getAbsolutePath() );
+ }
Review Comment:
I removed the `null` check and marking this as resolved based on the above discussion.
--
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 #169: [MCOMPILER-522] Use maven-resolver to resolve 'annotationProcessorPaths' dependencies
Posted by GitBox <gi...@apache.org>.
psiroky commented on code in PR #169:
URL: https://github.com/apache/maven-compiler-plugin/pull/169#discussion_r1081648930
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1814,29 +1810,26 @@ private List<String> resolveProcessorPathEntries()
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler( coord.getType() );
Artifact artifact = new DefaultArtifact(
- coord.getGroupId(),
- coord.getArtifactId(),
- VersionRange.createFromVersionSpec( coord.getVersion() ),
- Artifact.SCOPE_RUNTIME,
- coord.getType(),
- coord.getClassifier(),
- handler,
- false );
-
- ArtifactResolutionRequest request = new ArtifactResolutionRequest()
- .setArtifact( artifact )
- .setResolveRoot( true )
- .setResolveTransitively( true )
- .setLocalRepository( session.getLocalRepository() )
- .setRemoteRepositories( project.getRemoteArtifactRepositories() );
-
- ArtifactResolutionResult resolutionResult = repositorySystem.resolve( request );
-
- resolutionErrorHandler.throwErrors( request, resolutionResult );
-
- for ( Artifact resolved : resolutionResult.getArtifacts() )
+ coord.getGroupId(),
+ coord.getArtifactId(),
+ coord.getClassifier(),
+ handler.getExtension(),
+ coord.getVersion()
+ );
+
+ CollectRequest collectRequest = new CollectRequest( new Dependency( artifact, JavaScopes.RUNTIME ),
+ project.getRemoteProjectRepositories() );
+ DependencyRequest dependencyRequest = new DependencyRequest();
+ dependencyRequest.setCollectRequest( collectRequest );
+ DependencyResult dependencyResult = repositorySystem.resolveDependencies(
+ session.getRepositorySession(), dependencyRequest );
+
+ for ( ArtifactResult resolved : dependencyResult.getArtifactResults() )
{
- elements.add( resolved.getFile().getAbsolutePath() );
+ if ( resolved.getArtifact() != null && resolved.getArtifact().getFile() != null )
+ {
+ elements.add( resolved.getArtifact().getFile().getAbsolutePath() );
+ }
Review Comment:
No, I am really just talking "in theory". I did not reproduce this. And the code is likely fine as it is.
As someone reading the code for the first time though, if I see a `null` check I expect the "thing" can actually be `null` is some cases, otherwise the `null` check would not there. So I would either remove the `null` check completely (and have an NPE thrown if this happens) or throw another explicit exception. I haven't really had a preference between the two, hence started the discussion.
--
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] ajarmoniuk commented on a diff in pull request #169: [MCOMPILER-522] Use maven-resolver to resolve 'annotationProcessorPaths' dependencies
Posted by GitBox <gi...@apache.org>.
ajarmoniuk commented on code in PR #169:
URL: https://github.com/apache/maven-compiler-plugin/pull/169#discussion_r1081577830
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1814,29 +1810,26 @@ private List<String> resolveProcessorPathEntries()
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler( coord.getType() );
Artifact artifact = new DefaultArtifact(
- coord.getGroupId(),
- coord.getArtifactId(),
- VersionRange.createFromVersionSpec( coord.getVersion() ),
- Artifact.SCOPE_RUNTIME,
- coord.getType(),
- coord.getClassifier(),
- handler,
- false );
-
- ArtifactResolutionRequest request = new ArtifactResolutionRequest()
- .setArtifact( artifact )
- .setResolveRoot( true )
- .setResolveTransitively( true )
- .setLocalRepository( session.getLocalRepository() )
- .setRemoteRepositories( project.getRemoteArtifactRepositories() );
-
- ArtifactResolutionResult resolutionResult = repositorySystem.resolve( request );
-
- resolutionErrorHandler.throwErrors( request, resolutionResult );
-
- for ( Artifact resolved : resolutionResult.getArtifacts() )
+ coord.getGroupId(),
+ coord.getArtifactId(),
+ coord.getClassifier(),
+ handler.getExtension(),
+ coord.getVersion()
+ );
+
+ CollectRequest collectRequest = new CollectRequest( new Dependency( artifact, JavaScopes.RUNTIME ),
+ project.getRemoteProjectRepositories() );
+ DependencyRequest dependencyRequest = new DependencyRequest();
+ dependencyRequest.setCollectRequest( collectRequest );
+ DependencyResult dependencyResult = repositorySystem.resolveDependencies(
+ session.getRepositorySession(), dependencyRequest );
+
+ for ( ArtifactResult resolved : dependencyResult.getArtifactResults() )
{
- elements.add( resolved.getFile().getAbsolutePath() );
+ if ( resolved.getArtifact() != null && resolved.getArtifact().getFile() != null )
+ {
+ elements.add( resolved.getArtifact().getFile().getAbsolutePath() );
+ }
Review Comment:
Correct. In addition, result.getExceptions() will contain the list of all gathered exceptions (if there are more than one).
--
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] ajarmoniuk commented on a diff in pull request #169: [MCOMPILER-522] Use maven-resolver to resolve 'annotationProcessorPaths' dependencies
Posted by GitBox <gi...@apache.org>.
ajarmoniuk commented on code in PR #169:
URL: https://github.com/apache/maven-compiler-plugin/pull/169#discussion_r1081635311
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1814,29 +1810,26 @@ private List<String> resolveProcessorPathEntries()
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler( coord.getType() );
Artifact artifact = new DefaultArtifact(
- coord.getGroupId(),
- coord.getArtifactId(),
- VersionRange.createFromVersionSpec( coord.getVersion() ),
- Artifact.SCOPE_RUNTIME,
- coord.getType(),
- coord.getClassifier(),
- handler,
- false );
-
- ArtifactResolutionRequest request = new ArtifactResolutionRequest()
- .setArtifact( artifact )
- .setResolveRoot( true )
- .setResolveTransitively( true )
- .setLocalRepository( session.getLocalRepository() )
- .setRemoteRepositories( project.getRemoteArtifactRepositories() );
-
- ArtifactResolutionResult resolutionResult = repositorySystem.resolve( request );
-
- resolutionErrorHandler.throwErrors( request, resolutionResult );
-
- for ( Artifact resolved : resolutionResult.getArtifacts() )
+ coord.getGroupId(),
+ coord.getArtifactId(),
+ coord.getClassifier(),
+ handler.getExtension(),
+ coord.getVersion()
+ );
+
+ CollectRequest collectRequest = new CollectRequest( new Dependency( artifact, JavaScopes.RUNTIME ),
+ project.getRemoteProjectRepositories() );
+ DependencyRequest dependencyRequest = new DependencyRequest();
+ dependencyRequest.setCollectRequest( collectRequest );
+ DependencyResult dependencyResult = repositorySystem.resolveDependencies(
+ session.getRepositorySession(), dependencyRequest );
+
+ for ( ArtifactResult resolved : dependencyResult.getArtifactResults() )
{
- elements.add( resolved.getFile().getAbsolutePath() );
+ if ( resolved.getArtifact() != null && resolved.getArtifact().getFile() != null )
+ {
+ elements.add( resolved.getArtifact().getFile().getAbsolutePath() );
+ }
Review Comment:
If that's the case then it looks like a bug. Did you actually reproduce that case?
Anyway, if such a case occurs, then this means that resolving some artifacts was not successful and I think it should not be ignored, but a fail-fast exception should be thrown.
--
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 #169: [MCOMPILER-522] Use maven-resolver to resolve 'annotationProcessorPaths' dependencies
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #169:
URL: https://github.com/apache/maven-compiler-plugin/pull/169#discussion_r1081646808
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1814,29 +1810,26 @@ private List<String> resolveProcessorPathEntries()
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler( coord.getType() );
Artifact artifact = new DefaultArtifact(
- coord.getGroupId(),
- coord.getArtifactId(),
- VersionRange.createFromVersionSpec( coord.getVersion() ),
- Artifact.SCOPE_RUNTIME,
- coord.getType(),
- coord.getClassifier(),
- handler,
- false );
-
- ArtifactResolutionRequest request = new ArtifactResolutionRequest()
- .setArtifact( artifact )
- .setResolveRoot( true )
- .setResolveTransitively( true )
- .setLocalRepository( session.getLocalRepository() )
- .setRemoteRepositories( project.getRemoteArtifactRepositories() );
-
- ArtifactResolutionResult resolutionResult = repositorySystem.resolve( request );
-
- resolutionErrorHandler.throwErrors( request, resolutionResult );
-
- for ( Artifact resolved : resolutionResult.getArtifacts() )
+ coord.getGroupId(),
+ coord.getArtifactId(),
+ coord.getClassifier(),
+ handler.getExtension(),
+ coord.getVersion()
+ );
+
+ CollectRequest collectRequest = new CollectRequest( new Dependency( artifact, JavaScopes.RUNTIME ),
+ project.getRemoteProjectRepositories() );
+ DependencyRequest dependencyRequest = new DependencyRequest();
+ dependencyRequest.setCollectRequest( collectRequest );
+ DependencyResult dependencyResult = repositorySystem.resolveDependencies(
+ session.getRepositorySession(), dependencyRequest );
+
+ for ( ArtifactResult resolved : dependencyResult.getArtifactResults() )
{
- elements.add( resolved.getFile().getAbsolutePath() );
+ if ( resolved.getArtifact() != null && resolved.getArtifact().getFile() != null )
+ {
+ elements.add( resolved.getArtifact().getFile().getAbsolutePath() );
+ }
Review Comment:
Remove null check from here, it was my mistake. If nothing thrown, all resolved just fine: https://github.com/apache/maven-resolver/blob/master/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultArtifactResolver.java#L486-L509
--
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 #169: [MCOMPILER-522] Use maven-resolver to resolve 'annotationProcessorPaths' dependencies
Posted by GitBox <gi...@apache.org>.
psiroky commented on code in PR #169:
URL: https://github.com/apache/maven-compiler-plugin/pull/169#discussion_r1081610573
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1814,29 +1810,26 @@ private List<String> resolveProcessorPathEntries()
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler( coord.getType() );
Artifact artifact = new DefaultArtifact(
- coord.getGroupId(),
- coord.getArtifactId(),
- VersionRange.createFromVersionSpec( coord.getVersion() ),
- Artifact.SCOPE_RUNTIME,
- coord.getType(),
- coord.getClassifier(),
- handler,
- false );
-
- ArtifactResolutionRequest request = new ArtifactResolutionRequest()
- .setArtifact( artifact )
- .setResolveRoot( true )
- .setResolveTransitively( true )
- .setLocalRepository( session.getLocalRepository() )
- .setRemoteRepositories( project.getRemoteArtifactRepositories() );
-
- ArtifactResolutionResult resolutionResult = repositorySystem.resolve( request );
-
- resolutionErrorHandler.throwErrors( request, resolutionResult );
-
- for ( Artifact resolved : resolutionResult.getArtifacts() )
+ coord.getGroupId(),
+ coord.getArtifactId(),
+ coord.getClassifier(),
+ handler.getExtension(),
+ coord.getVersion()
+ );
+
+ CollectRequest collectRequest = new CollectRequest( new Dependency( artifact, JavaScopes.RUNTIME ),
+ project.getRemoteProjectRepositories() );
+ DependencyRequest dependencyRequest = new DependencyRequest();
+ dependencyRequest.setCollectRequest( collectRequest );
+ DependencyResult dependencyResult = repositorySystem.resolveDependencies(
+ session.getRepositorySession(), dependencyRequest );
+
+ for ( ArtifactResult resolved : dependencyResult.getArtifactResults() )
{
- elements.add( resolved.getFile().getAbsolutePath() );
+ if ( resolved.getArtifact() != null && resolved.getArtifact().getFile() != null )
+ {
+ elements.add( resolved.getArtifact().getFile().getAbsolutePath() );
+ }
Review Comment:
I meant this case -- there is no `DependencyResolutionException` thrown, but also for some reason `resolved.getArtifact()` or `resolved.getArtifact().getFile()` actually returns null. As I understand it, this should not happen, unless there is a bug in the resolver. But then I am wondering if we should just remove those null checks here? Or maybe throw a fail-fast exception to not silently swallow resolved artifacts where the file is `null`?
--
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] ajarmoniuk commented on a diff in pull request #169: [MCOMPILER-522] Use maven-resolver to resolve 'annotationProcessorPaths' dependencies
Posted by GitBox <gi...@apache.org>.
ajarmoniuk commented on code in PR #169:
URL: https://github.com/apache/maven-compiler-plugin/pull/169#discussion_r1081650624
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1814,29 +1810,26 @@ private List<String> resolveProcessorPathEntries()
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler( coord.getType() );
Artifact artifact = new DefaultArtifact(
- coord.getGroupId(),
- coord.getArtifactId(),
- VersionRange.createFromVersionSpec( coord.getVersion() ),
- Artifact.SCOPE_RUNTIME,
- coord.getType(),
- coord.getClassifier(),
- handler,
- false );
-
- ArtifactResolutionRequest request = new ArtifactResolutionRequest()
- .setArtifact( artifact )
- .setResolveRoot( true )
- .setResolveTransitively( true )
- .setLocalRepository( session.getLocalRepository() )
- .setRemoteRepositories( project.getRemoteArtifactRepositories() );
-
- ArtifactResolutionResult resolutionResult = repositorySystem.resolve( request );
-
- resolutionErrorHandler.throwErrors( request, resolutionResult );
-
- for ( Artifact resolved : resolutionResult.getArtifacts() )
+ coord.getGroupId(),
+ coord.getArtifactId(),
+ coord.getClassifier(),
+ handler.getExtension(),
+ coord.getVersion()
+ );
+
+ CollectRequest collectRequest = new CollectRequest( new Dependency( artifact, JavaScopes.RUNTIME ),
+ project.getRemoteProjectRepositories() );
+ DependencyRequest dependencyRequest = new DependencyRequest();
+ dependencyRequest.setCollectRequest( collectRequest );
+ DependencyResult dependencyResult = repositorySystem.resolveDependencies(
+ session.getRepositorySession(), dependencyRequest );
+
+ for ( ArtifactResult resolved : dependencyResult.getArtifactResults() )
{
- elements.add( resolved.getFile().getAbsolutePath() );
+ if ( resolved.getArtifact() != null && resolved.getArtifact().getFile() != null )
+ {
+ elements.add( resolved.getArtifact().getFile().getAbsolutePath() );
+ }
Review Comment:
Just use Objects.requireNonNull
--
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] ajarmoniuk commented on a diff in pull request #169: [MCOMPILER-522] Use maven-resolver to resolve 'annotationProcessorPaths' dependencies
Posted by GitBox <gi...@apache.org>.
ajarmoniuk commented on code in PR #169:
URL: https://github.com/apache/maven-compiler-plugin/pull/169#discussion_r1081577830
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1814,29 +1810,26 @@ private List<String> resolveProcessorPathEntries()
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler( coord.getType() );
Artifact artifact = new DefaultArtifact(
- coord.getGroupId(),
- coord.getArtifactId(),
- VersionRange.createFromVersionSpec( coord.getVersion() ),
- Artifact.SCOPE_RUNTIME,
- coord.getType(),
- coord.getClassifier(),
- handler,
- false );
-
- ArtifactResolutionRequest request = new ArtifactResolutionRequest()
- .setArtifact( artifact )
- .setResolveRoot( true )
- .setResolveTransitively( true )
- .setLocalRepository( session.getLocalRepository() )
- .setRemoteRepositories( project.getRemoteArtifactRepositories() );
-
- ArtifactResolutionResult resolutionResult = repositorySystem.resolve( request );
-
- resolutionErrorHandler.throwErrors( request, resolutionResult );
-
- for ( Artifact resolved : resolutionResult.getArtifacts() )
+ coord.getGroupId(),
+ coord.getArtifactId(),
+ coord.getClassifier(),
+ handler.getExtension(),
+ coord.getVersion()
+ );
+
+ CollectRequest collectRequest = new CollectRequest( new Dependency( artifact, JavaScopes.RUNTIME ),
+ project.getRemoteProjectRepositories() );
+ DependencyRequest dependencyRequest = new DependencyRequest();
+ dependencyRequest.setCollectRequest( collectRequest );
+ DependencyResult dependencyResult = repositorySystem.resolveDependencies(
+ session.getRepositorySession(), dependencyRequest );
+
+ for ( ArtifactResult resolved : dependencyResult.getArtifactResults() )
{
- elements.add( resolved.getFile().getAbsolutePath() );
+ if ( resolved.getArtifact() != null && resolved.getArtifact().getFile() != null )
+ {
+ elements.add( resolved.getArtifact().getFile().getAbsolutePath() );
+ }
Review Comment:
Your comment in the other PR is correct. So, explicit checking is not necessary. In addition, ArtifactResult::getExceptions (if you catch a DependencyResolutionException) will contain the list of all gathered exceptions (if there are more than one).
--
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 #169: [MCOMPILER-522] Use maven-resolver to resolve 'annotationProcessorPaths' dependencies
Posted by GitBox <gi...@apache.org>.
psiroky commented on code in PR #169:
URL: https://github.com/apache/maven-compiler-plugin/pull/169#discussion_r1081140364
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1814,29 +1810,26 @@ private List<String> resolveProcessorPathEntries()
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler( coord.getType() );
Artifact artifact = new DefaultArtifact(
- coord.getGroupId(),
- coord.getArtifactId(),
- VersionRange.createFromVersionSpec( coord.getVersion() ),
- Artifact.SCOPE_RUNTIME,
- coord.getType(),
- coord.getClassifier(),
- handler,
- false );
-
- ArtifactResolutionRequest request = new ArtifactResolutionRequest()
- .setArtifact( artifact )
- .setResolveRoot( true )
- .setResolveTransitively( true )
- .setLocalRepository( session.getLocalRepository() )
- .setRemoteRepositories( project.getRemoteArtifactRepositories() );
-
- ArtifactResolutionResult resolutionResult = repositorySystem.resolve( request );
-
- resolutionErrorHandler.throwErrors( request, resolutionResult );
-
- for ( Artifact resolved : resolutionResult.getArtifacts() )
+ coord.getGroupId(),
+ coord.getArtifactId(),
+ coord.getClassifier(),
+ handler.getExtension(),
+ coord.getVersion()
+ );
+
+ CollectRequest collectRequest = new CollectRequest( new Dependency( artifact, JavaScopes.RUNTIME ),
+ project.getRemoteProjectRepositories() );
+ DependencyRequest dependencyRequest = new DependencyRequest();
+ dependencyRequest.setCollectRequest( collectRequest );
+ DependencyResult dependencyResult = repositorySystem.resolveDependencies(
+ session.getRepositorySession(), dependencyRequest );
+
+ for ( ArtifactResult resolved : dependencyResult.getArtifactResults() )
{
- elements.add( resolved.getFile().getAbsolutePath() );
+ if ( resolved.getArtifact() != null && resolved.getArtifact().getFile() != null )
+ {
+ elements.add( resolved.getArtifact().getFile().getAbsolutePath() );
+ }
Review Comment:
Not sure about this. I assume the artifact, nor the path should be null if the resolving is successful. However, if we explicitly check, maybe we should throw an exception in the `else` branch? Otherwise we would just not include on of the dependencies without any kind of message to the user.
--
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] ajarmoniuk commented on a diff in pull request #169: [MCOMPILER-522] Use maven-resolver to resolve 'annotationProcessorPaths' dependencies
Posted by GitBox <gi...@apache.org>.
ajarmoniuk commented on code in PR #169:
URL: https://github.com/apache/maven-compiler-plugin/pull/169#discussion_r1081635311
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1814,29 +1810,26 @@ private List<String> resolveProcessorPathEntries()
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler( coord.getType() );
Artifact artifact = new DefaultArtifact(
- coord.getGroupId(),
- coord.getArtifactId(),
- VersionRange.createFromVersionSpec( coord.getVersion() ),
- Artifact.SCOPE_RUNTIME,
- coord.getType(),
- coord.getClassifier(),
- handler,
- false );
-
- ArtifactResolutionRequest request = new ArtifactResolutionRequest()
- .setArtifact( artifact )
- .setResolveRoot( true )
- .setResolveTransitively( true )
- .setLocalRepository( session.getLocalRepository() )
- .setRemoteRepositories( project.getRemoteArtifactRepositories() );
-
- ArtifactResolutionResult resolutionResult = repositorySystem.resolve( request );
-
- resolutionErrorHandler.throwErrors( request, resolutionResult );
-
- for ( Artifact resolved : resolutionResult.getArtifacts() )
+ coord.getGroupId(),
+ coord.getArtifactId(),
+ coord.getClassifier(),
+ handler.getExtension(),
+ coord.getVersion()
+ );
+
+ CollectRequest collectRequest = new CollectRequest( new Dependency( artifact, JavaScopes.RUNTIME ),
+ project.getRemoteProjectRepositories() );
+ DependencyRequest dependencyRequest = new DependencyRequest();
+ dependencyRequest.setCollectRequest( collectRequest );
+ DependencyResult dependencyResult = repositorySystem.resolveDependencies(
+ session.getRepositorySession(), dependencyRequest );
+
+ for ( ArtifactResult resolved : dependencyResult.getArtifactResults() )
{
- elements.add( resolved.getFile().getAbsolutePath() );
+ if ( resolved.getArtifact() != null && resolved.getArtifact().getFile() != null )
+ {
+ elements.add( resolved.getArtifact().getFile().getAbsolutePath() );
+ }
Review Comment:
If that's the case then it looks like a bug. Did you actually reproduce that case?
--
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 #169: [MCOMPILER-522] Use maven-resolver to resolve 'annotationProcessorPaths' dependencies
Posted by GitBox <gi...@apache.org>.
psiroky commented on code in PR #169:
URL: https://github.com/apache/maven-compiler-plugin/pull/169#discussion_r1081648930
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1814,29 +1810,26 @@ private List<String> resolveProcessorPathEntries()
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler( coord.getType() );
Artifact artifact = new DefaultArtifact(
- coord.getGroupId(),
- coord.getArtifactId(),
- VersionRange.createFromVersionSpec( coord.getVersion() ),
- Artifact.SCOPE_RUNTIME,
- coord.getType(),
- coord.getClassifier(),
- handler,
- false );
-
- ArtifactResolutionRequest request = new ArtifactResolutionRequest()
- .setArtifact( artifact )
- .setResolveRoot( true )
- .setResolveTransitively( true )
- .setLocalRepository( session.getLocalRepository() )
- .setRemoteRepositories( project.getRemoteArtifactRepositories() );
-
- ArtifactResolutionResult resolutionResult = repositorySystem.resolve( request );
-
- resolutionErrorHandler.throwErrors( request, resolutionResult );
-
- for ( Artifact resolved : resolutionResult.getArtifacts() )
+ coord.getGroupId(),
+ coord.getArtifactId(),
+ coord.getClassifier(),
+ handler.getExtension(),
+ coord.getVersion()
+ );
+
+ CollectRequest collectRequest = new CollectRequest( new Dependency( artifact, JavaScopes.RUNTIME ),
+ project.getRemoteProjectRepositories() );
+ DependencyRequest dependencyRequest = new DependencyRequest();
+ dependencyRequest.setCollectRequest( collectRequest );
+ DependencyResult dependencyResult = repositorySystem.resolveDependencies(
+ session.getRepositorySession(), dependencyRequest );
+
+ for ( ArtifactResult resolved : dependencyResult.getArtifactResults() )
{
- elements.add( resolved.getFile().getAbsolutePath() );
+ if ( resolved.getArtifact() != null && resolved.getArtifact().getFile() != null )
+ {
+ elements.add( resolved.getArtifact().getFile().getAbsolutePath() );
+ }
Review Comment:
No, I am really just talking "in theory". I did not reproduce this. And the code is likely fine as it is.
As someone reading the code for the first time though, if I see a `null` check I expect the "thing" can actually be `null` is some cases, otherwise the `null` check would not be there. So I would either remove the `null` check completely (and have an NPE thrown if this happens) or throw another explicit exception. I haven't really had a preference between the two, hence started the discussion.
--
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] ajarmoniuk commented on a diff in pull request #169: [MCOMPILER-522] Use maven-resolver to resolve 'annotationProcessorPaths' dependencies
Posted by GitBox <gi...@apache.org>.
ajarmoniuk commented on code in PR #169:
URL: https://github.com/apache/maven-compiler-plugin/pull/169#discussion_r1081650624
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1814,29 +1810,26 @@ private List<String> resolveProcessorPathEntries()
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler( coord.getType() );
Artifact artifact = new DefaultArtifact(
- coord.getGroupId(),
- coord.getArtifactId(),
- VersionRange.createFromVersionSpec( coord.getVersion() ),
- Artifact.SCOPE_RUNTIME,
- coord.getType(),
- coord.getClassifier(),
- handler,
- false );
-
- ArtifactResolutionRequest request = new ArtifactResolutionRequest()
- .setArtifact( artifact )
- .setResolveRoot( true )
- .setResolveTransitively( true )
- .setLocalRepository( session.getLocalRepository() )
- .setRemoteRepositories( project.getRemoteArtifactRepositories() );
-
- ArtifactResolutionResult resolutionResult = repositorySystem.resolve( request );
-
- resolutionErrorHandler.throwErrors( request, resolutionResult );
-
- for ( Artifact resolved : resolutionResult.getArtifacts() )
+ coord.getGroupId(),
+ coord.getArtifactId(),
+ coord.getClassifier(),
+ handler.getExtension(),
+ coord.getVersion()
+ );
+
+ CollectRequest collectRequest = new CollectRequest( new Dependency( artifact, JavaScopes.RUNTIME ),
+ project.getRemoteProjectRepositories() );
+ DependencyRequest dependencyRequest = new DependencyRequest();
+ dependencyRequest.setCollectRequest( collectRequest );
+ DependencyResult dependencyResult = repositorySystem.resolveDependencies(
+ session.getRepositorySession(), dependencyRequest );
+
+ for ( ArtifactResult resolved : dependencyResult.getArtifactResults() )
{
- elements.add( resolved.getFile().getAbsolutePath() );
+ if ( resolved.getArtifact() != null && resolved.getArtifact().getFile() != null )
+ {
+ elements.add( resolved.getArtifact().getFile().getAbsolutePath() );
+ }
Review Comment:
I'd just use Objects.requireNonNull. API says an exception should be thrown.
--
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] ajarmoniuk commented on pull request #169: [MCOMPILER-522] Use maven-resolver to resolve 'annotationProcessorPaths' dependencies
Posted by GitBox <gi...@apache.org>.
ajarmoniuk commented on PR #169:
URL: https://github.com/apache/maven-compiler-plugin/pull/169#issuecomment-1396835348
No problem, I'll do that in the evening
--
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 #169: [MCOMPILER-522] Use maven-resolver to resolve 'annotationProcessorPaths' dependencies
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski merged PR #169:
URL: https://github.com/apache/maven-compiler-plugin/pull/169
--
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 #169: [MCOMPILER-522] Use maven-resolver to resolve 'annotationProcessorPaths' dependencies
Posted by GitBox <gi...@apache.org>.
psiroky commented on PR #169:
URL: https://github.com/apache/maven-compiler-plugin/pull/169#issuecomment-1396834486
@ajarmoniuk would you have some time to review? (since you did similar changes in other plugins)
--
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 #169: [MCOMPILER-522] Use maven-resolver to resolve 'annotationProcessorPaths' dependencies
Posted by GitBox <gi...@apache.org>.
psiroky commented on code in PR #169:
URL: https://github.com/apache/maven-compiler-plugin/pull/169#discussion_r1081610573
##########
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java:
##########
@@ -1814,29 +1810,26 @@ private List<String> resolveProcessorPathEntries()
ArtifactHandler handler = artifactHandlerManager.getArtifactHandler( coord.getType() );
Artifact artifact = new DefaultArtifact(
- coord.getGroupId(),
- coord.getArtifactId(),
- VersionRange.createFromVersionSpec( coord.getVersion() ),
- Artifact.SCOPE_RUNTIME,
- coord.getType(),
- coord.getClassifier(),
- handler,
- false );
-
- ArtifactResolutionRequest request = new ArtifactResolutionRequest()
- .setArtifact( artifact )
- .setResolveRoot( true )
- .setResolveTransitively( true )
- .setLocalRepository( session.getLocalRepository() )
- .setRemoteRepositories( project.getRemoteArtifactRepositories() );
-
- ArtifactResolutionResult resolutionResult = repositorySystem.resolve( request );
-
- resolutionErrorHandler.throwErrors( request, resolutionResult );
-
- for ( Artifact resolved : resolutionResult.getArtifacts() )
+ coord.getGroupId(),
+ coord.getArtifactId(),
+ coord.getClassifier(),
+ handler.getExtension(),
+ coord.getVersion()
+ );
+
+ CollectRequest collectRequest = new CollectRequest( new Dependency( artifact, JavaScopes.RUNTIME ),
+ project.getRemoteProjectRepositories() );
+ DependencyRequest dependencyRequest = new DependencyRequest();
+ dependencyRequest.setCollectRequest( collectRequest );
+ DependencyResult dependencyResult = repositorySystem.resolveDependencies(
+ session.getRepositorySession(), dependencyRequest );
+
+ for ( ArtifactResult resolved : dependencyResult.getArtifactResults() )
{
- elements.add( resolved.getFile().getAbsolutePath() );
+ if ( resolved.getArtifact() != null && resolved.getArtifact().getFile() != null )
+ {
+ elements.add( resolved.getArtifact().getFile().getAbsolutePath() );
+ }
Review Comment:
I meant this case -- there is no `DependencyResolutionException`, but also for some reason `resolved.getArtifact()` or `resolved.getArtifact().getFile()` actually returns null. As I understand it, this should not happen, unless there is a bug in the resolver. But then I am wondering if we should just remove those null checks here? Or maybe throw a fail-fast exception to not silently swallow resolved artifacts where the file is `null`?
--
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