-
Notifications
You must be signed in to change notification settings - Fork 821
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
Update nb-javac to 19+33. #4467
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.
awesome! please update:
- version check to 19 as in version bump: require javac 18. #3953 line: https://github.com/apache/netbeans/pull/3953/files#diff-4acb756ec7378fd25f5bc06c331506df4d42d67d21b4356f8c90ff35dfaa1f75R37
- the module description as in updated nb-javac18 plugin version and description #4010
- ignored-overlaps file as seen in upgrade nb-javac to 18.0.1. #4106 https://github.com/apache/netbeans/pull/4106/files#diff-afa15c4cdb24d884c580b41d4e36504eb2576b904623433f15d385cf784be6a6 (i don't know what this file does, but it id going to have the wrong version in it)
- bump this line to 20 to enable testing on JDK 19 with xserver enabled
netbeans/.github/workflows/main.yml
Line 106 in 6155cbf
if: ${{ matrix.java != '19-ea' }} # see #4299
would be good to have everything in one PR so that we can use it as template and won't forget things anymore.
...java.source.base/src/org/netbeans/modules/java/source/transform/ImmutableTreeTranslator.java
Outdated
Show resolved
Hide resolved
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.
Obviously the binaries need distributing via Maven and the binaries-list files updating rather than deleting before this can be merged.
Is there a reason for the PR in current state? Be good to get the binaries sorted before using Travis and GitHub CI overhead for this.
This is just a temporary state (see do-not-merge badge) to check NB build and tests with the new nb-javac binary before pushing it to Maven. Will be changed to updating binaries-list later. |
I assumed so - thanks for the updated description. You can use URLs in |
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.
looks good to me as far as I can tell - thanks for updating all the version checks and descriptions.
I only left a comment for a small refactoring opportunity I noticed and a question.
java/java.source.base/src/org/netbeans/api/java/source/TreeMaker.java
Outdated
Show resolved
Hide resolved
java/java.source.base/src/org/netbeans/api/java/source/TreeMaker.java
Outdated
Show resolved
Hide resolved
java/java.source.base/src/org/netbeans/modules/java/source/save/Reformatter.java
Show resolved
Hide resolved
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.
Seems reasonable.
@neilcsmith-net: Changes requested are IMHO done, do you want to update your review before merge ? |
i think we have to update javac again before NB 16 release, right? 19 GA is 19+36, we have 19+33 right now. |
Updating nb-javac to version 19+33.