-
Notifications
You must be signed in to change notification settings - Fork 20
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
Compilation under JDK11 #56
base: master
Are you sure you want to change the base?
Conversation
@sramazzina Thanks for the PR! What's the reason for bumping the required Java version from 7 to 11? Can we compile with 11 (and newer) without this breaking change? |
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.
A few more comments.
<maven.compiler.source>11</maven.compiler.source> | ||
<maven.compiler.target>11</maven.compiler.target> | ||
<source.jdk.version>11</source.jdk.version> | ||
<target.jdk.version>11</target.jdk.version> | ||
<compilerArgument /> |
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.
See my comment here.
<maven.deploy.skip>true</maven.deploy.skip> | ||
<maven.site.deploy.skip>true</maven.site.deploy.skip> | ||
<gpg.keyname>B1606F22</gpg.keyname> | ||
|
||
<slf4j.version>1.7.25</slf4j.version> | ||
<slf4j.version>2.0.7</slf4j.version> |
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.
Let's not bump the major version of SLF4J. It's not required to make it build with Java 11.
@@ -123,26 +126,34 @@ | |||
<version>2.19.0</version> | |||
<scope>test</scope> | |||
</dependency> | |||
<dependency> | |||
<!-- <dependency> |
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.
Why did you comment this dependency for the test scope?
<plugin> | ||
<groupId>org.codehaus.mojo</groupId> | ||
<artifactId>versions-maven-plugin</artifactId> | ||
<version>2.5</version> | ||
<configuration> | ||
<generateBackupPoms>false</generateBackupPoms> | ||
</configuration> | ||
</plugin> |
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.
What's the purpose of this change?
<compilerId>javac-with-errorprone</compilerId> | ||
<!-- compilerId>javac-with-errorprone</compilerId --> | ||
<forceJavacCompilerUse>true</forceJavacCompilerUse> | ||
<showDeprecation>true</showDeprecation> | ||
<source>${source.jdk.version}</source> | ||
<target>${target.jdk.version}</target> | ||
<compilerArgument>${compilerArgument}</compilerArgument> |
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 don't understand the purpose of these changes. Can you elaborate?
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.
Hi @bernd, I commented it because I remember compiling with that entry enabled I got into a bunch of errors. Tried to google a bit and got answers that says we needed to use a more recent version of the library but I remember I continued to not being able to compile. Let me review the things and I will be back to you with fresh news because I decided to submit the PR one month later I applied these changes and frankly I don't remember. Talk to you soon.
<dependency> | ||
<!-- dependency> | ||
<groupId>org.codehaus.plexus</groupId> | ||
<artifactId>plexus-compiler-javac-errorprone</artifactId> | ||
<version>2.8.4</version> | ||
<version>2.13.0</version> | ||
</dependency> | ||
<dependency> | ||
<groupId>com.google.errorprone</groupId> | ||
<artifactId>error_prone_core</artifactId> | ||
<version>2.3.1</version> | ||
</dependency> | ||
<version>2.18.0</version> | ||
</dependency --> |
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.
Can you elaborate why you commented these?
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.
See my previous comment
Just made some changes to have the code compiling under JDK 11 (see issue #44) . I hope you find it interesting and you will merge it. Let me know in case of any questions
Notes for Reviewers