search

se-edu / addressbook-level4

:ab::four: Address Book sample application (Level 4)
https://se-edu.github.io/addressbook-level4
MIT License
42 stars 1.72k forks source link

Distributing AddressBook-Level4 in a JDK11+ world #951

Closed pyokagan closed 5 years ago

pyokagan commented 5 years ago

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:

Expected deliverables:

Opportunities and benefits:

Mentor: @pyokagan Assigned to: @fzdy1914

fzdy1914 commented 5 years ago

@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.

pyokagan commented 5 years ago

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)

fzdy1914 commented 5 years ago

@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.

pyokagan commented 5 years ago

@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 https://github.com/se-edu/addressbook-level4/commit/fae87009249fc5a0084ea0d4fd28d1d8bf312921 and https://github.com/se-edu/addressbook-level4/commit/8e4d306e4cc579b4e967b267a3c41aafcedbc700 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.

pyokagan commented 5 years ago

Something like fae8700 and 8e4d306 combined , which

I'm referring to the commit messages.

fzdy1914 commented 5 years ago

@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

pyokagan commented 5 years ago

@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.

fzdy1914 commented 5 years ago

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.

pyokagan commented 5 years ago

@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 https://github.com/se-edu/addressbook-level4/pull/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.

fzdy1914 commented 5 years ago

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

pyokagan commented 5 years ago

@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.

fzdy1914 commented 5 years ago

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

fzdy1914 commented 5 years ago

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.

fzdy1914 commented 5 years ago

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

fzdy1914 commented 5 years ago

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

The problem is partly solve by this https://github.com/gradle/gradle/issues/8286

fzdy1914 commented 5 years ago

Progress update: The checkStyle failure in JDK11 are fixed.

fzdy1914 commented 5 years ago

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

fzdy1914 commented 5 years ago

Headless task were fixed refering to: https://github.com/TestFX/TestFX/issues/640

fzdy1914 commented 5 years ago

@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.

fzdy1914 commented 5 years ago

A reference may solve the problem: https://github.com/kt3k/coveralls-gradle-plugin/issues/85

pyokagan commented 5 years ago

@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.

fzdy1914 commented 5 years ago

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.

pyokagan commented 5 years ago

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.

fzdy1914 commented 5 years ago

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

pyokagan commented 5 years ago

@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 commented 5 years ago

@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？

damithc commented 5 years ago

@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?

fzdy1914 commented 5 years ago

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

fzdy1914 commented 5 years ago

@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?

pyokagan commented 5 years ago

@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.

pyokagan commented 5 years ago

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

fzdy1914 commented 5 years ago

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

fzdy1914 commented 5 years ago

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

fzdy1914 commented 5 years ago

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

fzdy1914 commented 5 years ago

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

pyokagan commented 5 years ago

@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.

pyokagan commented 5 years ago

Fixed in #961