-
Notifications
You must be signed in to change notification settings - Fork 49
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
Add support for the JVM release option for scalac and javac #170
Conversation
I think this closes #79 :) |
settings/src/main/scala/org/typelevel/sbt/TypelevelSettingsPlugin.scala
Outdated
Show resolved
Hide resolved
settings/src/main/scala/org/typelevel/sbt/TypelevelSettingsPlugin.scala
Outdated
Show resolved
Hide resolved
@@ -29,13 +29,17 @@ object TypelevelSettingsPlugin extends AutoPlugin { | |||
object autoImport { | |||
lazy val tlFatalWarnings = | |||
settingKey[Boolean]("Convert compiler warnings into errors (default: false)") | |||
lazy val tlJvmRelease = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, bikeshed: why "jvm" vs "jdk"?
JDK seems to be what everyone else is saying e.g. in scala/scala#6362.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, interesting, javac uses JVM.
--release <release>
Compile for a specific VM version. Supported targets: 6, 7, 8, 9, 10, 11
-target <release> Generate class files for specific VM version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to jdk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay! Thank you :)
I've been thinking about how we can get this PR in to the 0.4.x series. Since CI is running on JDK 8 anyway, I guess the point of this PR is to support projects that are published manually, from possibly newer JDKs? If that's the case, I propose we actually don't set a default value for this setting. If this setting is not set, then no flags are added; but if it is set, it works exactly like you've got it here. So for the projects release manually this can be set to 8 as needed. We can create a follow-up issue to default it to 8 when we break compat in the next minor bump. WDYT? |
@vasilmkd sorry I have another question about this. IIUC fs2 will have to set |
Nothing that's added here works for fs2 and essentially breaks it. We should put this on ice. |
Right, by ice you mean until the next breaking release, yes? Fast-forwarding to then, I'm still concerned whether this works for fs2 at all. |
It doesn't, if this ships, fs2 would have to disable all of it. |
So then maybe the setting should be an |
} | ||
|
||
import autoImport._ | ||
import TypelevelKernelPlugin.autoImport._ | ||
|
||
override def globalSettings = Seq( | ||
tlFatalWarnings := false, | ||
tlJdkRelease := Some(8), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to be too annoying, but we can make this None
and get this into series/0.4
:)
Yeah, sorry, I got distracted. |
scalacOptions ++= { | ||
val releaseOption = tlJdkRelease | ||
.value | ||
.map { release => if (isJava8) Seq.empty else Seq("-release", release.toString) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the isJava8
check for scalacOptions
?
Also, what is the failure mode if tlJdkRelease > 8
but java.version == 1.8
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If java.version == 1.8
it means we're running javac 1.8
which doesn't understand the --release
flag. scalac
somehow makes use of this and also fails compilation with
[error] '8' is not a valid choice for '-target'
[21](https://github.com/typelevel/cats-effect/runs/5215692131?check_suite_focus=true#step:12:21)
[error] '8' is not a valid choice for '-target'
[22](https://github.com/typelevel/cats-effect/runs/5215692131?check_suite_focus=true#step:12:22)
[error] bad option: '-target:8'
[23](https://github.com/typelevel/cats-effect/runs/5215692131?check_suite_focus=true#step:12:23)
[error] bad option: '-target:8'
[24](https://github.com/typelevel/cats-effect/runs/5215692131?check_suite_focus=true#step:12:24)
[error] (kernelJVM / Compile / compileIncremental) Compilation failed
[25](https://github.com/typelevel/cats-effect/runs/5215692131?check_suite_focus=true#step:12:25)
[error] (kernelJS / Compile / compileIncremental) Compilation failed
[26](https://github.com/typelevel/cats-effect/runs/5215692131?check_suite_focus=true#step:12:26)
[error] Total time: 13 s, completed Feb 16, 2022 11:57:41 AM
[27](https://github.com/typelevel/cats-effect/runs/5215692131?check_suite_focus=true#step:12:27)
[28](https://github.com/typelevel/cats-effect/runs/5215692131?check_suite_focus=true#step:12:28)
Error: Process completed with exit code 1.
from https://github.com/typelevel/cats-effect/runs/5215692131?check_suite_focus=true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, cool, that seems good. But if we don't add the flag, it won't protect us from a target JDK newer than the compile-time JDK since it basically ignores that setting, so it seems we need another check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JDK 8 cannot target a newer JDK. Am I understanding your question correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't specify anything, javac 1.8
compiles JVM 8 bytecode, 17 compiles JVM 17 bytecode, while scalac compiles JVM 8 bytecode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies if I'm not reading your code correctly. Here's my hypothetical situation:
tlJdkRelease := Some(17)
- I load my project in sbt on Java 8
- No release flags are added to
scalacOptions
, b/cjava.version == 1.8
- My project compiles, even though I asked to target JDK 17.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a project is specifically targetting 17 and the code is compilable by JDK 8, I don't know why they're targetting 17 in the first place. 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair point :)
scalac
can generate more optimized bytecode in certain situations when targeting newer JDKs.
Also, the fact that your project would fail to compile if you load it in Java 9 (but 8 would work!) is a bit odd imo :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
settings/src/main/scala/org/typelevel/sbt/TypelevelSettingsPlugin.scala
Outdated
Show resolved
Hide resolved
} | ||
case n => | ||
sys.error( | ||
s"You're using JDK $n, which is not supported by `sbt-typelevel`. Please switch to a newer JDK.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
n
is the target JDK right, not the user's runtime JDK?
s"You're using JDK $n, which is not supported by `sbt-typelevel`. Please switch to a newer JDK.") | |
s"Target JDK is $n, which is not supported by `sbt-typelevel`. Please select a JDK >= 8.") |
@vasilmkd looks like they are discussing renaming these flags in Scala 3. |
No description provided.