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.

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.

4 participants