Revert default JDK to 8 and GraalVM to 22.3.0 #136
Conversation
mdedetrich
force-pushed
the
update-default-jdk-to-lts-17
branch
3 times, most recently
from
8e586af to
0e90ed8
Compare
mdedetrich
force-pushed
the
update-default-jdk-to-lts-17
branch
2 times, most recently
from
4aeeece to
009c4fe
Compare
mdedetrich
changed the title
mdedetrich
force-pushed
the
update-default-jdk-to-lts-17
branch
6 times, most recently
from
287cfa8 to
fce4a02
Compare
README.md
Outdated
| @@ -119,7 +119,7 @@ Any and all settings which affect the behavior of the generative plugin should b | |||
| - `githubWorkflowBuildPreamble` : `Seq[WorkflowStep]` – Contains a list of steps which will be inserted into the `build` job in the **ci.yml** workflow *after* setup but *before* the `sbt test` invocation. Defaults to empty. | |||
| - `githubWorkflowBuildPostamble` : `Seq[WorkflowStep]` – Similar to the `Preamble` variant, this contains a list of steps which will be added to the `build` job *after* the `sbt test` invocation but before cleanup. Defaults to empty. | |||
| - `githubWorkflowBuild` : `Seq[WorkflowStep]` – The steps which invoke sbt (or whatever else you want) to build and test your project. This defaults to just `[sbt test]`, but can be overridden to anything. For example, sbt plugin projects probably want to redefine this to be `Seq(WorkflowStep.Sbt(List("test", "scripted")))`, which would run the `test` and `scripted` sbt tasks, in order. Note that all uses of `WorkflowStep.Sbt` are compiled using the configured `githubWorkflowSbtCommand` invocation, and properly configured with respect to the build matrix-selected Scala version. | |||
| - `githubWorkflowJavaVersions` : `Seq[JavaSpec]` – A list of Java versions to be used for the build job. The publish job will use the *first* of these versions. Defaults to `JavaSpec.temurin("11")`). | |||
| - `githubWorkflowJavaVersions` : `Seq[JavaSpec]` – A list of Java versions to be used for the build job. The publish job will use the *first* of these versions. Defaults to `JavaSpec.temurin("17")`). | |||
There was a problem hiding this comment.
Sorry I wasn't clear in my issue comment. The general setup of defaulting to JDK 11 I think is ok. What I think should happen is using JDK 8 for publishing so the users can run sbt on JDK 8 with this plugin.
There was a problem hiding this comment.
Thanks for clarifying. I still think this PR is useful as we should be using the latest default LTS version by default which is 17, it also updates the GraalVM to latest version.
There was a problem hiding this comment.
I still think this PR is useful as we should be using the latest default LTS version by default which is 17
Note that not everyone feels this way.
There was a problem hiding this comment.
My counter argument to that would be that if you don't want the plugin to update to the latest stable in a newer release than you should set githubWorkflowJavaVersions statically (just like this plugin itself does when publishing as we found out) and you will never get this problem.
Updating to the latest stable LTS release is completely reasonable in my view, people also need to move away from JDK versions especially ones that aren't even supported anymore (which imho personally is the strongest argument, I have had many problems supporting ancient JDK's).
There was a problem hiding this comment.
I do think however this behaviour should be documented so I will update the README.md to signify this.
There was a problem hiding this comment.
There's a difference in what's most appropriate for applications (17) and libraries (we're now in the gray zone between 8 and 11). That's why typelevel-nix maintains two devshells.
I would very strongly recommend against this change. If you guess too low, it usually breaks at compile time. If you guess too high, it usually breaks at runtime, often in another org.
There was a problem hiding this comment.
There's a difference in what's most appropriate for applications (17) and libraries (we're now in the gray zone between 8 and 11). That's why typelevel-nix maintains two devshells.
I was talking about libraries, not applications. For libraries that are very complex then yes, setting githubWorkflowJavaVersions makes sense (and this has always been possible). For most "standard" libraries, I would actually argue its beneficial for JDK to update to latest stable release (once its been known to work with Scala, it wouldn't be updated in this plugin beforehand).
I would very strongly recommend against this change. If you guess too low, it usually breaks at compile time.
What is the issue in setting githubWorkflowJavaVersions if you have such strict requirements? If it was impossible to change this behaviour I would understand, but setting githubWorkflowJavaVersions is also a lot more clear about what your intention is, i.e. its ultra critical that my library does not update the JVM/Scala version when sbt-github-actions and this is clearly noted in sbt.
If you guess too high, it usually breaks at runtime, often in another org.
Unless I am missing something, we are talking about CI JVM version, not the JVM version that a user is running that happens to have a transitive dependency. If this is the case, it should break at runtime in your CI which to me is actually working as intended (you happened to hit that rare case where a JVM bug was hit).
There was a problem hiding this comment.
I guess the supposed scenario here is that an open source project uses this plugin without setting githubWorkflowJavaVersions and with automated publishing merges Scala Steward suggesting to bump up to then ext version (0.15.0?), and it would then publish the library at JDK 17. Since the production Java runtime is set at different companies at their own pace etc, the downstream implication is that in those companies the library would no longer be usable. One solution for that is (sbt-multi-release-jar - eed3si9n/scalacenter-proposal#1), but for now libraries tend to pick either JDK 8 or 11.
Generally, if we automate something, I think it's better for the automation to be least surprising.
There was a problem hiding this comment.
I guess the supposed scenario here is that an open source project uses this plugin without setting githubWorkflowJavaVersions and with automated publishing merges Scala Steward suggesting to bump up to then ext version (0.15.0?), and it would then publish the library at JDK 17. Since the production Java runtime is set at different companies at their own pace etc, the downstream implication is that in those companies the library would no longer be usable. One solution for that is (sbt-multi-release-jar - eed3si9n/scalacenter-proposal#1), but for now libraries tend to pick either JDK 8 or 11.
In that case would changing the default value of githubWorkflowJavaVersions to Seq(JavaSpec.temurin("8"), JavaSpec.temurin("17")) with only the second value being updated as sbt-github-actions is updated and since JavaSpec.temurin("8") is first that is the version that will be published make more sense?
There was a problem hiding this comment.
JavaSpec.temurin("8") is first that is the version that will be published make more sense?
That sounds great.
mdedetrich
force-pushed
the
update-default-jdk-to-lts-17
branch
2 times, most recently
from
f35b02f to
b49f6f2
Compare
mdedetrich
force-pushed
the
update-default-jdk-to-lts-17
branch
9 times, most recently
from
874fb4d to
3adc079
Compare
mdedetrich
force-pushed
the
update-default-jdk-to-lts-17
branch
2 times, most recently
from
fc1c72c to
0ca9db8
Compare
mdedetrich
force-pushed
the
update-default-jdk-to-lts-17
branch
3 times, most recently
from
39303b1 to
ac79cdd
Compare
|
@armanbilge @rossabaker Im pinging you on this PR because you voiced concern about changing the default JDK for publishing at #136 (comment). I have updated the PR so that Note that in that previously mentioned conversation I did say JDK 8 was the default however in fact the current default is actually JDK 11 so I am keeping that. |
mdedetrich
force-pushed
the
update-default-jdk-to-lts-17
branch
from
ac79cdd to
bf5db8d
Compare
| @@ -28,7 +28,7 @@ final case class WorkflowJob( | |||
| env: Map[String, String] = Map(), | |||
| oses: List[String] = List("ubuntu-latest"), | |||
| scalas: List[String] = List("2.13.6"), | |||
| javas: List[JavaSpec] = List(JavaSpec.temurin("11")), | |||
| javas: List[JavaSpec] = List(JavaSpec.temurin("11"), JavaSpec.temurin("17")), | |||
There was a problem hiding this comment.
So I don't think this is actually strictly neccessary? WorkflowJob is not manually used by users, instead its populated by this plugin from its various keys but I changed it to reflect the change to githubWorkflowJavaVersions.
mdedetrich
force-pushed
the
update-default-jdk-to-lts-17
branch
2 times, most recently
from
5cec24d to
91b1459
Compare
mdedetrich
changed the title
mdedetrich
force-pushed
the
update-default-jdk-to-lts-17
branch
from
91b1459 to
3051f2c
Compare
|
So after discussion with @armanbilge , I have decided to update the PR to revert the default JDK to 1.8. Even though the latest release of sbt-github-actions updated to JDK 11, this decision was quite controversial (see https://github.com/sbt/sbt-github-actions/pull/91/files#r760953600) and it caused quite a few ecosystem issues. Likewise the addition of JDK 11 and/or JDK 17 was removed, since the |
Updates default JDK to 17 and GraalVM 22.3.0. I also fixed some discrepancies in the tests where JDK versions were mixed, the only exception to this is the
"compile a job with java setup, two JVMs and two Scalas"inGenerativePluginSpecwhich is specifically testing for different JVMs.