-
Notifications
You must be signed in to change notification settings - Fork 154
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
[#2177] Migrate to Java 11 Syntax and Features #2183
Conversation
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 we should also update the docs, particularly the required Java version in the developer guide!
Adding on to what Ryan has mentioned, we should also consider upgrading checkstyle and other parts that can be done following the migration! |
@sopa301 Done! I have updated the docs to reflect the migration from Java 8 to Java 11! |
@supermii2 I agree, we should discuss the different components that can be upgraded and begin implementing the changes in this PR! |
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.
LGTM!
@supermii2 I have upgraded checkstyle to 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.
LGTM!
build.gradle
Outdated
toolVersion = '9.3' | ||
toolVersion = '10.10.0' |
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 would be better if we do the checkstyle upgrade in a different PR, because there may be some changes that break backward compatibility.
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 see, sure thing, I will revert back to the previous commit before the introduction of the new checkstyle tool!
The updating of the Checkstyle tool will be deferred to another PR to avoid causing dependency issues arising from backwards compatibility.
Reapplying revert action as it was reverted on the wrong commit.
The updating of the Checkstyle tool will be deferred to another PR to avoid causing dependency issues arising from backwards compatibility.
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.
LGTM!
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.
Really sorry for the review delays these last 2 weeks! Thanks for the thorough CI and docs updates. Looks most good to go!
Can we remove the killDescendants
in serveTestReportInBackground
at line 202 of build.gradle
?
@gok99 I have removed the flag from the |
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.
LGTM
Right now, the status checks are not passing because the branch protection rules require the "JDK 8" version to pass, whereas this PR changes the CI name to use "JDK 11". Hence, the "Branch protection rules" or "Status checks that are required" settings must be updated to reflect the new name for this PR to be merged. |
@ckcherry23 I've updated the branch protection rules to require the new JDK 11 checks instead. Though the frontend tests seem to be failing somewhat randomly, based on the history of CI runs on this PR. Should probably fix that soon and merge this as it breaks the other PRs. |
The following links are for previewing this pull request:
|
Part of #2177
Proposed commit message
Other information
N/A