Skip to content

Propagate canUseRelease into CustomJavacClassLoader#9383

Open
jtulach wants to merge 1 commit intoapache:deliveryfrom
jtulach:CanUseRelease
Open

Propagate canUseRelease into CustomJavacClassLoader#9383
jtulach wants to merge 1 commit intoapache:deliveryfrom
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.

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 do not merge Don't merge this PR, it is not ready or just demonstration purposes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants