se-edu/addressbook-level3

Use JavaFX 16 for Duke/AB3?

damithc opened this issue · 19 comments

@se-edu/tech-team-level1 See nus-cs2103-AY2122S1/forum#159

I suppose OK to use later JavaFX versions as the relevant files are bundled inside the JAR file?

kouyk commented

I think the warning is something like

Loading FXML document with JavaFX API of version 16 by JavaFX runtime of version 11

It is mainly caused by xmlns = "http://javafx.com/javafx/16" property, there doesn't seem to be any methods to set scene builder to use javafx11 specifically. Changing the property to version 11 seems to remove the warnings without any observable side effects, but any edits to fxml files using scene builder will overwrite those changes as well.

Browsing through https://github.com/openjdk/jfx/tree/master/doc-files doesn't seem to indicate any breaking change of concern to the course.

Thanks @kouyk for the update. What's the best way forward for students? Just ignore the warning?
Note that AB3 build.gradle specifies javafx version as 11.

What's the dependency between Java version and JavaFX version

kouyk commented

Thanks @kouyk for the update. What's the best way forward for students? Just ignore the warning?
Note that AB3 build.gradle specifies javafx version as 11.

I have two solutions in mind:

  1. Upgrade to JavaFX 16
    I would prefer upgrading JavaFX to v16 for an easy fix, although v11 is the LTS version. This is in view of Gluon releasing Scene Builder and they only have the latest version on their website, it will be difficult to get hold of the v11 binaries (other than to compile it from source), not forgetting the compatibility issues going forward with devices like the M1 Macbooks.
  2. Manually change the relevant strings
    This is better in terms of having less frequent changes since JavaFX 11 is an LTS release, we won't need to constantly keep up with the updates of JavaFX, making it much more inline with the choice of JDK 11 as well. However, this is prone to breaking since Scene Builder will always override it, making it difficult for students. It is also less than ideal to be overriding it this way given that there are some backward incompatible changes that in the rare case might be difficult to debug and identify for students.

What's the dependency between Java version and JavaFX version

From here, it is stated that

JavaFX 16 requires JDK 11 or later.

It should be safe to assume that JDK 11 will continue to be supported until its EOL.

Perhaps others can weigh in on the pros and cons.

Thanks for the detailed analysis @kouyk Very useful. For the iP, I guess we can advise students to either ignore the warning, upgrade JavaFX to 16, or manually change the generated files to JavaFX 11, whichever they prefer.

kouyk commented

For the iP, I guess we can advise students to either ignore the warning, upgrade JavaFX to 16, or manually change the generated files to JavaFX 11, whichever they prefer.

What will the direction be for AB3 though? It's certain that the same issue will surface again, are we going to take the same approach as iP?

What will the direction be for AB3 though? It's certain that the same issue will surface again, are we going to take the same approach as iP?

Good question @kouyk. We can leave students to choose -- after all, upgrading outdated dependencies is part of doing a brownfield project. But we can upgrade AB3 to to JavaFX 16 too. Although the tP repo has been released to students, we can still do a patch if we move fast.

What do you guys think?

kouyk commented

I have drafted a new PR for this, but in the process I have uncovered a downside with JavaFX 16 that introduces another warning regarding JavaFX being loaded from an unnamed module. To resolve that warning will require us to make AB3 modular and import dependencies as modules too, which is far beyond the scope of this issue. Not too sure what exactly is the best way forward.

@kouyk I think we are going to have a problem with the iP final peer evaluation. Some Mac users will not be able to test the JARs created by Windows/Linux users, right?

kouyk commented

Yes, I do think there will be issues if they are not using the x86 version of jdk running under the rosetta layer on m1 macs. More testing is needed to figure out what the compatibility matrix looks like though, is it possible that we get hold of an m1 mac or two to test and find a recommended course of action?

Issues like this are what we are concerned about right?

More testing is needed to figure out what the compatibility matrix looks like though, is it possible that we get hold of an m1 mac or two to test and find a recommended course of action?

Unlikely we can do that, certainly not in a hurry. Would asking all students to use JavaFX 16 fix this? See nus-cs2103-AY2122S1/forum#135 (comment)

kouyk commented

It might be a potential simple fix, but it is slightly uncomfortable to me to use a fix without understanding why it helps. Though based on recent issues in the forums it looks like the font issue on macs originate from javafx libraries not being verified. My hypothesis here is that the newer versions are all verified and loads, while javafx 11 might fail depending on the installation source. If that's the case then javafx 16 should help alleviate the problem and might even have made the font switch redundant.

It might be a potential simple fix, but it is slightly uncomfortable to me to use a fix without understanding why it helps. Though based on recent issues in the forums it looks like the font issue on macs originate from javafx libraries not being verified. My hypothesis here is that the newer versions are all verified and loads, while javafx 11 might fail depending on the installation source. If that's the case then javafx 16 should help alleviate the problem and might even have made the font switch redundant.

I agree. others from @se-edu/tech-team-level1 any inputs? Do you foresee any pitfalls? If we are to advise students to upgrade to JavaFX 16, we'll have to do it quickly and there are only 4 days left to the final submission deadline. So far, only 4 out of 458 students seem to be using JavaFX 16.

I did some search regarding compatibility issues with JavaFX. There is a merged PR on OpenJDK that adds support for building on M1 chip issue. This fix affects both JavaFX 16.

However, I am not really sure if this fix helps resolve the potential issue that we have on mac users not being able to run JAR files built on other OS. I think that we should do some testing to make sure it does not cause any other additional issues

Not sure if anyone here is an M1 Mac User

kouyk commented

This issue has also been resolved, see nus-cs2103-AY2122S1/forum#231 for resolution.

JavaFX 11 is ending LTS in September 2023, the next non LTS version is JavaFX 17. Not sure if there is still a need to upgrade JavaFX.

image

The main issue/different is the 'need' to modularise the application as mentioned above which is beyond the scope of the module. (However, this is the 'intended' way to implement GUI with JavaFX).

Currently #193 can be directly solved by upgrading JavaFX to >=13

Currently #193 can be directly solved by upgrading JavaFX to >=13

@Eclipse-Dominator Does JavaFX 13 show the warning about loading from an unnamed module?

Currently #193 can be directly solved by upgrading JavaFX to >=13

@Eclipse-Dominator Does JavaFX 13 show the warning about loading from an unnamed module?

Nope, the warning is introduced in JavaFX 16 and there are no important changes between ver 11 to ver 15, so it's probably why ver 12 to 15 are not LTS.

We can try upgrading to JavaFX 15, for the time being.

Upgrading to JavaFX 15 is fine I think. However, there is apparently no major difference besides the warning so it might be better to enjoy the LTS benefit of ver 17 if we can afford to ignore the warning.

I have taken the liberty to test out migrating ab3 to use modules as well as injecting modules as launch flags.

As for moving to modular:
Noticeable changes:

  • moving to modules requires bumping jackson to a higher version which causes a change in behavior when serializing object to file as by default it is now saved as global file path instead of local file path. This means unit test changes or some way to modify the default behavior which is undesirable

The alternative is to launch jar with --include-modules which also removes the warning.

However, for both cases, the main issue is that there is no good way to package a FAT JAR while keeping the modules.
e.g. when doing ./gradlew run, the warning will not appear since JavaFX modues are found in module path. However, when building to a FAT JAR, the modules are actually ignored so the warning will continue to show up.

I dont think there is any good way to create a standalone jar file that can avoid this.

The closes alternative I found is to use jlink which directly compiles it as an executable/elf/... which is not of jar format.


Besides the warning, there is no major difference between having modules or not. It's possible to suppress the logger that is responsible logging the message but its more like avoiding the issue for now.