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

[backport] New -Yrelease option supplements --release, allows access to additional JVM packages #10671

Merged
merged 1 commit into from Jan 25, 2024

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Jan 23, 2024

backport #10543 to fix scala/bug#12643

Gah, too late for 2.12.19!*

*unless @SethTisue says otherwise of course

@scala-jenkins scala-jenkins added this to the 2.12.20 milestone Jan 23, 2024
@SethTisue SethTisue self-assigned this Jan 23, 2024
@SethTisue
Copy link
Member

I'm mildly opposed to adding more changes to 2.12.19 after we've already announced a release candidate, and I'm mildly opposed to adding features to 2.12.x at all. 2.12 is pretty much EOL at this point. It's still good to fix significant bugs and address new compatibility concerns, but entire new features seems questionable to me. The 2.13 PR says this is a "convenience intended to support a project while migrating away from using Unsafe", but people should be migrating off 2.12 altogether.

I say “mildly” because I'm open to being convinced, or overruled by Lukas.

@He-Pin
Copy link
Contributor

He-Pin commented Jan 23, 2024

感谢感谢

@som-snytt
Copy link
Contributor Author

There is no urgency, but this is for cross-building support for, e.g., pekko. But it is unleashed only with the forward port to dotty, which must also wait a release cycle.

@He-Pin
Copy link
Contributor

He-Pin commented Jan 23, 2024

I think this should be the last most wanted feature, and him already spend his precious time on it.

And this will help lightbend's commercial users too.

@SethTisue
Copy link
Member

SethTisue commented Jan 23, 2024

this will help lightbend's commercial users too

Lightbend's commercial users are on 2.13 (or 3); the company dropped support for 2.12 in the Lightbend stack a while back.

@SethTisue
Copy link
Member

SethTisue commented Jan 23, 2024

I do understand correctly that e.g. the Pekko maintainers are already able to work around this in their build, and the added feature would merely be a “convenience”.... yes?

@He-Pin
Copy link
Contributor

He-Pin commented Jan 23, 2024

this will help lightbend's commercial users too

Lightbend's commercial users are on 2.13 (or 3); the company dropped support for 2.12 in the Lightbend stack a while back.

So databicks is no?But another thing is be more friendly to community will bring more users on board , and thank you all.

@He-Pin
Copy link
Contributor

He-Pin commented Jan 23, 2024

I do understand correctly that e.g. the Pekko maintainers are already able to work around this in their build, and the added feature would merely be a “convenience”.... yes?

I think the community , especially who still use 2.12 will be greatful for both you and the team at lightbend, many users don't know how to do that, and you kindness will make their life easier, I will always say thank you, even this is been rejected, especially after I work with kotlin😀

@SethTisue
Copy link
Member

SethTisue commented Jan 23, 2024

It seems to me you are mostly making general arguments about keeping 2.12 alive. They're good arguments, and they're why we continue to do 2.12.x releases at all, and we don't plan to stop anytime soon, though 2.12.x releases continue getting both more modest and farther apart.

I think this particular feature is extremely niche and will not drive community growth, as any growth is going to come from people adopting 2.13 and/or 3, not from people keeping legacy 2.12 builds alive.

The less attention we give 2.12, the more attention we can give 2.13 and 3, where we believe the benefit to the community is greater.

I will always say thank you

Thanks 🙏

@He-Pin
Copy link
Contributor

He-Pin commented Jan 23, 2024

You are the release lead, I will follow your lead, thanks you all for making Scala better and better.

@som-snytt
Copy link
Contributor Author

For the record, this was an especially trivial backport.

--release is for cross-building, and -Yrelease is for encouraging safe cross-building when a project needs unclean API. It's anyone stuck on old stuff who need it most. It's a case of, "Neither do I condemn you."

@lrytz
Copy link
Member

lrytz commented Jan 24, 2024

but people should be migrating off 2.12 altogether

Agree, but this is for library authors (who cross-build), dropping 2.12 is probably too early for many.

The workaround is really not that great. For a project that wants to target Java 8, the options are

  • Require Java 8 in CI / local development, which is bad because it's unsupported and slow (especially on macOS where there's no arm64 release)
  • Do nothing; this can trip up contributors when they write code that uses newer JDK APIs which then fails to compile on CI / Java 8
  • Use the pekko workaround, find a Java 8 installation and use its rt.jar on the classpath

In the case of akka the use of Unsafe seems to be in a single Java file. So it turns out that our improvement doesn't actually help, unless they would move the code using Unsafe to Scala. Pekko's workaround its in the javac options, for scalac they are using -release 8.

What is interesting is that javac --release 8 prevents access to Unsafe, but --release 9 (or newer) doesn't.

➜ sandbox javac -version
javac 21.0.1

➜ sandbox javac --release 8 A.java
warning: [options] source value 8 is obsolete and will be removed in a future release
warning: [options] target value 8 is obsolete and will be removed in a future release
warning: [options] To suppress warnings about obsolete options, use -Xlint:-options.
A.java:2: error: package sun.misc does not exist
  public sun.misc.Unsafe un = null;
                 ^
1 error
3 warnings

➜ sandbox javac --release 9 A.java
A.java:2: warning: Unsafe is internal proprietary API and may be removed in a future release
  public sun.misc.Unsafe un = null;
                 ^
1 warning

It's the same when using scalac -release 8 vs scalac -release 9 (or newer). To check I unzipped ct.sym and indeed:

➜ a unzip -l /Users/luc/.jabba/jdk/adopt@21/Contents/Home/lib/ct.sym | grep Unsafe
     4682  10-17-2023 00:00   9A/jdk.unsupported/sun/misc/Unsafe.sig
     4327  10-17-2023 00:00   BCDE/jdk.unsupported/sun/misc/Unsafe.sig
     4637  10-17-2023 00:00   F/jdk.unsupported/sun/misc/Unsafe.sig
     4637  10-17-2023 00:00   G/jdk.unsupported/sun/misc/Unsafe.sig
     4439  10-17-2023 00:00   H/jdk.unsupported/sun/misc/Unsafe.sig
     4513  10-17-2023 00:00   IJK/jdk.unsupported/sun/misc/Unsafe.sig

The top folder represents the Java versions that the file is valid for, 9A means valid for 9 and 10, BCDE for 11 - 14, etc (I guess they will have to go into emojis after Java 35...). Unsafe is only missing for Java 8. https://bugs.openjdk.org/browse/JDK-8206937.

All that considered, I think our fix is still worthwhile. But it won't help pekko as they need it for javac.

@He-Pin
Copy link
Contributor

He-Pin commented Jan 24, 2024

unless they would move the code using Unsafe to Scala

@lrytz Do you mean rewrite the Unsafe.java to Unsafe.scala?

@He-Pin
Copy link
Contributor

He-Pin commented Jan 24, 2024

@mdedetrich
Copy link
Contributor

mdedetrich commented Jan 25, 2024

Regarding the supporting Scala 2.12, we (i.e. Pekko) are supporting Scala 2.12 because most of our users are using Scala 2.12. Ontop of this sbt 1.x is the current maintained/used version of sbt which happens to use Scala 2.12 so while I understand the sentiment that we would like to minimize support of Scala 2.1, at the end of the day the buck stops at the users. This is basically the same situation as JDK 8, a lot of the stakeholders have been crying/pushing etc etc to "stop using JDK 8" but for various reasons its still the lowest floor of supported JDK versions for JVM languages (I can't help that this is also somewhat ironic given that this feature is about improving a QoL issue when building against JDK 8).

Its unlikely that Pekko will drop Scala 2.12 support anytime soon, I expect it to happen at earliest when Pekko 2.0.x starts being considered and/or SBT 2.x (which uses Scala 3.3.x) starts being used, and for this reason specific features like this one would need to be in Scala 2.12/2.13 and 3.3.x for us to use it otherwise for this feature specifically we would just keep on using the workaround that we currently have. So to clarify we are not trying to dump more maintenance work on Scala 2.12 and almost all of the Scala 2.12 compatibility is something that Pekko is already handling itself, its just that for THIS feature, its not really useful or us until it gets backported and released in Scala 2.12.

@SethTisue Might be a crazy idea, but it may be worth exploring whether you want Scala 2.12 to be maintained by the community rather than only Lightbend? You could slowly transition by adding community maintainers which have a history of doing Scala 2.12 compiler work that you trust? I understand the sentiment that is coming from Lightbend's side on this, but generally this is usually the end state of such cases, i.e. community starts taking over and helping.

@lrytz
Copy link
Member

lrytz commented Jan 25, 2024

we would just keep on using the workaround that we currently have

To re-iterate: the workaround you currently have is for javac, not for scalac. But if you rewrite the Unsafe file in Scala then the fix in scalac will be useful.

Scala 2.12 to be maintained by the community

This is already the current state in terms of contributions to 2.12. We're only reviewers / gatekeepers and release managers - we certainly welcome community reviews, it just doesn't happen often (@som-snytt reviews the small minority of PRs that are not submitted by himself).

For a backport like this one, IMO we only need to discuss whether to include it in 2.12.19 or 2.12.20, I don't see a reason to reject small, non-risky backports, especially when it helps cross-building. You can take a look at unmerged PRs to 2.12.x, there's almost nothing that was rejected. The only example from the last years is #10378 - coincidentally for an issue that you were involved with (scala/bug#12765), so I can understand if that affects your perspective.

@som-snytt
Copy link
Contributor Author

Oh man, I forgot about that one. But it was truly risky. Nothing is risk-free. One day in Feb 2020, I forward-ported to dotty a small xml parser fix I submitted one day in Dec 2014. Somehow I made a dumb mistake, no idea why, it's literally the same code text except that a do-while became a while-do. That was the week before I got covid, so I don't have that excuse, IIRC. Or let me take it back, I'll try that excuse. In any case, someone has corrected that LOC now.

@mdedetrich
Copy link
Contributor

mdedetrich commented Jan 25, 2024

To re-iterate: the workaround you currently have is for javac, not for scalac. But if you rewrite the Unsafe file in Scala then the fix in scalac will be useful.

Yup I am aware of this, but thanks for pointing it out. As @He-Pin said I already translated that file to Scala due to another thing I was working on.

coincidentally for an issue that you were involved with (scala/bug#12765), so I can understand if that affects your perspective.

No hard feelings, I totally forgot about that one!

@SethTisue SethTisue modified the milestones: 2.12.20, 2.12.19 Jan 25, 2024
@SethTisue
Copy link
Member

SethTisue commented Jan 25, 2024

Sounds like @lrytz is okay with merging this. Also, additional community interest has now been expressed. So I withdraw my objection.

And since we're merging it, I would prefer it go in 2.12.19, since 2.12.19 and 2.13.13 (where this was already merged) are a coordinated pair.

I second Lukas's remarks about 2.12 maintenance. It's possible we're a bit more conservative about what we merge than hypothetical community maintainers might be, but that caution stems from our long-term commitment to the project. I think there's real value for 2.12 users in prioritizing caution and stability, this late in the 2.12 lifecycle.

Nothing is risk-free

agree

@SethTisue SethTisue changed the title [backport] -Yrelease and related ctsym/jrt tweaks [backport] -Yrelease supplements --release, allows access to additional JVM packages Jan 25, 2024
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Jan 25, 2024
@SethTisue SethTisue merged commit 6c0a9bc into scala:2.12.x Jan 25, 2024
3 checks passed
@som-snytt som-snytt deleted the backport/12643-y-release branch January 25, 2024 16:42
@mdedetrich
Copy link
Contributor

Thanks everyone for the help, the only thing is that we need to add this to Scala 3 + backport to the 3.3.x LTS series, is there an issue on this that I can track?

@SethTisue
Copy link
Member

SethTisue commented Jan 25, 2024

Hear that, @som-snytt? See what work you made for yourself? 😜

@som-snytt
Copy link
Contributor Author

som-snytt commented Jan 26, 2024

I anticipated the dotty port would be a long slog, I just didn't expect so much resistance on 2.12 where the wheels are greasier.

To answer the question, there is no ticket, and ports don't always have them. It doesn't seem to make it more likely that they'll want to close it by merging.

@SethTisue SethTisue changed the title [backport] -Yrelease supplements --release, allows access to additional JVM packages [backport] New -Yrelease option supplements --release, allows access to additional JVM packages Feb 6, 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