se-edu/addressbook-level4

Distributing AddressBook-Level4 in a JDK11+ world

pyokagan opened this issue · 37 comments

Background:

AB-4 is currently distributed as a single "Fat JAR". This JAR is packaged using the shadowjar plugin, which helps bundles dependencies with our application code. This works well when our dependencies do not contain any native libraries.

However, starting with JDK11+ JavaFX is not distributed with Oracle JDK any more. This is problematic for us because (1) JavaFX contains platform-specific native libraries, and (2) JavaFX specifically searches for its modules via the module system, so we probably can't use JARs anymore (since it will just be loaded as an unnamed module)

Problem Statement:

We need to find a distribution method for AB-4 that works for JDK11+.

Keep in mind that:

  • This method needs to be cross-platform in some way. (e.g. a "fat distribution" that works on all supported platforms, or a method that would automatically build platform-specific distributions for each supported platform)
  • The method should not require an installer. (CS2103/CS2113 requirement)
  • The method should only require end-users to have JDK11+ installed.
  • The method should be robust and easy to use -- this would be used by students in CS2103/CS2113, which is supposed to be an introduction software engineering course.

Expected deliverables:

  • A (short) report detailing research findings.
  • The final chosen method should be implemented in the AB-4 project.

Opportunities and benefits:

  • Improve your proficiency with the Java programming language and (packaging) ecosystem.
  • You may possibly get a chance to improve upstream projects.
  • Results from this project will be used to benefit students taking CS2103/CS2113.

Mentor: @pyokagan
Assigned to: @fzdy1914

@pyokagan Is it ok for the implementation to be only available for java 11(10 is not tested yet) but not work for java 9 (9.0.2 is not ok) anymore? Because Gradle has a plugin of javafx but require higher version of java.

Is it ok for the implementation to be only available for java 11(10 is not tested yet) but not work for java 9 (9.0.2 is not ok) anymore? Because Gradle has a plugin of javafx but require higher version of java.

Yes, it's expected that we'll only use this project with JDK/JRE 11 and above. (i.e. we'll bump targetCompatibility to 11 and above)

@pyokagan You have mentioned that a report of my research is needed? I currently find a way to solve the problem. The trick is to use another Main class to be the entry of our application. The reason can be referred from https://stackoverflow.com/questions/52569724/javafx-11-create-a-jar-file-with-gradle/52571719#52571719. I have tested it and it works for all three ways(gradlew run, run directly in intellij and run the jar made of shadowJar).
Things to notice is that gradle checkStyle and test tasks fails after changing to java11, will continue looking into it.

@fzdy1914

You have mentioned that a report of my research is needed?

Yes. The report is to ensure that you understand the problem space. It doesn't need to be very long, though. Something like fae8700 and 8e4d306 combined , which gives a technical overview of the problem and goes though the possible solutions one by one, along with analysis.

I currently find a way to solve the problem. The trick is to use another Main class to be the entry of our application. The reason can be referred from https://stackoverflow.com/questions/52569724/javafx-11-create-a-jar-file-with-gradle/52571719#52571719. I have tested it and it works for all three ways(gradlew run, run directly in intellij and run the jar made of shadowJar).

Does the same JAR work on all platforms we support (Windows, Mac, Linux)?

Things to notice is that gradle checkStyle and test tasks fails after changing to java11, will continue looking into it.

Yes, please do.

Something like fae8700 and 8e4d306 combined , which

I'm referring to the commit messages.

@pyokagan

Does the same JAR work on all platforms we support (Windows, Mac, Linux)?

I have tested on Windows and it works. I am not sure about Mac and Linux because I do not have such a system. But I will try to figure out a way to test the jar.

The file can be get from below:
https://drive.google.com/open?id=1p6958im4fveIKmwmPv2DtQLFa4ZA5Sej

@fzdy1914

I have tested on Windows and it works. I am not sure about Mac and Linux because I do not have such a system. But I will try to figure out a way to test the jar.

Yes, please do. I'm much more concerned about what happens when we run a JAR built on one system (e.g. Windows) on another (e.g. macOS). From what I understand JavaFX dependency artifacts are platform-dependent. If you see here you can see that there is a "linux" version, a "win" version and a "mac" version.

Update my progress:
I have tried other way to solve the problem, which is adding a module-info.java to our ab4.
However, the progress seems to be unsuccessful due to the following reasons:

  1. The classloader is unable to get the resource anymore which may because it is a module now.
  2. gradlew run fails saying that cannot find jackson.core/annotation, reason is unknown.
  3. I googled and get the result that, even the running is successful, distributing jar is still a problem.

So, I will just leave this way here and try it later.

For the testing of jar file of mac and linux, I have borrowed a mac and a ubuntu on Friday, the result can be get by then.

@fzdy1914

Update my progress:
I have tried other way to solve the problem, which is adding a module-info.java to our ab4.

OK. Not sure if you have seen the work done in #908 but it might help you.

For the testing of jar file of mac and linux, I have borrowed a mac and a ubuntu on Friday, the result can be get by then.

OK. To speed up development and testing, since support for Linux can be easily tested in a VM, you may wish to prioritize getting a JAR that is compiled on Windows to work on Linux first. Once that works, it would be more likely that a JAR that is compiled on Windows would also work on a Mac as well.

So we are only allowed to use jdk11 right? So letting them download another jar file is not OK right?

@fzdy1914

So we are only allowed to use jdk11 right? So letting them download another jar file is not OK right?

Do you mean asking the user to download JavaFX separately? If so, that's undesirable and should only be done as a last resort.

Breakthrough update:
The cross-platform should be solved.
Refering to https://stackoverflow.com/questions/52653836/maven-shade-javafx-runtime-components-are-missing/52654791#52654791
Now the jar generated in Win able to run in Linux

The jar generated in Linux is able to run in Win, too
I believe the cross-platform distributing problem are partly solved.
However, need to mention that the ./gradlew run for this program still fails for unknown reason.

However, need to mention that the ./gradlew run for this program still fails for unknown reason.

This problem is fixed by adding OS specified order of dependency

Next, I will be more focused on fixing checkSytleMain failure problem.

The problem is partly solve by this
gradle/gradle#8286

Progress update: The checkStyle failure in JDK11 are fixed.

Progress update: Gradle build is able to run correctly locally, however, the headless task seems to fail. Will looking into it.

Headless task were fixed refering to:
TestFX/TestFX#640

@pyokagan All the gradle task are able to run locally right now. However, the coverall task fails on Travis CI (Other tasks are OK either). I will keep looking into the reason of it.

A reference may solve the problem:
kt3k/coveralls-gradle-plugin#85

@pyokagan All the gradle task are able to run locally right now. However, the coverall task fails on Travis CI (Other tasks are OK either). I will keep looking into the reason of it.

Great effort! I won't be able to take a look until Thursday unfortunately.

Great effort! I won't be able to take a look until Thursday unfortunately.

I believe I am able to finish all the tasks by Thursday.

I believe I am able to finish all the tasks by Thursday.

It needs to be polished enough to be merged into master though :-)

When you are done, make sure you understand the full technical details of your approach. We'll be doing a full technical evaluation.

It seems that I am not able to configure netlify so I will leave it here.

@fzdy1914

It seems that I am not able to configure netlify so I will leave it here.

????

It's probably not a good idea to leave Netlify broken...

@fzdy1914

It seems that I am not able to configure netlify so I will leave it here.

????

It's probably not a good idea to leave Netlify broken...

I am not so sure about how the netlify build works for our repo. It seems that there is no local config file under ab4. So, I believe this may due to the build script are on the netlify. However, I am not able to configure the netlify online, so that is the problem. Can you also help looking into it?

@fzdy1914 Here are the current settings:
image

AB4 documentation has instructions on how to set up Netlify.
Perhaps you can set up Netlify for your own fork and try to troubleshoot?

@damithc Thanks for your reply, I will look into it.

@pyokagan I will rebase my pr and add some commit message to it.
A more general question is that, is it ok to upgrade the version of the plugin without using any feature of it just because it may suit jdk11 better?

In my point, I would like to answer all your question first. After that, I do a rebase for it. Is it ok for you?

@fzdy1914

A more general question is that, is it ok to upgrade the version of the plugin without using any feature of it just because it may suit jdk11 better?

Yes, it is OK. Plugin upgrades need to be done in a separate commit though, unless they are closely relevant to the commit at hand.

After that, I do a rebase for it. Is it ok for you?

Yes, authors are required to rebase their PRs as needed. You can see the contribution guide for more details.

After some thinking, I do strongly suspect that the Netlify issues might be due to it only supporting JDK 8.

From this repo, It seems that Netlify may only support JDK8.

@pyokagan The netlify issue should be fixed. I have removed the plugin entirely.

There are some minor problems and I will fix it tomorrow.

@pyokagan Is all your question being answered except for the prism.order problem? If so, I am able to start rebasing my commits.

@fzdy1914 You can (and should) start rebasing now. We treat individual commits as individual changes to be discussed independently.

As mentioned in the PR:

I understand that this PR is a work in progress/proof of concept, but it is hard to judge the quality of the approach taken if the code changes aren't at least annotated with commit messages explaining the intent of the change.

Please follow our commit organization requirements for that.

I've left some comments, as well as lots of "Why?" comments below. The answers to those questions should be explained in the commit message.

Fixed in #961