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

-Yrelease supplements -release, allows access to additional JVM packages #10543

Merged
merged 1 commit into from Jan 24, 2024

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Sep 14, 2023

Legacy usage of sun.misc.Unsafe makes it difficult to leverage -release in a build, since the internal class is not available as API. To enable this common use case, -Yrelease is introduced to allow access to specified platform packages.

For example, -release:8 -Yrelease:sun.misc.

This "private" option is a convenience intended to support a project while migrating away from using Unsafe.

Fixes scala/bug#12643

@scala-jenkins scala-jenkins added this to the 2.13.13 milestone Sep 14, 2023
@som-snytt som-snytt marked this pull request as ready for review September 16, 2023 01:31
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Sep 19, 2023
@SethTisue
Copy link
Member

SethTisue commented Sep 19, 2023

@som-snytt it seems likely we'll merge this. but by merge time we'll need a public-facing PR description explaining what this does and when it's needed. but I think it would actually be helpful if you provided that now, to help reviewers?

@SethTisue
Copy link
Member

Another thought: do you intend to port this to Scala 3? We're always a bit reluctant to add things to Scala 2 unless the path to supporting them in 3 as well is clear. That's partly because the 3 folks aren't happy if they feel we are putting pressure on them to support things they don't want to support, and also partly because users can be annoyed or disappointed if the two versions seem gratuitously misaligned to them.

Do we know if the workaround suggested on the scala/bug ticket also works in Scala 3? Because if it does, that would ease the 2 vs 3 tension here a bit, since 3 users wouldn't be blocked.

@som-snytt
Copy link
Contributor Author

-Yunsafe is a convenience to get a Scala 2 project to use --release, for a net gain in API safety without too much hassle.

A project that is cross-built probably has the machinery to build against the jar of its choice, if Unsafe is part of its long-term strategy, so that would not be a use case supported by this private option.

@SethTisue
Copy link
Member

SethTisue commented Sep 19, 2023

Nice description — thank you.

Let's ponder the naming, prior to merge. -Yunsafe's broadness is a bit cryptic and threatening. Maybe something that makes it clear it's 1) a very specific/niche thing, and/or 2) a modifier to --release? like -Yrelease-include-unsafe or -Yopen-internal-packages something? thinking out loud

Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

This LGTM, it's a very nice idea!

@som-snytt som-snytt changed the title -Yunsafe opens packages in jrt under --release -Yopen-packages opens packages in jrt under --release Dec 6, 2023
@som-snytt
Copy link
Contributor Author

Instead of release-notes the label should be --release notes.

I assUME that NonFatal is adequate. I don't remember if I had noticed something thrown such as a LinkageError (to guess).

Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

LGTM. @SethTisue wdyt about the flag name -Yopen-packages?

@som-snytt
Copy link
Contributor Author

som-snytt commented Dec 7, 2023

Maybe it should be allow-packages. I was thinking of modules. Edit: use-packages? oh, require-packages.

@SethTisue SethTisue removed their assignment Jan 18, 2024
@SethTisue SethTisue changed the title -Yopen-packages opens packages in jrt under --release Add -Yopen-packages, for opening JVM-internal packages under --release Jan 18, 2024
@SethTisue
Copy link
Member

SethTisue commented Jan 18, 2024

I'm almost ready to hit "approve" here, only remaining concerns are naming and doc

wdyt about the flag name -Yopen-packages?

@som-snytt what is the relationship between this new flag and --add-opens? I'm asking for my own understanding, and also because it could affect the naming, and also because it could affect what we put in the PR description

P.S. needs rebase!

@som-snytt
Copy link
Contributor Author

Apparently, -Yunsafe was the right name all along. Who's been messing with DirectoryClassPath?

An interesting post-mortem would be rates of change to modules (classes and packages), as a measure of what was so complicated.

@He-Pin
Copy link
Contributor

He-Pin commented Jan 22, 2024

Thank you for this.
Will there a '-Yexport-packages' too?

@som-snytt
Copy link
Contributor Author

This is just for considering packages on the real class path during compilation. Clearly it needs a better name

-Yrerelease? Because you use it when --release which hides the package and you want to release it for use. -Yunrelease.

oh just --release:8,sun.misc because you're saying use release 8 of the API but include this extra package.

@som-snytt
Copy link
Contributor Author

--release:8,sun.misc would be nice, but it also creates a cross-building burden, besides complicating help and parsing.

Instead, we get a companion option -Yrelease:sun.misc which allows look-ups in the named packages when --release is set by the user.

@som-snytt som-snytt changed the title Add -Yopen-packages, for opening JVM-internal packages under --release -Yrelease opens JVM packages hidden by --release Jan 23, 2024
@He-Pin
Copy link
Contributor

He-Pin commented Jan 23, 2024

@mdedetrich ping, pekko/akka can make use of it in future

@mdedetrich
Copy link
Contributor

Yup, it has to be released in Scala 2.12/2.13/3.3.x for us to use it though

@som-snytt
Copy link
Contributor Author

That could happen! I might need a reminder...

@mdedetrich
Copy link
Contributor

That could happen! I might need a reminder...

Ive been told I'm good at that, to the detriment of societies sanity

@SethTisue
Copy link
Member

SethTisue commented Jan 24, 2024

@som-snytt what is the relationship between this new flag and --add-opens? I'm asking for my own understanding, and also because it could affect the naming, and also because it could affect what we put in the PR description

What does the current workaround look like? Could someone point me to the relevant section of the Akka build or the Pekko build?

I think -Yrelease might be an okay name, but I'd like to understand this whole thing better.

@som-snytt
Copy link
Contributor Author

@SethTisue this hasn't to do with modules, but with the --release mechanism, where the available platform API is defined by the ct.sym (and not by rt.jar). This just asks for a "backup" jrt class path that allows only the given package for look-up.

Practically, it's to support access to sun.misc.Unsafe, which is not a platform module but unsupported stuff. There is a JDK ticket to come up with replacements, but that is not yet the case even for projects willing to migrate.

Note to self: does a.b.c also allow classes of a and a.b?

@SethTisue SethTisue changed the title -Yrelease opens JVM packages hidden by --release -Yrelease allows access to JVM packages hidden by --release Jan 24, 2024
@SethTisue SethTisue changed the title -Yrelease allows access to JVM packages hidden by --release -Yrelease supplements --release, allowing access to additional JVM packages Jan 24, 2024
Copy link
Member

@SethTisue SethTisue left a comment

Choose a reason for hiding this comment

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

to @lrytz for final review/merge

@SethTisue
Copy link
Member

What does the current workaround look like? Could someone point me to the relevant section of the Akka build or the Pekko build?

Still curious about this, if @mdedetrich or @He-Pin or somebody could answer.

@SethTisue SethTisue assigned lrytz and unassigned SethTisue Jan 24, 2024
@SethTisue SethTisue changed the title -Yrelease supplements --release, allowing access to additional JVM packages -Yrelease supplements --release, allows access to additional JVM packages Jan 24, 2024
@He-Pin
Copy link
Contributor

He-Pin commented Jan 24, 2024

@SethTisue This code origin from Akka too, some kind of this.
https://github.com/apache/incubator-pekko/blob/main/project/JdkOptions.scala#L69

@SethTisue
Copy link
Member

Thanks @He-Pin — though I see now we already had this same exchange over at scala/bug#12643 😆

Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

Thank you! This is a nice improvement.

@lrytz lrytz merged commit 8d598d1 into scala:2.13.x Jan 24, 2024
3 checks passed
@som-snytt som-snytt deleted the issue/12643-access-Unsafe branch January 24, 2024 15:15
@SethTisue SethTisue changed the title -Yrelease supplements --release, allows access to additional JVM packages -Yrelease supplements -release, allows access to additional JVM packages Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
6 participants