You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "mdedetrich (via GitHub)" <gi...@apache.org> on 2023/06/18 20:45:38 UTC
[GitHub] [incubator-pekko-http] mdedetrich opened a new pull request, #187: Use release 8 scalac flag to allow building on JDK11+
mdedetrich opened a new pull request, #187:
URL: https://github.com/apache/incubator-pekko-http/pull/187
This PR is the equivalent of https://github.com/apache/incubator-pekko/pull/258 but for pekko-http. The context behind these changes are complicated (read the linked pekko PR for more detail) but long story short the `release` scalac flag for Scala 2 changed at some point. The original version of `release` flag (which the code currently in pekko-http was written for) only makes scalac produce JDK 8 bytecode but it ignores the JDK 8 runtime `rt.jar`. This is almost never what you want, so a the same flag was updated in later versions of Scala (see https://github.com/scala/scala/pull/9982) which also brings in the correct `rt.jar`, i.e. if you build a project using JDK 11 with the `-release:8` flag then not only will it produce JDK8 bytecode but it will also bring in JDK 8's `rt.jar`.
That part about bringing in the `rt.jar` 8 jar is critical because, because there was a change from JDK 8 to JDK9's stdlib where if you compile a codebase with JDK9+, because of a newly added overloaded method that is only available in JDK9+ the produced bytecode will reference that new method rather than the JDK8 one. This is what was documented in https://github.com/apache/incubator-pekko/pull/258/files#diff-c11a10bd9e773399e4272cd1ee5dff72545afb2f8bed23f2d1bbc49d4e1fc03fL61-L66.
tl;dr Since we are using the most modern versions of scalac, we can just use `release:8` and it handles the corner cases described above. pekko-core had code in sbt to prevent building without an available JDK 8, pekko-http had the same core issue with the deprecated flag but it seems that this same check wasn't added.
To verify that this PR does in fact produce the same bytecode as if the project was compiled under JDK 8 https://github.com/lightbend-labs/jardiff was used. Its fairly easy to verify this yourself, clone incubator-pekko into a new directory (lets call it `incubator-pekko-2`) and compile it under JDK 8 using `sbt ++compile`. Then compile this branch in an `incubator-pekko` folder using JDK 11 and create a new folder called `diff-temp`. Then you can just run
```
jardiff -g ~/github/diff-temp/ ~/github/incubator-pekko-http/http-core/target/scala-2.12/ ~/github/incubator-pekko-http-2/http-core/target/scala-2.12/
```
Which creates a git repository in `diff-temp` showing the bytecode diff. Using a git diff viewer (I use Intellij) you get the following
As you can see while there are some difference its not anything of consequence (i.e. `Version` has changed but thats because the project version which is derived from the git has changed).
If we however run the jardiff tool without the changes in this PR we get the following
Notice how we now have differences in `HttpHeaderParser`/`CustomCharsetByteStringRenderer`, and if we view what the difference is we get
Which is precisely the problem talked about earlier.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on pull request #187: Use release 8 scalac flag to allow building on JDK11+
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #187:
URL: https://github.com/apache/incubator-pekko-http/pull/187#issuecomment-1607268323
> This change shouldn't have any effect.
Those changes definitely have an effect if you build with JDK 11, they allow you to build with JDK 11 and produce completely compatible JKD 8 bytecode.
It's detailed in the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on pull request #187: Use release 8 scalac flag to allow building on JDK11+
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on PR #187:
URL: https://github.com/apache/incubator-pekko-http/pull/187#issuecomment-1607264702
This change shouldn't have any effect.
`-release 8` was added before for any other JDK than JDK 8. Now it is also added for JDK 8 where it never was necessary (but was forbidden for older versions of scalac).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich merged pull request #187: Use release 8 scalac flag to allow building on JDK11+
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich merged PR #187:
URL: https://github.com/apache/incubator-pekko-http/pull/187
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #187: Use release 8 scalac flag to allow building on JDK11+
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #187:
URL: https://github.com/apache/incubator-pekko-http/pull/187#discussion_r1242049909
##########
project/Common.scala:
##########
@@ -35,10 +35,8 @@ object Common extends AutoPlugin {
"-Wconf:cat=deprecation&msg=since Akka 2\\.6\\.:s",
// tolerate deprecations from Akka HTTP 10.2.0 until Pekko 1.1.x where we clean up
"-Wconf:cat=deprecation&msg=since Akka HTTP 10\\.2\\.:s",
- "-Wconf:msg=reached max recursion depth:s") ++
- (if (isJdk8) Seq.empty
- else if (scalaBinaryVersion.value == "2.12") Seq("-target:jvm-1.8")
- else Seq("-release", "8")),
Review Comment:
Yes the flag was actually updated and improved, the previpusly linked PR provides all of the details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #187: Use release 8 scalac flag to allow building on JDK11+
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #187:
URL: https://github.com/apache/incubator-pekko-http/pull/187#discussion_r1242049909
##########
project/Common.scala:
##########
@@ -35,10 +35,8 @@ object Common extends AutoPlugin {
"-Wconf:cat=deprecation&msg=since Akka 2\\.6\\.:s",
// tolerate deprecations from Akka HTTP 10.2.0 until Pekko 1.1.x where we clean up
"-Wconf:cat=deprecation&msg=since Akka HTTP 10\\.2\\.:s",
- "-Wconf:msg=reached max recursion depth:s") ++
- (if (isJdk8) Seq.empty
- else if (scalaBinaryVersion.value == "2.12") Seq("-target:jvm-1.8")
- else Seq("-release", "8")),
Review Comment:
Yes the flag was actually updated and improved, the PR provides all of the details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] mdedetrich commented on a diff in pull request #187: Use release 8 scalac flag to allow building on JDK11+
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #187:
URL: https://github.com/apache/incubator-pekko-http/pull/187#discussion_r1242049909
##########
project/Common.scala:
##########
@@ -35,10 +35,8 @@ object Common extends AutoPlugin {
"-Wconf:cat=deprecation&msg=since Akka 2\\.6\\.:s",
// tolerate deprecations from Akka HTTP 10.2.0 until Pekko 1.1.x where we clean up
"-Wconf:cat=deprecation&msg=since Akka HTTP 10\\.2\\.:s",
- "-Wconf:msg=reached max recursion depth:s") ++
- (if (isJdk8) Seq.empty
- else if (scalaBinaryVersion.value == "2.12") Seq("-target:jvm-1.8")
- else Seq("-release", "8")),
Review Comment:
Yes the flag was actually updated and improved, the previously linked PR provides all of the details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on a diff in pull request #187: Use release 8 scalac flag to allow building on JDK11+
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on code in PR #187:
URL: https://github.com/apache/incubator-pekko-http/pull/187#discussion_r1242048045
##########
project/Common.scala:
##########
@@ -35,10 +35,8 @@ object Common extends AutoPlugin {
"-Wconf:cat=deprecation&msg=since Akka 2\\.6\\.:s",
// tolerate deprecations from Akka HTTP 10.2.0 until Pekko 1.1.x where we clean up
"-Wconf:cat=deprecation&msg=since Akka HTTP 10\\.2\\.:s",
- "-Wconf:msg=reached max recursion depth:s") ++
- (if (isJdk8) Seq.empty
- else if (scalaBinaryVersion.value == "2.12") Seq("-target:jvm-1.8")
- else Seq("-release", "8")),
Review Comment:
Here `-release 8` was added before. Are you saying that it makes a difference whether `-release:8` or `-release 8` is used?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko-http] jrudolph commented on pull request #187: Use release 8 scalac flag to allow building on JDK11+
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on PR #187:
URL: https://github.com/apache/incubator-pekko-http/pull/187#issuecomment-1607276571
I see, it only makes a difference for Scala 2.12 where this PR moves from `-target:jvm-1.8` to `-release:8` (which wasn't supported in earlier versions of 2.12).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org