-
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
[WIP] Add official Java 15 support #5836
Conversation
@cd2357 I think in that case we could get rid of the downloading of the JDK 15 in the binary build process? Could you create a PR against mine where we get rid of this part? |
4e4d71f
to
d0e802e
Compare
I've also extended our build setup to include multiple OSes and Java versions to recognize faster any issues on specific supported setups. |
232d6ad
to
5407f39
Compare
FYI, we could start using Gradle's built-in toolchain provisioning to ensure we're always building against the right JDK. See https://docs.gradle.org/7.3/userguide/toolchains.html for details, would be a welcome PR. |
* 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 comment
The 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
https://www.azul.com/downloads/?version=java-15-mts&os=windows&architecture=x86-64-bit&package=jdk
strategy: | ||
matrix: | ||
os: [ubuntu-latest, macOS-latest, windows-latest] | ||
java: [ '11', '11.0.3', '15', '15.0.5'] |
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.
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to enforce v15, or also allow v15?
If enforce
- this means binaries will be compiled with v15 (not just
jpackage
-d) - then we should also enforce v15 for development (i.e. not allow anything below, like v10 or v11)
If also allow
- I can look into the right syntax using Gradle toolchains, as cbeams suggested
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
@cd2357 wrote:
Do you want to enforce v15, or also allow v15?
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:
we don't support any other LTS java version
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 comment
The 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
- This is the Java version used by devs to compile, develop and test the codebase locally
- this can be anything from JDK 11 and up
- Depends on what devs have installed locally, not explicitly checked or enforced anywhere (but code only compiles with v11 and up)
2. The (GitHub) build JDK
- This is the Java version (matrix of versions) used to build on GH
- It is defined in
build.yml
L15 of this PR to be v11, 11.0.3, v15, v15.0.5 (before this PR, it was only v11) - Goal is to see if the code compiles and tests run for these versions
3. The (release packaging) build JDK
- This is the JDK available on the machine that creates the release binaries
- It is used to compile and build the release Bisq jars
- It's currently specifically defined to be JDK 11
bisq/desktop/package/package.gradle
Line 11 in b5a43c7
assert JavaVersion.current().isJava11(): "JDK 11 is required"
(changed by this PR to v15)
4. The (release packaging) jpackager
JDK
- Defined at
bisq/desktop/package/package.gradle
Line 68 in b5a43c7
Map jdk15Binaries = [ - It is used exclusively to create the platform-specific release binaries (
jpackager
from earlier JDKs failed for various reasons) - This also determines the JRE included in the OS-specific installers and binaries (currently JRE 15)
- It does not compile or build any jars, instead uses the jars previously compiled in step 3 above
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 |
Also rebasing on #5839 will likely fix the failing builds. |
5407f39
to
6c37ea6
Compare
* 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 comment
The reason will be displayed to describe this comment to others. Learn more.
* Windows: Download latest version from https://www.oracle.com/java/technologies/javase/jdk15-archive-downloads.html | |
* Windows: Download latest version from https://www.azul.com/downloads/?version=java-15-mts&os=windows&architecture=x86-64-bit&package=jdk |
Closing as superseded by #5864 |
No description provided.