-
-
Notifications
You must be signed in to change notification settings - Fork 75
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
[JENKINS-57852] - Deprecate Java 7 targets support in Plugin POM #207
[JENKINS-57852] - Deprecate Java 7 targets support in Plugin POM #207
Conversation
Co-Authored-By: Jesse Glick <jglick@cloudbees.com>
thanks @jglick , quickhack 🤦♂ |
<activation> | ||
<property> | ||
<name>java.level</name> | ||
<value>7</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There may be some plugins using 6 out there still, though I suppose they are not using a modern POM either…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no good way to do conditional profiles or to chain them. Hit the same in Java 11 support PRs. So I just did the check for Java 7, but can copy-paste if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is good enough, just noting this.
Co-Authored-By: Jesse Glick <jglick@cloudbees.com>
</goals> | ||
<configuration> | ||
<tasks> | ||
<echo>WARNING: Plugin POM defines java.level=${java.level} which is deprecated in the current Plugin POM version. See https://github.com/jenkinsci/plugin-pom/blob/master/README.md#java-support</echo> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside, maybe for #133, we ought to be deprecating this property and introducing a new one which expects an actual Java specification version—thus 1.8
, 11
, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this would properly be a good time to do it 😓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe possible to combine the tasks? Have a profile issuing a warning with two different clauses if you have java.level
defined at all, and introduce the new property defaulting to 1.${java.level}
. Just a thought; it is not a blocker for this PR.
Will merge it taking the feedback in the mailing list |
Follow-up to https://groups.google.com/forum/#!topic/jenkinsci-dev/SdMOMKs7XIQ
Should unblock #200 and other similar PRs