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

[Project A][MacSilicon][Kenneth Seet] Upgrade to Java 17 #6

Conversation

itstrueitstrueitsrealitsreal

Description

This pull request includes updates to the documentation and the gradle.yml file to reflect the migration of the project to Java 17. Additionally, various JDK distributions were tested for compatibility with our codebase.

Environment Setup

  • Operating System: MacOS Sonoma 14.2.1 (23C71)
  • JDK Distributions Tested:
    • 17.0.11.fx-zulu
    • 17.0.11.fx-librca
  • IDE Used: IntelliJ IDEA 2023.2.1 (Ultimate Edition)
  • Link to JAR File: Download MacSilicon-KennethSeet.jar

Issues Encountered

Running AB3 on ARM64 Systems:

As an M1 Mac user, I encountered compatibility issues with certain Java versions. Non-supported SDKs would lead to a java.lang.UnsatisfiedLinkError indicating an 'incompatible architecture'—my machine uses ARM64, but the application required X86-64.

To resolve this, it's necessary to install an ARM64 compatible SDK with JavaFX bundled. However, constantly switching between different SDKs for various needs can be cumbersome.

During CS2103 in AY23/24 Semester 2, I discovered SDKMAN!, a UNIX command-line tool that facilitates switching between Java SDK versions. This tool proved essential as I needed Java 17 for teaching CS2030 and Java 11 for CS2103.

For my system, only the JDK versions 17.0.11.fx-zulu and 17.0.11.fx-librca were compatible.

Suggestion for Future Iterations

Considering the growing popularity of ARM64 machines, documenting the use of SDKMAN! for managing and switching Java SDKs could be beneficial for future courses.

@damithc
Copy link

damithc commented Jun 30, 2024

@itstrueitstrueitsrealitsreal Thanks for investigating the Mac ARM issue.
BTW, shouldn't there be a change to build.gradle as well? Is it possible that you didn't change build.gradle, and therefore, used Java 11 to build the JAR?

@itstrueitstrueitsrealitsreal
Copy link
Author

@damithc Thanks for pointing that out, it seems I missed that out. I have updated sourceCompatibility and targetCompatability in build.gradle to reflect this, and updated the download link to the jar file. I have also verified that I am still able to build the jar file without any additional issues.

@itstrueitstrueitsrealitsreal
Copy link
Author

itstrueitstrueitsrealitsreal commented Jun 30, 2024

In addition, I'd like to demonstrate how to see the full list of Java SDKs that SDKMAN offers for download, for others that may not be familiar with its usage.

  1. After downloading SDKMAN, enter the command sdk install java followed by pressing tab and wait for the full list of Java SDKs to be displayed on screen. It may take a few seconds, but this is what the list should look like.
Screenshot 2024-06-30 at 3 13 17 PM
  1. After selecting the desired version, press Enter and wait for the download and installation to complete. It may take around 3-5 minutes, depending on the strength of your connection. You can then choose to set it as the default version, or not.

  2. After that, if you wish to switch between versions, you can run the command sdk use followed by tab to see all the installed SDKs on your system, and choose the desired version.

  3. Similarly, if you wish to set one of the installed SDKs as a default, you can run the command sdk default followed by tab to see all the installed SDKs on your system, and choose the desired version.

For any other use cases, you can refer to SDKMAN's documentation or run the command sdk help to see a list of all the commands.

// the user (if looking at the log output) that the said warning appearing in the log
// can be ignored.

logger.warning("The warning about Unsupported JavaFX configuration below can be ignored.");
Copy link

Choose a reason for hiding this comment

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

@itstrueitstrueitsrealitsreal So, this isn't applicable anymore? As per the comment, this is related to the JavaFX version rather than the Java version.

Copy link
Author

Choose a reason for hiding this comment

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

@damithc Thanks for pointing that out, I mixed up the JavaFX version and the Java version. I'll revert this change.

Copy link

Choose a reason for hiding this comment

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

Interestingly, the JavaFX warning doesn't appear in Java 17. So, the comment could be wrong, and this warning should be removed. Worth investigating.

Copy link
Author

Choose a reason for hiding this comment

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

Looking at the JavaFX 16 release notes linked in the comment, it states that loading JavaFX classes from the classpath instead of modules will log a warning, which contradicts my testing.

This set of release notes also links to this bug report about the above mentioned warning, which aims to document this behavior. However, there are also some interesting comments from users on this issue.

One comment by Gluon co-founder, Johan Vos, summarizing this email thread on the openjfx-dev mailing list stands out:

The mail thread is very interesting. To give my summary of that thread:

  1. end users get errors (no javafx.graphics module on modulepath) that they don't understand and that seem pointless.

  2. there is however a very clear reason on why these warnings are there: JavaFX is designed to work as a set of modules (which of course does not mean that all JavaFX apps need to be modules too).

  3. At this moment, I am not aware of functionality that is broken when using the JavaFX JARs on the classpath, fuelling the assumption that the error is bogus.

  4. It is very likely though that some functionality is broken, or will be broken in the future (due to different access etc in a modular system), hence it is important we recommend users to use the JavaFX artifacts on the modulepath. It will be impossible to support the JavaFX platform jars running on the classpath, due to differences between packages/modules etc (which currently can mainly be overcome with e.g. add-opens etc).

  5. The javafx maven and gradle plugin already do this: if you use those, the javafx artifacts are on the modulepath.

  6. If you still use other tools/IDE's, or manually start JavaFX on the classpath, (a) that should be allowed, but (b) a clear warning is needed that, which explains that you might run into issues (now or later).

This issue will deal with the last point (6b).

In particular, points 4 and 5 stand out, as they highlight that despite the errors seeming unintuitive and unfriendly to beginners, future changes might break the functionalities of JavaFX if users continue using JavaFX JARs on the classpath.

Point 5 is important because it points out that the JavaFX artifacts are already on the modulepath if the JavaFX Maven and Gradle plugins are already used, which is the case for AB3.

The actual email also outlines a workaround for the no javafx.graphics module on modulepath error, which is similar to what is currently being done in AB3 in Main.java.

Taking all this into account, we can either opt to:

Keep the Warning

Keeps the code future-proof:

  • Helps protect against future JavaFX updates that might reintroduce issues if classes are not loaded as modules.
  • Aligns with community guidance and official documentation, preventing potential future problems.

Maintenance:

  • Educates users on the recommended configuration for JavaFX and the significance of module usage.
  • Provides clarity in logs, helping users quickly identify and understand the source of the warning.
  • Helps to document past design decisions, helping others maintain the codebase in the future.

Remove the Warning

Not useful for current users:

  • The current setup with JavaFX 17.0.7 does not produce the warning, indicating the issue may be resolved or the configuration is correct.
  • The JavaFX Maven and Gradle plugins already ensure compliance with best practices, making the warning redundant.

YAGNI:

  • Removing outdated comments and warnings reduces code clutter and potential confusion for new developers.
  • Simplifies logs, making it easier to spot actual issues that need attention.

Minimizes verbosity:

  • Reduces the number of warnings which may lead to confusion even though they may not hinder functionality.

I believe that the best course of action is to keep the warning, given that it is in line with the official documentation of the maintainers of JavaFX, and the removal of the warning may lead to confusion down the line as future changes in the plugins bundled with JavaFX, such as Maven and Gradle, may lead to incompatibilities in the future.

Copy link

Choose a reason for hiding this comment

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

I believe that the best course of action is to keep the warning, given that it is in line with the official documentation of the maintainers of JavaFX, and the removal of the warning may lead to confusion down the line as future changes in the plugins bundled with JavaFX, such as Maven and Gradle, may lead to incompatibilities in the future.

Thanks for the detailed investigation @itstrueitstrueitsrealitsreal
Yes, we can keep it. In that case we should probably revise the text as ... below (if any) can be ignored. (i.e., add (if any)) because the JavaFX warning might not appear in most cases.

@damithc damithc changed the title [Project A][Kenneth Seet] Upgrade to Java 17 [Project A][MacSilicon][Kenneth Seet] Upgrade to Java 17 Jul 4, 2024
@hjungwoo01
Copy link

Tested on Mac Silicon with JDK-FX 17.0.11.fx-zulu, and working.

Steps I followed:

  1. Downloaded the JAR file from the PR.
  2. Switched to JDK-FX 17.0.11.fx-zulu using SDKMAN.
  3. Ran the JAR file using java -jar MacSilicon-KennethSeet.jar.
  4. Performed a smoke test to check basic functionality.

Results:
Everything is functioning as expected.

Screenshots:
Screenshot 2024-07-04 at 3 43 45 PM

@jyue487
Copy link

jyue487 commented Jul 5, 2024

Tested on Mac Silicon with JDK-FX 17.0.11.fx-zulu, and working.

Steps and results are identical to above.

@teoks0199
Copy link

Tested on Windows 11, and working.

Steps I followed:

  1. Downloaded the JAR file from the PR.
  2. Ran the JAR file using java -jar MacSilicon-KennethSeet.jar.
  3. Performed a smoke test to check basic functionality.

image

@bennyLCK
Copy link

bennyLCK commented Jul 6, 2024

Smoke-tested on Windows 11 using Oracle OpenJDK version 17.0.10 on Windows Powershell Terminal in the same manner as @teoks0199 and ensured all functionalities were in place.

image

@baskargopinath
Copy link

Tested on Mac Silicon with JDK-FX 17.0.11.fx-zulu, and working.

Steps:
Downloaded the JAR file from the PR.
Switched to JDK-FX 17.0.11.fx-zulu
Ran the JAR file using java -jar MacSilicon-KennethSeet.jar.
Performed a smoke test to check basic functionality.
Results:
Everything is functioning as expected.

Screenshots:
image

@aureliony
Copy link

aureliony commented Jul 6, 2024

Tested on Ubuntu 22.04.4 LTS with zulu jdk 17.0.11. JAR is working fine.

Steps followed:
Downloaded the JAR file from the release.
Ran the JAR file using java -jar MacSilicon-KennethSeet.jar.
Performed a smoke test to check basic functionality.

Results:
Everything is functioning as expected.

Screenshot from 2024-07-06 19-32-52

@gongg21
Copy link

gongg21 commented Jul 7, 2024

Tested on MacSilicon with JDK-FX 17.0.11.fx-zulu, and working fine.
Switched using SDKMAN, ensure it is 17.0.11.fx-zulu, note the .fx suffix.
Add/Delete command tested, works.
Help command tested, works.
Ran JAR file using java -jar <filename>.

Screenshot 2024-07-07 at 5 24 11 PM

@Carlintyj
Copy link

Carlintyj commented Jul 7, 2024

Tested on Mac (Silicon) using JDK-FX 17.0.11.fx-zulu and working.

Downloaded the JAR release, ran the jar file using java -jar MacSilicon-KennethSeet.jar
Performed smoke test to check functionality.

Screenshot 2024-07-07 at 6 59 12 PM

@drustanyjt
Copy link

Tested on Linux (Ubuntu 22.04) using OpenJDK 17.0.11. Working.

Steps taken:

  1. Downloaded release
  2. Ran jar file using java -jar MacSilicon-KennethSeet.jar
  3. Tested with basic commands

image

@ryanozx
Copy link

ryanozx commented Jul 8, 2024

Tested on MacSilicon (Sonoma 14.5) using JDK 17.0.11.fx-zulu and working.

Steps taken:

  1. Downloaded release
  2. Ran jar file using java -jar MacSilicon-KennethSeet.jar
  3. Performed smoke test to check basic functionality
Screenshot 2024-07-07 at 11 13 28 PM

@jedkohjk
Copy link

jedkohjk commented Jul 8, 2024

Tested on Windows 11 and working.

Steps I followed:

  1. Downloaded the JAR file from the release.
  2. Ran java -jar MacSilicon-KennethSeet.jar.
  3. Tested some basic commands.

Screenshot

@@ -35,7 +35,7 @@ public static void main(String[] args) {
// the user (if looking at the log output) that the said warning appearing in the log
// can be ignored.

logger.warning("The warning about Unsupported JavaFX configuration below can be ignored.");
logger.warning("The warning about Unsupported JavaFX configuration below (if any) can be ignored.");
Copy link

Choose a reason for hiding this comment

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

@itstrueitstrueitsrealitsreal Can you send a separate PR for this?

@damithc
Copy link

damithc commented Jul 11, 2024

Closing, as the migration was done in fe805d8

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.