Uploaded image for project: 'Maven'
  1. Maven
  2. MNG-6233

maven-resolver-provider mixes JRS 330 and Plexus annotations

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.3.9, 3.5.0
    • Fix Version/s: 3.5.2
    • Component/s: None
    • Labels:
      None

      Description

      Mixed annotations confuse guice/sisu and result in hard to troubleshoot and impossible to workaround problems in applications that embed Maven core runtime, like m2e and gshell.

      I believe plugins annotations where left in the code by mistake so the plan is to update the code to use jsr330 exclusively and completely remove plexus annotations. This change is fully transparent to the users (and we've been using it internally for couple of months now).

      See also https://github.com/apache/maven/pull/116

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ifedorenko commented on the issue:

          https://github.com/apache/maven/pull/116

          Opened https://issues.apache.org/jira/browse/MNG-6233. If you any concerns or suggestions, I suggest we continue the discussion there.

          @jdillon I can't close this pull request

          Show
          githubbot ASF GitHub Bot added a comment - Github user ifedorenko commented on the issue: https://github.com/apache/maven/pull/116 Opened https://issues.apache.org/jira/browse/MNG-6233 . If you any concerns or suggestions, I suggest we continue the discussion there. @jdillon I can't close this pull request
          Hide
          michael-o Michael Osipov added a comment -

          Looking forward to see the branch. Do you think we can provide an IT for that?

          Show
          michael-o Michael Osipov added a comment - Looking forward to see the branch. Do you think we can provide an IT for that?
          Hide
          igorf Igor Fedorenko added a comment -

          I don't believe IT is justified here. The change completely removes plexus-annotations support from the project and I think it is proof enough the two annotations are not being used simultaneously. From what I recall, the plan has always been to fully migrate to jsr330, we just never spent the time to do that.

          Show
          igorf Igor Fedorenko added a comment - I don't believe IT is justified here. The change completely removes plexus-annotations support from the project and I think it is proof enough the two annotations are not being used simultaneously. From what I recall, the plan has always been to fully migrate to jsr330, we just never spent the time to do that.
          Hide
          rfscholte Robert Scholte added a comment -

          I assume we still generate the metadata-descriptor to have faster access compared to class scanning.

          Show
          rfscholte Robert Scholte added a comment - I assume we still generate the metadata-descriptor to have faster access compared to class scanning.
          Hide
          michael-o Michael Osipov added a comment -

          Sounds reasonable. Though, I cannot even find an issue for the JSR330 move. Maybe we need an umbrella ticket and change module by module?

          Show
          michael-o Michael Osipov added a comment - Sounds reasonable. Though, I cannot even find an issue for the JSR330 move. Maybe we need an umbrella ticket and change module by module?
          Hide
          rfscholte Robert Scholte added a comment -

          IIRC Jason van Zyl prepared this on his system. However, JSR330 doesn't support different configurations with the same implementation, like we do for lifecycles. Not sure what happened to that part.
          If Jason can apply it on the current sourcetree without issues, we could do that. Otherwise let's go for a per-module upgrade, although I think it should all fit within one release. That said, having 1 issue with a checklist for all modules is good enough for me.

          Show
          rfscholte Robert Scholte added a comment - IIRC Jason van Zyl prepared this on his system. However, JSR330 doesn't support different configurations with the same implementation, like we do for lifecycles. Not sure what happened to that part. If Jason can apply it on the current sourcetree without issues, we could do that. Otherwise let's go for a per-module upgrade, although I think it should all fit within one release. That said, having 1 issue with a checklist for all modules is good enough for me.
          Hide
          igorf Igor Fedorenko added a comment -

          We already have plenty of jsr330 components in maven, so this is my no means new in Maven. I was slowly replacing plexus annotations with jsr330 over last few years, but if anyone feels like doing bulk conversion I won't object

          Robert Scholte maven does not do classpath scanning for jsr330 components, check how plexus container is created in MavenCli.

          Show
          igorf Igor Fedorenko added a comment - We already have plenty of jsr330 components in maven, so this is my no means new in Maven. I was slowly replacing plexus annotations with jsr330 over last few years, but if anyone feels like doing bulk conversion I won't object Robert Scholte maven does not do classpath scanning for jsr330 components, check how plexus container is created in MavenCli.
          Hide
          igorf Igor Fedorenko added a comment -

          Pushed proposed change to https://git-wip-us.apache.org/repos/asf?p=maven.git;a=shortlog;h=refs/heads/MNG-6233_maven-resolver-provider-jsr330. Let me know if you agree/disagree to include this change in 3.5.1. Like I said, we've used this in our local version of maven for some time now without any problems. All ITs pass for me locally too.

          Show
          igorf Igor Fedorenko added a comment - Pushed proposed change to https://git-wip-us.apache.org/repos/asf?p=maven.git;a=shortlog;h=refs/heads/MNG-6233_maven-resolver-provider-jsr330 . Let me know if you agree/disagree to include this change in 3.5.1. Like I said, we've used this in our local version of maven for some time now without any problems. All ITs pass for me locally too.
          Hide
          jdillon Jason Dillon added a comment -

          Igor Fedorenko changes look reasonable, and tested locally seems to function as expected.

          Show
          jdillon Jason Dillon added a comment - Igor Fedorenko changes look reasonable, and tested locally seems to function as expected.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user jdillon commented on the issue:

          https://github.com/apache/maven/pull/116

          Igor already created https://issues.apache.org/jira/browse/MNG-6233 but if you really need another issue I can file one just to fix this specific problem.

          Seems a bit odd though that you want an IT to apply a clear coding mistake due to the ctor parameter being ignored and not setting the component causing NPE when used.

          Show
          githubbot ASF GitHub Bot added a comment - Github user jdillon commented on the issue: https://github.com/apache/maven/pull/116 Igor already created https://issues.apache.org/jira/browse/MNG-6233 but if you really need another issue I can file one just to fix this specific problem. Seems a bit odd though that you want an IT to apply a clear coding mistake due to the ctor parameter being ignored and not setting the component causing NPE when used.
          Show
          igorf Igor Fedorenko added a comment - Submitted the fix as https://git-wip-us.apache.org/repos/asf?p=maven.git;a=commit;h=66fc74d6296ea0a33f8a9712dc5ed5eb3affd529
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build maven-3.x #1653 (See https://builds.apache.org/job/maven-3.x/1653/)
          MNG-6233 don't mix plexus and jsr330 annotations in aether-provider (ifedorenko: http://git-wip-us.apache.org/repos/asf/?p=maven.git&a=commit&h=66fc74d6296ea0a33f8a9712dc5ed5eb3affd529)

          • (edit) maven-resolver-provider/pom.xml
          • (edit) maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/VersionsMetadataGeneratorFactory.java
          • (edit) maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultArtifactDescriptorReader.java
          • (edit) maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionResolver.java
          • (edit) maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionRangeResolver.java
          • (edit) maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/SnapshotMetadataGeneratorFactory.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build maven-3.x #1653 (See https://builds.apache.org/job/maven-3.x/1653/ ) MNG-6233 don't mix plexus and jsr330 annotations in aether-provider (ifedorenko: http://git-wip-us.apache.org/repos/asf/?p=maven.git&a=commit&h=66fc74d6296ea0a33f8a9712dc5ed5eb3affd529 ) (edit) maven-resolver-provider/pom.xml (edit) maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/VersionsMetadataGeneratorFactory.java (edit) maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultArtifactDescriptorReader.java (edit) maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionResolver.java (edit) maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionRangeResolver.java (edit) maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/SnapshotMetadataGeneratorFactory.java
          Hide
          michael-o Michael Osipov added a comment - - edited

          There are two problems now:

          1. No one seconded this candidate. We agreed to have a seconder on the dev mailing list
          2. You didn't read my comments on GitHub and merged Jason Dillon's changed with this PR although this is not related.

          Show
          michael-o Michael Osipov added a comment - - edited There are two problems now: 1. No one seconded this candidate. We agreed to have a seconder on the dev mailing list 2. You didn't read my comments on GitHub and merged Jason Dillon 's changed with this PR although this is not related.
          Hide
          igorf Igor Fedorenko added a comment -

          My apologies, I assumed I addressed all concerned about my change here already and didn't ask on the mailing list. Let me do that now and I'll revert my change if I don't get +1s (or get -1s) within 72 hours.

          I am not sure what you mean by "merged Jason Dillon's changed with this PR". The change I propose fully converts maven-resolver-provider to jsr330 annotations, which means only jsr330 codepaths are used after the change and our existing ITs prove everything works. In other words, it is not possible to implement jsr330 conversion (i.e. this change) without fixing Jason's problem too, so this change really supersedes PR 116.

          Show
          igorf Igor Fedorenko added a comment - My apologies, I assumed I addressed all concerned about my change here already and didn't ask on the mailing list. Let me do that now and I'll revert my change if I don't get +1s (or get -1s) within 72 hours. I am not sure what you mean by "merged Jason Dillon's changed with this PR". The change I propose fully converts maven-resolver-provider to jsr330 annotations, which means only jsr330 codepaths are used after the change and our existing ITs prove everything works. In other words, it is not possible to implement jsr330 conversion (i.e. this change) without fixing Jason's problem too, so this change really supersedes PR 116.
          Hide
          michael-o Michael Osipov added a comment -

          I am not sure what you mean by "merged Jason Dillon's changed with this PR". The change I propose fully converts maven-resolver-provider to jsr330 annotations, which means only jsr330 codepaths are used after the change and our existing ITs prove everything works. In other words, it is not possible to implement jsr330 conversion (i.e. this change) without fixing Jason's problem too, so this change really supersedes PR 116.

          OK, that's what I wanted to know.
          Though, the commit message does not has the same title as the JIRA issue and you should have simply added "This closes #116" too.

          The change itself is perfectly fine.

          Show
          michael-o Michael Osipov added a comment - I am not sure what you mean by "merged Jason Dillon's changed with this PR". The change I propose fully converts maven-resolver-provider to jsr330 annotations, which means only jsr330 codepaths are used after the change and our existing ITs prove everything works. In other words, it is not possible to implement jsr330 conversion (i.e. this change) without fixing Jason's problem too, so this change really supersedes PR 116. OK, that's what I wanted to know. Though, the commit message does not has the same title as the JIRA issue and you should have simply added "This closes #116" too. The change itself is perfectly fine.

            People

            • Assignee:
              igorf Igor Fedorenko
              Reporter:
              igorf Igor Fedorenko
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development