-
Notifications
You must be signed in to change notification settings - Fork 914
Fixing handling of multi-source JavaSources on older JDKs. #3321
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
Conversation
| } | ||
| } | ||
| } finally { | ||
| if (lowMemoryCancel.get()) { |
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.
OK, centralized low-memory conditions for all types of failures, not just Runtime + CancelAbort.
| } | ||
| } | ||
| currentPhase = Phase.RESOLVED; | ||
| if (!lowMemoryCancel.get()) { |
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.
Completed without an exception/error, but still low memory cancel ?
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.
Yes, old(er) versions of javac will not propagate the exceptions from the parse and analyze methods. See:
openjdk/jdk@4333942
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.
@jlahoda Thanks for expanation - I don't have such a wide context, so now I better understand the core piece of this PR :)
|
my understanding was that NB 13 won't have a mode without nb-javac. Its installed by default, based on JDK 17 (backported with jackpot) and will stay enabled even when NB is started on 18+. |
Jan fixes a problem that exists now. We can speculate/plan the future, but it has not happened yet: the relevant PRs are #3251 and #2783. They are not merged yet. |
I saw that yes, that was the reason for my comment. Even if the PRs are not merged yet (it would be a good time to do it soon btw, early in the cycle), I had the impression that the decision was pretty much already made - looks like i was wrong. |
|
I noticed both of the above mentioned javac PRs are now integrated. This would make JDK 17 javac the new baseline, right?. |
|
@jlahoda Do we still need this one? |
|
No reply so rather than bump another milestone, will close. Reopen if still a need. |
The multi-file JavaSources should work reasonably well after #2959 on new JDKs/with nb-javac, but due to implementation changes in javac, they may not work well on older JDKs (like JDK 14/11). While overall I think we should eventually drop support for older JDKs without (some sort of) nb-javac, that is not the case currently, so trying to make the multi-file sources work also on older JDKs (which should improve the random failures on Travis).