Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIXED JENKINS-36872] Switch to com.mysema.maven:apt-maven-plugin for Java 8 support #2724

Merged
merged 2 commits into from Feb 18, 2017

Conversation

olivergondza
Copy link
Member

JENKINS-36872

This is a blocker for dropping Java 7 support as the release is broken by this. The LTS RC at least. I reproduced the release is passing with this change.

@kohsuke, @rtyler, @jglick, Any idea how to verify this does not break https://wiki.jenkins-ci.org/display/JENKINS/Extension+points? If it is needed at all...

@daniel-beck
Copy link
Member

how to verify

Easy:

last edited by Michael Rooney on Nov 22, 2016

That page is supposed to be updated by https://github.com/jenkins-infra/backend-extension-indexer but it hasn't run in a while. And once we have INFRA-947, this needs to move off the wiki anyway.

@olivergondza
Copy link
Member Author

@daniel-beck, so you suggest we should stop updating it? Or just that this is probably not all that risky since it is already broken?

@daniel-beck
Copy link
Member

The latter. But I really don't know how this is used at all. Maybe dead code anyway? Have you looked into that?

@olivergondza
Copy link
Member Author

I failed to observe any difference in target directories or release binaries with using old version, new version or removing the the plugin at all. I can only speak for LTS rc release, though.

I am ok to remove this as it is either broken or something else is that prevent this from having desired effect. Releasing on Java 8 is the priority here.

@daniel-beck daniel-beck added the needs-more-reviews Complex change, which would benefit from more eyes label Jan 25, 2017
@olivergondza
Copy link
Member Author

@jenkinsci/code-reviewers?

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Feb 15, 2017

The maven repository site shows that the com.mysema.maven.apt-maven-plugin has released versions through 1.1.3 and last released in Nov 2014, while the org.codehaus.mojo.apt-maven-plugin has released versions through 1.0-alpha5 and last released in Jul 2012. That's enough for me to believe that the com.mysema.maven.apt-maven-plugin is the better choice.

I think this change should be made, since it moves from an alpha version to a released version, and from an older version to a newer version, and it allows Java 8. Even if it breaks some existing user, we need to move to Java 8.

I'm also OK with removing it. In either case, users referencing it may see a change in the transition from the 1.0-alpha5 to 1.1.3, and they can declare it themselves.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current level of Extension Points investigation is good enough. 👍 for going forward with merge

@oleg-nenashev oleg-nenashev added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Feb 17, 2017
@oleg-nenashev oleg-nenashev merged commit 37d2e6a into jenkinsci:master Feb 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
4 participants