-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Addressed Java 17 compile warnings #769
Conversation
@javadev Thanks for taking care of this, will review later today. |
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.
It looks like there is a warning about the bootstrap not being set properly.
Source Main:
Source Test:
Can you try a few option in the Maven configuration to see about adding the new "release" flag?
I found this StackOverflow relating to the warning: https://stackoverflow.com/questions/59097317/warning-options-bootstrap-class-path-not-set-in-conjunction-with-source-8
Do note that this may be a non-issue as the warning is relating to accidentally compiling with a newer version of the JDK, thus accidentally using new APIs that may not be available in the old JDK being targeted. Since our build pipeline is cross compiling with explicit versions of the JDK that we support, this should NOT be an issue as the JDK 8 part of the pipeline would fail if someone accidentally used an API that wasn't available in JDK 8. Point being, If this isn't easily fixable, I would not worry about it. |
I have reviewed the logs, and it appears that these warnings are specific to the Java compilers 11 and 17. It may be advisable to consider creating alternative pom.xml files for these compilers and specifying Java 11 or 17 accordingly. |
The pull request is prepared for merging. |
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.
Thanks for looking into it. At this time I don't think multiple POM are needed based on the documentation for the warning.
What problem does this code solve? Risks Changes to the API? Will this require a new release? Should the documentation be updated? Does it break the unit tests? Was any code refactored in this commit? Review status Starting 3-day comment window |
stleary commented 3 days ago |
No description provided.