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

Restore JDK 8 compatibility #888

Closed
hvesalai opened this issue Oct 31, 2023 · 12 comments · Fixed by #893
Closed

Restore JDK 8 compatibility #888

hvesalai opened this issue Oct 31, 2023 · 12 comments · Fixed by #893
Milestone

Comments

@hvesalai
Copy link
Contributor

@gnodet regarding my previous fix for emacs, the Scala folks are not happy upgrading to JDK11. Since 3.23 is the latest version supporting JDK8, would it be possible to cherry pick my fix to a 3.23.x branch and make a 3.23.1 release with it?

@gnodet
Copy link
Member

gnodet commented Oct 31, 2023

Why can't you use 3.24.0 ?
The requirements are JDK 8 at runtime and JDK 21 at build time, but I don't think you need to rebuild JLine, do you ?

@hvesalai
Copy link
Contributor Author

hvesalai commented Oct 31, 2023

@gnodet According to this comment, JLine3 3.24.0 is using a method that is not in JDK8:

scala/scala-dev#856 (comment)

Looks like JLine 3.24.0 is using the ByteBuffer limit(int newLimit) (new https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/nio/ByteBuffer.html#limit(int)) where as JDK8 has only the old Buffer limit(int newLimit)

Perhaps that was not intentional?

p.s. ByteBuffer limit(int newLimit) was added by JDK9

@gnodet
Copy link
Member

gnodet commented Oct 31, 2023

@gnodet According to this comment, JLine3 3.24.0 is using a method that is not in JDK8:

scala/scala-dev#856 (comment)

Looks like JLine 3.24.0 is using the ByteBuffer limit(int newLimit) (new https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/nio/ByteBuffer.html#limit(int)) where as JDK8 has only the old Buffer limit(int newLimit)

Perhaps that was not intentional?

p.s. ByteBuffer limit(int newLimit) was added by JDK9

Yes, that method was added as an override of Buffer.limit(int) which was present in JDK 8. While raising the build time requirement, the JDK from the new compiler uses the new override method instead of the one from Buffer. So the code has not changed and that change was clearly not intentional.

I'll try to do a 3.24.1 to restore JDK 8 compatibility asap.

@som-snytt
Copy link

This was the gotcha (which the Akka project hit) which made Scala support --release.

@gnodet
Copy link
Member

gnodet commented Oct 31, 2023

This was the gotcha (which the Akka project hit) which made Scala support --release.

Unfortunately, --release 1.8 is not supported anymore by JDK 21, which JLine requires at build time. I'll have to find another way, maybe using animal-sniffer-maven-plugin...

@gnodet gnodet changed the title JLine 3.23.1 for scala Restore JDK 8 compatibility Oct 31, 2023
@SethTisue
Copy link

SethTisue commented Oct 31, 2023

Try --release 8 instead of --release 1.8? (reference: https://stackoverflow.com/a/52419344/86485, though surely there's a better reference...)

@som-snytt
Copy link

som-snytt commented Oct 31, 2023

javac --help

  --source <release>, -source <release>
        Provide source compatibility with the specified Java SE release.
        Supported releases:
            8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21

Notably, the doc page refers you to the command line!

The supported values of release are the current Java SE release and a limited number of previous releases, detailed in the command-line help.

It is in deprecation of a sort.

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.

@gnodet
Copy link
Member

gnodet commented Oct 31, 2023

javac --help

  --source <release>, -source <release>
        Provide source compatibility with the specified Java SE release.
        Supported releases:
            8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21

Notably, the doc page refers you to the command line!

The supported values of release are the current Java SE release and a limited number of previous releases, detailed in the command-line help.

It is in deprecation of a sort.

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.

You're right. Unfortunately, it does not seem to be sufficient to detect usage of the JDK 9 methods, which is a bit weird...
I'm leaning towards using animal-sniffer...

@gnodet gnodet added this to the 3.24.1 milestone Oct 31, 2023
@SethTisue
Copy link

SethTisue commented Oct 31, 2023

Unfortunately, it does not seem to be sufficient to detect usage of the JDK 9 methods, which is a bit weird...

I think that's because the new method on ByteBuffer isn't an entirely new method, but an override that narrows the return type. So it could make sense that with --release 8 it would still compile, it's just that the compile-time return type will be Buffer rather than the more specific ByteBuffer. (But you might want to test this theory by inspecting the generated bytecode?)

@gnodet
Copy link
Member

gnodet commented Oct 31, 2023

Unfortunately, it does not seem to be sufficient to detect usage of the JDK 9 methods, which is a bit weird...

I think that's because the new method on ByteBuffer isn't an entirely new method, but an override that narrows the return type. So it could make sense that with --release 8 it would still compile, it's just that the compile-time return type will be Buffer rather than the more specific ByteBuffer. (But you might want to test this theory by inspecting the generated bytecode?)

You're right. With --release 8, the compiler is using the JDK 8 Buffer methods rather than the overrides in ByteBuffer and CharBuffer which are new in JDK 9.

@som-snytt
Copy link

som-snytt commented Oct 31, 2023

It's a weird template with inheritDoc. Suspiciously, JavaDoc does not report since 9.

https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/nio/X-Buffer.java.template#L1563

But a simple test... oh, jinx.

A Scala user did have a problem with API changing in JDK 8, they had to compile against rt.jar and ignore -release; so I'm willing to believe there are points of failure, but the simple case for limit works.

Anyway, thanks for the non-trivial fix!

@SethTisue
Copy link

thank you for the fix and the release 🙏

JLine is a huge boon to the Scala project and language

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants