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

[JENKINS-54842] Define compiler configuration with Maven properties where possible and use Java 11 built-in functionality instead of Animal Sniffer #523

Merged
merged 5 commits into from Mar 31, 2022

Conversation

basil
Copy link
Member

@basil basil commented Mar 30, 2022

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.

@basil basil added the internal label Mar 30, 2022
@basil basil requested a review from jglick March 30, 2022 21:33
@@ -338,7 +342,6 @@
<artifactId>maven-javadoc-plugin</artifactId>
<version>3.3.2</version>
<configuration>
<source>8</source>
Copy link
Member Author

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>
Copy link
Member Author

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.

Copy link
Member

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.

@basil basil changed the title Define compiler configuration with Maven properties where possible [JENKINS-54842] Define compiler configuration with Maven properties where possible Mar 31, 2022
@basil basil changed the title [JENKINS-54842] Define compiler configuration with Maven properties where possible [JENKINS-54842] Define compiler configuration with Maven properties where possible and use Java 11 built-in functionality instead of Animal Sniffer Mar 31, 2022
Copy link
Member

@jglick jglick left a 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>
Copy link
Member

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.

@basil
Copy link
Member Author

basil commented Mar 31, 2022

Is there a request for change on this PR? If not, why was this PR not approved?

Copy link
Member

@jglick jglick left a 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.

@basil basil merged commit 943a57b into jenkinsci:master Mar 31, 2022
@basil basil deleted the properties branch March 31, 2022 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants