Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
lrytz merged 1 commit intoscala:2.13.xfromsom-snytt:issue/12643-access-Unsafe
Jan 24, 2024

Conversation

@som-snytt
Copy link
Contributor

@som-snyttsom-snytt commentedSep 14, 2023
edited by SethTisue
Loading

Legacy usage ofsun.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 usingUnsafe.

Fixesscala/bug#12643

He-Pin reacted with thumbs up emojiHe-Pin reacted with heart emoji
@scala-jenkinsscala-jenkins added this to the2.13.13 milestoneSep 14, 2023
@som-snyttsom-snyttforce-pushed theissue/12643-access-Unsafe branch fromccfc0dc to57391b0CompareSeptember 15, 2023 01:30
@som-snyttsom-snytt marked this pull request as ready for reviewSeptember 16, 2023 01:31
@SethTisueSethTisue added the release-notesworth highlighting in next release notes labelSep 19, 2023
@SethTisue
Copy link
Member

SethTisue commentedSep 19, 2023
edited
Loading

@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
ContributorAuthor

-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, ifUnsafe 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 commentedSep 19, 2023
edited
Loading

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

@lrytzlrytz 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!

@lrytzlrytzforce-pushed theissue/12643-access-Unsafe branch from57391b0 toa37a063CompareNovember 28, 2023 14:02
@som-snyttsom-snytt changed the title-Yunsafe opens packages in jrt under --release-Yopen-packages opens packages in jrt under --releaseDec 6, 2023
@som-snytt
Copy link
ContributorAuthor

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

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

@lrytzlrytzforce-pushed theissue/12643-access-Unsafe branch from6ea44b7 to47e1f65CompareDecember 7, 2023 09:59
Copy link
Member

@lrytzlrytz 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
ContributorAuthor

som-snytt commentedDec 7, 2023
edited
Loading

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

@SethTisueSethTisue removed their assignmentJan 18, 2024
@SethTisueSethTisue changed the title-Yopen-packages opens packages in jrt under --releaseAdd-Yopen-packages, for opening JVM-internal packages under--releaseJan 18, 2024
@SethTisue
Copy link
Member

SethTisue commentedJan 18, 2024
edited
Loading

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
ContributorAuthor

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

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

@som-snytt
Copy link
ContributorAuthor

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 torelease 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-snyttsom-snyttforce-pushed theissue/12643-access-Unsafe branch from47e1f65 to8f55db5CompareJanuary 23, 2024 04:19
@som-snytt
Copy link
ContributorAuthor

--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-snyttsom-snytt changed the titleAdd-Yopen-packages, for opening JVM-internal packages under--release-Yrelease opens JVM packages hidden by--releaseJan 23, 2024
@He-Pin
Copy link
Contributor

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

guoyu09403 reacted with thumbs up emoji

@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
ContributorAuthor

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

He-Pin reacted with heart emoji

@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 commentedJan 24, 2024
edited
Loading

@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
ContributorAuthor

@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 tosun.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: doesa.b.c also allow classes ofa anda.b?

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

@SethTisueSethTisue 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.

@SethTisueSethTisue assignedlrytz and unassignedSethTisueJan 24, 2024
@SethTisueSethTisue changed the title-Yrelease supplements--release, allowing access to additional JVM packages-Yrelease supplements--release, allows access to additional JVM packagesJan 24, 2024
@He-Pin
Copy link
Contributor

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

SethTisue reacted with eyes emoji

@SethTisue
Copy link
Member

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

He-Pin and BruceKellan reacted with rocket emoji

Copy link
Member

@lrytzlrytz 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.

@lrytzlrytz merged commit8d598d1 intoscala:2.13.xJan 24, 2024
@som-snyttsom-snytt deleted the issue/12643-access-Unsafe branchJanuary 24, 2024 15:15
@SethTisueSethTisue changed the title-Yrelease supplements--release, allows access to additional JVM packages-Yrelease supplements-release, allows access to additional JVM packagesFeb 26, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@lrytzlrytzlrytz approved these changes

@SethTisueSethTisueSethTisue approved these changes

Assignees

@lrytzlrytz

Labels

release-notesworth highlighting in next release notes

Projects

None yet

Milestone

2.13.13

Development

Successfully merging this pull request may close these issues.

[2.13.9] Can't referencesun. package classes with-release option

6 participants

@som-snytt@SethTisue@He-Pin@mdedetrich@lrytz@scala-jenkins

[8]ページ先頭

©2009-2025 Movatter.jp