-
-
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-54842] Define compiler configuration with Maven properties where possible and use Java 11 built-in functionality instead of Animal Sniffer #523
Conversation
@@ -338,7 +342,6 @@ | |||
<artifactId>maven-javadoc-plugin</artifactId> | |||
<version>3.3.2</version> | |||
<configuration> | |||
<source>8</source> |
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.
The default value is ${maven.compiler.source}
, which we are setting above.
<properties> | ||
<maven.compiler.release>8</maven.compiler.release> | ||
<maven.compiler.testRelease>8</maven.compiler.testRelease> | ||
<animal.sniffer.skip>true</animal.sniffer.skip> |
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.
Technically a change in behavior, but release
serves the same purpose as Animal Sniffer.
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.
Well, almost. With A.S. you can use @IgnoreJRERequirements
on a particular class/method. I do not recommend it (better to use reflection) and would not miss that capability being dropped.
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.
General style is to prefer <plugin>
/ <configuration>
to setting <properties>
, so this is slightly undesirable on its own, but of course the main consideration is whether or not we do jenkinsci/maven-hpi-plugin#323.
<properties> | ||
<maven.compiler.release>8</maven.compiler.release> | ||
<maven.compiler.testRelease>8</maven.compiler.testRelease> | ||
<animal.sniffer.skip>true</animal.sniffer.skip> |
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.
Well, almost. With A.S. you can use @IgnoreJRERequirements
on a particular class/method. I do not recommend it (better to use reflection) and would not miss that capability being dropped.
Is there a request for change on this PR? If not, why was this PR not approved? |
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.
Yes approved conditionally on jenkinsci/maven-hpi-plugin#323 being merged, sorry for being unclear.
This change should be functionally equivalent to the existing code, and by itself it doesn't buy us anything, but in concert with jenkinsci/maven-hpi-plugin#323, this allows us to require Java 11 in the future without an awkward flag day.