search

getodk / briefcase

ODK Briefcase is a Java application for fetching and pushing forms and their contents. It helps make billions of data points from ODK portable. Contribute and make the world a better place! ✨💼✨
https://docs.getodk.org/briefcase-intro
Other
60 stars 154 forks source link

Issue 750 compiling with java11plus #754

Closed ggalmazor closed 5 years ago

ggalmazor commented 5 years ago

Closes #750

This PR prepares the repo to be compiled with Java 11 or greater. Moving up from Java 8 has some consequences that have to be dealt with:

What has been done to verify that this works as intended?

I've compiled Briefcase with Java11 and tested it with the following JVMs:

Vendor Java 8 Java 9 Java 10 Java 11 Java 12
Java.net (OpenJDK) N/A OK OK OK OK
Oracle OK N/A N/A OK OK

N/A: Not available from this vendor

Why is this the best possible solution? Were any other approaches considered?

Fixing a look and feel for Linux distros could be controversial, but at the end of the day they will simplify our troubleshooting efforts.

Updating the build tools could be skipped, but we will eventually have to do it anyway, so this time seems as good as any to do it.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

The biggest user-facing change is that, due to the change on the JVM used to build Briefcase, the MEDIUM datetime formatting style has changed, which will produce different date strings in exported CSV files.

Linux users should expect a different look and feel without any change in the behavior.

My tests of the AggregateAttachment.md5() method could be too simple and I might be computing incorrect hashes under other scenarios.

Does this change require updates to documentation? If so, please file an issue at https://github.com/opendatakit/docs/issues/new and include the link below.

Nope.

yanokwa commented 5 years ago

I see why you want to test the MD5, but the current test seems oddly complicated. Wouldn't testing a file with "Hello World" would be the same as what you have now? Why does it have to be a 40k file?

ggalmazor commented 5 years ago

So, I'm still getting the same SQL error. It was only visible on CLI.

Error pulling a form: java.sql.SQLException: No suitable driver found for jdbc:hsqldb:file:/tmp/ODK Briefcase Storage/forms/IPAQ/info.hsqldb/info (see the logs for more info)
ggalmazor commented 5 years ago

Now it loads the HSQLDB driver to prevent the error

ggalmazor commented 5 years ago

I see why you want to test the MD5, but the current test seems oddly complicated. Wouldn't testing a file with "Hello World" would be the same as what you have now? Why does it have to be a 40k file?

I want to use a file for that because the MD5 method is used to get hashes of files.

Regarding the 40k size, do you have any other figure in mind? Birds.xml is ~10k big. Maybe use a smaller file?

This is relevant while processing form and submission attachments, so I would expect big-ish files.

ggalmazor commented 5 years ago

It's unclear what distro of Java 9-11 you tested. Was it Oracle? OpenJDK?

Starting from this PR on, Briefcase will have to be built using Java.net (OpenJDK) Java 11.0.1 or greater as long as it's Java 11 LTS.

To test it, I've used:

ggalmazor commented 5 years ago

Can you confirm that the fragmentation of the Swing look/feel change is because of the compilation with Java 11? I want to make sure we are making a targeted change. Linux is 2-3% of user base, so I don't know if it's that big of a deal.

No. The fragmentation comes from the JVM you use to run Briefcase, not from the JDK you use to build it.

Here is a sequence of screenshots to show the changes (the changes comes from 10 to 11). All the screenshots come from a Briefcase compiled with Java.net Java 11.

image image image image image

By using the "Metal" look and feel, we get the same design on every JVM. All the important UI elements are there: image

I know there are few Linux users but this change would make me much happier. Notice how Java 11 and 12 tabs and checklist borders are hidden. It's super annoying for me :)

In other words, with this change, it looks like we won't annoy many people and it will me make my work easier and I'll be happier ;)

ggalmazor commented 5 years ago

I say we should imitate the old format until we give people a heads up.

Done!

kkrawczyk123 commented 5 years ago

I was able to build jar on java 11. I've run that build on Ubuntu, Mac and Windows with javas: 8, 9, 10 and 11. All runs have been successful, I was able to pull forms with media and instances. I've also pulled and export encrypted forms. I haven't noticed any functional problems.

yanokwa commented 5 years ago

And to be explicit about what I'm approving..

  1. I'm fine with the Linux UI change. I agree the consistency are a problem and this fixes it. Also, it's low risk given the number of users.
  2. I see now that you want the MD5 test to reflect the file sizes to roughly match the expected input, and thus the speed of execution. My gut says this isn't necessary, but I'll defer to your experience. @lognaturel can sanity check later.

My only concern is making sure that we aren't leaving folks who use new Oracle Java installs can run this PR.

ggalmazor commented 5 years ago

Can you confirm that Oracle Java 11 and 12 also work before merging? I bet a lot of folks have that.

Confirmed and updated the PR description with a table showing the JVMs I've used for future reference