Skip to content
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

Upgrade to nb-javac from JDK 23, build 30 #7484

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Jun 14, 2024

A draft for now, but if/when tests are OK with this version of javac, I think we should include it. (And, eventually, upgrade to a newer version when it becomes available, of course.)

@lahodaj lahodaj added the ci:all-tests [ci] enable all tests label Jun 14, 2024
@mbien
Copy link
Member

mbien commented Jun 14, 2024

goodbye string templates, lets hope they will be back at some point in future.

This is something we should mention on the download page since it might be surprising for users that NB 23 won't know about STs anymore even when the project is on JDK 21.

@lahodaj
Copy link
Contributor Author

lahodaj commented Jun 14, 2024

goodbye string templates, lets hope they will be back at some point in future.

This is something we should mention on the download page since it might be surprising for users that NB 23 won't know about STs anymore even when the project is on JDK 21.

Agreed. Although I don't see anything we could reasonably technically do to mitigate the situation.

@mbien mbien added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) Upgrade Library Library (Dependency) Upgrade ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Jun 14, 2024
@mbien mbien added this to the NB23 milestone Jun 14, 2024
@mbien mbien linked an issue Jun 14, 2024 that may be closed by this pull request
@mbien
Copy link
Member

mbien commented Jun 14, 2024

Although I don't see anything we could reasonably technically do to mitigate the situation.

agreed. String templates were API+language and implemented in java.base and jdk.compiler. So even when we would create a frankenstein nb-javac and partially revert openjdk/jdk#18688 it would create problems.
We also don't know how the next version of string templates will look like or when it will arrive - as soon it is available it would break the JDK 21 templates anyway. (preview features are locked to the JDK version for a reason - otherwise the JDK would have to provide multiple implementations of the same feature)

This is the simplest approach, so +1 from me. Thanks for working on this!

Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes look good, left a few comments inline

ran a smoke test and NB worked fine with nb-javac 23+26.

before merge, don't forget to:

  • switch the staged dependencies to a release build
  • squash as you see fit

adding do-not-merge label just to be sure!

@mbien mbien added the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Jun 26, 2024
@mbien mbien mentioned this pull request Jul 3, 2024
@matthiasblaesing
Copy link
Contributor

@lahodaj would you mind finalizing this? While looking into the JSF modules in the NetBeans codebase I also got exceptions from the ThisEscapeAnalyser and I have some hopes, that they are fixed by the update of nb-javac. I would test this one, but the referenced binaries seem not to have been released and thus were not promoted to central, so the nb-javac release has to be respun.

@matthiasblaesing
Copy link
Contributor

Build a local test version of nb-javac and build netbeans with this applied: build.patch.txt

First impression is good, I plan to run with it the next week.

@lahodaj
Copy link
Contributor Author

lahodaj commented Jul 8, 2024

The plan is to finalize with a new (currently up-to-date) javac. Ideally, sometime during this week, I think. Thanks for the comments!

@mbien
Copy link
Member

mbien commented Jul 8, 2024

this PR would have to bump the two 22 values to 23-ea for the java hints job like we did in past a few times:

java: [ '17', '22' ]
config: [ 'batch1', 'batch2' ]
exclude:
- java: ${{ github.event_name == 'pull_request' && 'nothing' || '22' }}

this would allow us to merge #7525 without having to wait for the nb-javac changes.

@mbien mbien self-requested a review July 8, 2024 15:48
@lahodaj lahodaj changed the title Upgrade to nb-javac from JDK 23, build 26 Upgrade to nb-javac from JDK 23, build 30 Jul 10, 2024
@lahodaj
Copy link
Contributor Author

lahodaj commented Jul 10, 2024

The nb-javac binary download has been updated to JDK 23 build 30, as uploaded to Maven central, I think I've fixed all the comments so far. So I think this is ready for review now. Thanks!

@lahodaj lahodaj marked this pull request as ready for review July 10, 2024 11:32
Copy link
Contributor

@dbalek dbalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me.

.github/workflows/main.yml Outdated Show resolved Hide resolved
@mbien mbien self-requested a review July 10, 2024 11:52
Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! Please don't forget to squash as you see fit.

@mbien mbien removed the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Jul 10, 2024
@lahodaj
Copy link
Contributor Author

lahodaj commented Jul 10, 2024

Thanks! I'll merge if there's a clean test run.

@mbien
Copy link
Member

mbien commented Jul 10, 2024

had to restart the platform batch2 and groovy jobs, but the failures look like normal flakiness unrelated to nb-javac. Should be green in a few minutes.

@mbien mbien merged commit fcb864f into apache:master Jul 10, 2024
40 checks passed
@mbien mbien linked an issue Aug 5, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:all-tests [ci] enable all tests ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) Upgrade Library Library (Dependency) Upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parsing of java code with broken lambdas fails Fails to parse a file with incomplete switch expression
4 participants