Skip to content

Propagate canUseRelease into CustomJavacClassLoader#9383

Merged
jtulach merged 1 commit intoapache:masterfrom
jtulach:CanUseRelease
May 9, 2026
Merged

Propagate canUseRelease into CustomJavacClassLoader#9383
jtulach merged 1 commit intoapache:masterfrom
jtulach:CanUseRelease

Conversation

@jtulach
Copy link
Copy Markdown
Contributor

@jtulach jtulach commented May 6, 2026

@jtulach jtulach self-assigned this May 6, 2026
@jtulach
Copy link
Copy Markdown
Contributor Author

jtulach commented May 6, 2026

  • @matthiasblaesing suggested to remove the canUseRelease code altogether
  • I would rather fix the code to honor canUseRelease everywhere as this PR does
    • using --release is preferred over --source and --target in my opinion
    • the --release exposes only official Java API
    • when nb-javac build of NetBeans succeeds, we are then sure that regular modules don't use sun.misc.Unsafe & co. ...
    • ... which was a typical problem before I implemented this replace source and target by release when possible check
  • with the change here and 1:1 semantics with DirectModuleWrapper over java.lang.Module JaroslavTulach/nb-javac#39 I can build ant build -Dnbjavac.class.path=/nb-javac/make/langtools/netbeans/nb-javac/dist/nb-javac-jdk-26+35-*jar successfully

@neilcsmith-net
Copy link
Copy Markdown
Member

I know @ebarboni planned to run the release vote this week. This isn't currently labelled high or critical so as is won't go in NB30. If you think it needs to, then justification and reason for additional RC and week delay needs adding to the PR.

@jtulach
Copy link
Copy Markdown
Contributor Author

jtulach commented May 6, 2026

I know @ebarboni planned to run the release vote this week. This isn't currently labelled high or critical so as is won't go in NB30. If you think it needs to, then justification and reason for additional RC and week delay needs adding to the PR.

Thank you Neil. No reason to require additional RC and have a week delay because of this change. If there is some other "high" or "critical" reason for another RC, then we could include this PR. Otherwise, we just integrate it into master.

@matthiasblaesing matthiasblaesing added the ci:all-tests [ci] enable all tests label May 6, 2026
Copy link
Copy Markdown
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Looks sane to me and can IMHO be merged to master, not delivery. I don't see a reason to interfere with release for this.

Independent from that I disagree with the assessment, that having the magic --target == --source maps to --release in there is a good idea. Many modules are already are correctly configured in their properties, to use javac.release, it would be better to complete the transition there instead of this magic.

@apache apache locked and limited conversation to collaborators May 6, 2026
@apache apache unlocked this conversation May 6, 2026
Copy link
Copy Markdown
Contributor

@lahodaj lahodaj left a comment

Choose a reason for hiding this comment

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

Looks OK to me. We can cleanup later when we stop using javac.source in NB.

@ebarboni ebarboni added the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label May 7, 2026
@ebarboni
Copy link
Copy Markdown
Contributor

ebarboni commented May 7, 2026

ok, flagging do not merge to not mess up. If you rebase on master please remove the label.

Btw I will try to do the release on monday. Running out of time

@matthiasblaesing
Copy link
Copy Markdown
Contributor

@jtulach I suggest to retarget this to master, check that target is indeed master, rerun checks and if they come back green merge.

@jtulach jtulach changed the base branch from delivery to master May 9, 2026 07:11
@jtulach jtulach removed the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label May 9, 2026
@jtulach
Copy link
Copy Markdown
Contributor Author

jtulach commented May 9, 2026

Looks sane to me and can IMHO be merged to master, not delivery. I don't see a reason to interfere with release for this.

OK, targeting master. We are still green.

Independent from that I disagree with the assessment, that having the magic --target == --source maps to --release in there is a good idea. Many modules are already are correctly configured in their properties, to use javac.release,

Yeah, using javac.release where possible is certainly better and less magical than the current "switch"...

We can cleanup later when we stop using javac.source in NB.

...but as Jan wrote. Let that happen in other PRs.

@jtulach jtulach merged commit 1ca1c48 into apache:master May 9, 2026
73 of 76 checks passed
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants