-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[WIP] Add official Java 15 support #5836
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -7,8 +7,8 @@ task jpackageSanityChecks { | |||||||
description 'Interactive sanity checks on the version of the code that will be packaged' | ||||||||
|
||||||||
doLast { | ||||||||
// Enforce JDK 11 for compiling and building | ||||||||
assert JavaVersion.current().isJava11(): "JDK 11 is required" | ||||||||
// Enforce JDK 15 for compiling and building | ||||||||
assert JavaVersion.current() == JavaVersion.VERSION_15: "JDK 15 is required" | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you want to enforce v15, or also allow v15? If enforce
If also allow
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think to have the choice would be better as long as we don't support any other LTS java version. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cd2357 wrote:
I'm not sure the distinction between allowing and enforcing is really meaningful in the context of using Gradle's toolchain support. If we declare that the build should run against a given toolchain, e.g. JDK 15, that's what's going to happen, in every case, unless the developer changes the line of code in the build file making that declaration. So I'd think we want to just pin the build to JDK 15. That'll reduce the CI build matrix and associated times considerably, as it'll just be about building on JDK 15 across the different major OS / architectures. Then we just ratchet up the toolchain version every time we're ready to make the jump to JDK 16, 17, etc. And by the way, as per #5835, we're not ready for that move yet! @ripcurlx wrote:
What does "support" mean here? I mean we ship a specific JRE version in our packaged binaries, and that's it, right? Would it have any meaning to say we "support" any other JRE than the one we ship with the app? I might be missing something here... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is indeed confusing. From a birds-eye view, there are three (actually now four) independent "Java versions" we're dealing with: 1. The (local) build JDK
2. The (GitHub) build JDK
3. The (release packaging) build JDK
4. The (release packaging)
|
Map jdk15Binaries = [ |
jpackager
from earlier JDKs failed for various reasons)As far as I understand, we've been using JDK 11 pretty much everywhere (except for jpackager
and delivered JRE).
This PR, according to ripcurl's comment above, would extend the versions 1-3 above to support both JDK 11 (LTS) and JDK 15 (non-LTS).
I think this is also what ripcurl means with "as long as we don't support any other LTS java version" (next LTS is JDK 17, which AFAIK we don't support).
Update: As far as I can tell, the Gradle toolchain would at most specify the version used for 1-3 above. For releases, the jpackager
would still need to be v15, which right now also specifies the delivered JRE.
A jpackager
option exists to tell it to package a specific JRE version in the resulting binaries, but it didn't work in my last tests:
bisq/desktop/package/package.gradle
Line 76 in b5a43c7
// TODO For some reason, using "--runtime-image jdk-11" does NOT work with a v15 jpackage, but works with v14 |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -69,10 +69,10 @@ Use VirtualBox > 6.1 with following configuration: | |||||
#### For every OS | ||||||
|
||||||
* Install latest security updates | ||||||
* Install/Upgrade to latest Java 11 SDK | ||||||
* macOS (brew option): `brew upgrade openjdk@11` | ||||||
* Ubuntu (brew option): `brew upgrade java11` | ||||||
* Windows: Download latest version from https://www.oracle.com/java/technologies/javase-jdk11-downloads.html | ||||||
* Install/Upgrade to latest Java 15 SDK | ||||||
* macOS (brew option): `brew upgrade zulu15` | ||||||
* Ubuntu (brew option): `brew upgrade zulu15` | ||||||
* Windows: Download latest version from https://www.oracle.com/java/technologies/javase/jdk15-archive-downloads.html | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the plan is to start using the Zulu JDK, then the right link here is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
#### For Windows | ||||||
|
||||||
|
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 are versions specified twice, once as generic and once with a specific minor version?
This will 2x the build verification time, thus the wait time between push and "PR is mergeable".
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.
To check if the supported version at the time of configuration works and also the lastest java sub 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.
The build verification time just increased from 9 to 12 min as most of the builds run in parallel.