Skip to content
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

chore: restructure 'platform' package and add 'jre' package #708

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

skaldarnar
Copy link
Member

Contains

Since we want to manage JREs for different platforms, the respective code needs to become a bit more elaborate. Therefore, introduce a new platform package with the Platform utility class and OS and Arch enums.

In addition, start working on jre-related classes to define and handle JREArtefacts to be downloaded and managed.

How to test

Ensure that the launcher is still working as before (no new features).

Outstanding before merging

Not yet ready for merging.

Since we want to manage JREs for different platforms, the respective code needs to become a bit more elaborate. Therefore, introduce a new `platform` package with the `Platform` utility class and `OS` and `Arch` enums.

In addition, start working on `jre`-related classes to define and handle `JREArtefact`s to be downloaded and managed.
Comment on lines +28 to +74
new JREArtefact(8,
new Platform(OS.LINUX, Arch.X64),
"https://github.com/adoptium/temurin8-binaries/releases/download/jdk8u392-b08/OpenJDK8U-jre_x64_linux_hotspot_8u392b08.tar.gz",
"91d31027da0d985be3549714389593d9e0da3da5057d87e3831c7c538b9a2a0f"
),
new JREArtefact(11,
new Platform(OS.LINUX, Arch.X64),
"https://github.com/adoptium/temurin11-binaries/releases/download/jdk-11.0.21%2B9/OpenJDK11U-jre_x64_linux_hotspot_11.0.21_9.tar.gz",
"156861bb901ef18759e05f6f008595220c7d1318a46758531b957b0c950ef2c3"
),
new JREArtefact(17,
new Platform(OS.LINUX, Arch.X64),
"https://github.com/adoptium/temurin17-binaries/releases/download/jdk-17.0.9%2B9.1/OpenJDK17U-jre_x64_linux_hotspot_17.0.9_9.tar.gz",
"c37f729200b572884b8f8e157852c739be728d61d9a1da0f920104876d324733"
),
// Mac
new JREArtefact(8,
new Platform(OS.MAC, Arch.X64),
"https://github.com/adoptium/temurin8-binaries/releases/download/jdk8u392-b08/OpenJDK8U-jre_x64_mac_hotspot_8u392b08.tar.gz",
"f1f15920ed299e10c789aef6274d88d45eb21b72f9a7b0d246a352107e344e6a"
),
new JREArtefact(11,
new Platform(OS.MAC, Arch.X64),
"https://github.com/adoptium/temurin11-binaries/releases/download/jdk-11.0.21%2B9/OpenJDK11U-jre_x64_mac_hotspot_11.0.21_9.tar.gz",
"43d29affe994a09de31bf2fb6f8ab6d6792ba4267b9a2feacaa1f6e042481b9b"
),
new JREArtefact(17,
new Platform(OS.MAC, Arch.X64),
"https://github.com/adoptium/temurin17-binaries/releases/download/jdk-17.0.9%2B9.1/OpenJDK17U-jre_x64_mac_hotspot_17.0.9_9.tar.gz",
"c69b37ea72136df49ce54972408803584b49b2c91b0fbc876d7125e963c7db37"
),
// Window
new JREArtefact(8,
new Platform(OS.WINDOWS, Arch.X64),
"https://github.com/adoptium/temurin8-binaries/releases/download/jdk8u392-b08/OpenJDK8U-jre_x64_mac_hotspot_8u392b08.tar.gz",
"a6b7e671cc12f9fc16db59419bda8be00da037e14aaf5d5afb78042c145b76ed"
),
new JREArtefact(11,
new Platform(OS.WINDOWS, Arch.X64),
"https://github.com/adoptium/temurin11-binaries/releases/download/jdk-11.0.21%2B9/OpenJDK11U-jre_x64_windows_hotspot_11.0.21_9.zip",
"a93d8334a85f6cbb228694346aad0353a8cb9ff3c84b5dc3221daf2c54a11e54"
),
new JREArtefact(17,
new Platform(OS.WINDOWS, Arch.X64),
"https://github.com/adoptium/temurin17-binaries/releases/download/jdk-17.0.9%2B9.1/OpenJDK17U-jre_x64_windows_hotspot_17.0.9_9.zip",
"6c491d6f8c28c6f451f08110a30348696a04b009f8c58592191046e0fab1477b"
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried for some time to be "smart" about assembling the URL, but that became so much code in the end I don't think it's worth it in a list that we will rarely update or extend.

The main issues are:

  • different format of Java version for Java 8 (e.g. 8u392-b08) and new versions (e.g. 17.0.0+9.1)
  • different ways this version is encoded in the URL
    • for Java 8 it's jdk8u392-b08 for the jdk path element and 8u392b08 in the artifact version
    • for Java 17 its's URL encoded 17.0.9%2B9.1 in the jdk path element (with full 9.1 build version) and 17.0.9_9in the artifact version (with only the major build version 9)
  • the major version is also referenced alone

So while we can write some Regex to handle that, compute the correct suffix, etc, I think that just maintaining these lists by hand is probably a better solution.

} else {
arch = platformArch;
arch = Arch.X64;
//TODO: throw new UnsupportedPlatformException();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Little hack for me to be able to develop on M1 Mac 😕 With this in place, the static platform initialisation will throw an exception and I cannot even run the tests.

I think this is kind of nice, because we can detect whether running the launcher would have make any sense on that platform or fail early on, but it is a bit annoying for development.

The brain split situation here is that the launcher can run on more platforms than the actual game. Maybe I'll add a "development mode" to be able to start the launcher on your own risk 🤔

Comment on lines +101 to +109
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;

Platform platform = (Platform) o;

if (os != platform.os) return false;
return arch == platform.arch;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, we only checked whether we are on a specific platform and had no means to create new instances of this class. Now that we want to manage the JRE for multiple platforms we have to open this up a bit.

Comment on lines +17 to +25
@ParameterizedTest(name = "JRE in version {1} available for platform {0}")
@MethodSource("provideSupportedPlatforms")
void testJresForAllSupportedPlatforms(Platform platform, Integer javaVersion) {
assertDoesNotThrow(() -> ManagedJRE.getJreFor(platform, javaVersion));
}

private static Stream<Arguments> provideSupportedPlatforms() {
return Platform.SUPPORTED_PLATFORMS.stream().flatMap(p -> ManagedJRE.SUPPORTED_JAVA_VERSIONS.stream().map(v -> Arguments.of(p, v)));
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First round of tests to ensure that for all the supported platforms and java versions a respective entry is added to ManagedJRE.supportedJavaVersions.

I don't think we should try to download all of these in a regular test run, but we should have a "pre-release test" that tries to download all of these and validate their checksums. For the start, this can be a manual task.

Comment on lines +17 to +19
public static final Set<Integer> SUPPORTED_JAVA_VERSIONS = Sets.newHashSet(8, 11, 17);

private static final Set<JREArtefact> supportedJavaVersions;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, bad naming 🤔 SUPPORTED_JAVA_VERSIONS denotes all the major Java versions we may need, while supportedJavaVersions is actually something like jreInfoForAllSupportedPlatformsAndJavaVersions - what a mouthful!

@skaldarnar skaldarnar mentioned this pull request Nov 27, 2023
1 task
@soloturn
Copy link
Contributor

soloturn commented Jul 7, 2024

@skaldarnar @BenjaminAmos this is still needed? and if yes it still has the old stuff in it?

@BenjaminAmos
Copy link
Contributor

#714 contains part of what was here but only for target platforms. JRE artifacts are not handled yet and #714 diverges from this, meaning that this pull request will likely need a re-write to base it on those changes instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants