Checking Java version on startup #3418

halirutan · 2017-11-09T04:54:55Z

This is not quite ready for review, but I appreciate some feedback. Since localizationUpdate seems broken, the messages are currently not added and I'm waiting for the fix.The logic behind the java check is as follows:

In gradle.build we can define the minRequiredJavaVersion which is added as a value to build.properties as this was the most fitting place and it makes the version check quite flexible. I did not use an upper bound for the version but a boolean allowJava9 indicator. The reasoning behind it is that a developer who is actually working on the Java 9 support just has to change this value to make JabRef start.
I put the version check as a method into BuildInfo. Other possible places would be Globals or when we give the build-info as a parameter then JavaVersionCheck itself. Suggestions?
The version check is called pretty early in main (as soon as localization is set up). I didn't bother to put the single error dialog into a separate class. Therefore, the logic of checking, showing the message and probably exiting is done directly there. Is this acceptable?

The version check itself has the only requirement to successfully access System.getProperty("java.version"). I implemented a very optimistic check that doesn't fail if we cannot determine or compare the versions. Only if we are sure that the used version is not sufficient, it will fail.

This will solve
#3310

tobiasdiez

In general the code looks good to me and I only have two smaller remarks about the code structure.

tobiasdiez · 2017-11-09T05:33:41Z

src/main/java/org/jabref/JabRefMain.java

@@ -65,6 +69,25 @@ private static void start(String[] args) {
        }

        Globals.prefs = preferences;
+
+        // Check if we are running an acceptable version of Java
+        final BuildInfo buildInfo = Globals.BUILD_INFO;


I would extract this new code at least to a new method.

A new method of JabRefMain or a different place?

In my opinion, the code can stay in JabRefMain.

tobiasdiez · 2017-11-09T05:36:54Z

src/main/java/org/jabref/logic/util/JavaVersionCheck.java

+    // Be adventurous and assume that we can access this property always!
+    private static final String JAVA_VERSION = System.getProperty("java.version");
+
+    private JavaVersionCheck() {


I would propose to make this class non-static / not a singleton. You then have a (public) constructor that accepts the current version string and a default constructor which uses JAVA_VERSION. In this way you can also easily write a test to verify the methods in this class.

Yes, good catch.

lenhard

I just ran this locally and it correctly detected that I was running it on 1.8.0_121. That's great!

However, I am not sure if terminating JabRef when the test fails is really the right thing to do. Shouldn't we just notify users that they need to upgrade and warn them that JabRef will be unstable if they don't? Maybe in the case of Java 9 terminating is fine, but otherwise I would rather allow users to work with JabRef and annoy them with the error message every time they start it.

lenhard · 2017-11-09T13:18:24Z

src/main/java/org/jabref/JabRefMain.java

+                versionError.append("\n");
+                versionError.append(Localization.lang("Note that currently, JabRef does not run with Java 9."));
+            }
+            final JFrame frame = new JFrame();


Since we are trying to move to JavaFX, would it be possible to display the error message with a FX window instead of a JFrame?

Have a look at the FXDialogService.class. there are already some predefined dialogs for fx

@Siedlerchr @lenhard That was on purpose. I was using JFrame because what good will it be if I check the Java version, try to display a message and crash with an exception because FX is not available? 😄

Can we safely assume the FXDialog will always work?

@lenhard I made JabRef quit only when Java 9 is used. Otherwise it displays the message but continues

@halirutan if no openjfx is avaiable it will crash nonetheless because the JabRef main inherits from Application (javafx.application.Application). That is the thing which you can't check for directly.
The only thing would be we use a "Pre-Main-Class" that does nit depend on javafx. But I am unsure if this would work

@Siedlerchr Ha.. overlooked this. My genius idea was to put the test at the very beginning before any of the fx stuff is instantiated. OK, I'm looking into fx dialog tonight.

halirutan · 2017-11-10T02:48:13Z

When I remember correctly, that lgtm analysis thing failed on every single PR I submitted so far..

halirutan · 2017-11-11T00:39:26Z

OK, after understanding the whole situation, I think this PR would have only one advantage: It could check if the user uses an old built of 1.8. Since JabRef doesn't even start for 1.6 or 9 or with missing FX, the Java check would fail to act on most of its target audience. The Java check needs to be run separately with a small program that runs on all JVMs.

On Skype @Siedlerchr suggested to put it in the installer. However, what happens if someone installs JabRef with an OK JVM and then upgrades Java to 9? Maybe this is acceptable but I wanted to point it out. In any case, as it stands the PR doesn't make sense. I will close this but leave the branch alive.

If someone is going to work on putting a check into the installer or in a small check program that is started prior JabRef, we have a starting point and JavaVersion doesn't need to be re-implemented completely.

tobiasdiez · 2017-11-11T00:53:03Z

I think this PR is already a huge step in the right direction. Even if the current version don't catch a missing OpenFX or Java9, it does report an error in case somebody still uses an older Java8 version like 1.8.31 that is known to cause bugs in JabRef. Thus I would like to get this PR merged.

I agree it would be also nice to handle the OpenFX & Java9 cases, but since this requires additional overhead (as you wrote, a small luncher that then starts JabRef) I don't think this will happen soon.

halirutan · 2017-11-11T01:42:31Z

@tobiasdiez Then, I could rework the Dialog to be replaced by an FX version. The question for the correct Java version we want to enforce still stands. I currently used 1.8.0_144 which is not the latest one but quite new. I additionally checked many issues to see which version reporters use. Many indeed use _152 while one guy still had _76 (or something close to that).

The big question that cannot be answered is, what version is recent enough to not present issues.. hmm..

tobiasdiez · 2017-11-11T05:07:13Z

Yes, 1.8.0_144 should be fine (at least I'm not aware of any bug that affects us but was fixed in a later java version). So please convert the dialog to JavaFX and this can be merged in my opinion.

halirutan · 2017-11-11T12:23:13Z

@Siedlerchr @tobiasdiez Quick question. If I use the FX Dialog at this point, I have to run it in the FX thread with

FXDialogService dialogService = new FXDialogService();                                           
DefaultTaskExecutor.runInJavaFXThread(                                                           
        () ->                                                                                    
        dialogService.showErrorDialogAndWait(Localization.lang("Error"), versionError.toString())
);

This, however, shows the dialog but doesn't wait for it to be closed and resumes to start JabRef. Additionally, it throws an exception

java.lang.NullPointerException: null
	at org.jabref.gui.FXDialog.setLocationRelativeToMainWindow(FXDialog.java:137) ~[JabRef-4.1-dev.jar:?]
	at org.jabref.gui.FXDialog.lambda$new$1(FXDialog.java:98) ~[JabRef-4.1-dev.jar:?]
...

since the rest of FX is not set up at this point. Any tips?

tobiasdiez · 2017-11-11T12:37:31Z

Yes, just keep the current swing dialog 😄 If it would work instantaneously, then using JavaFX is of course preferable.

Siedlerchr · 2017-11-11T14:22:04Z

Oh okay, then stick to the Swing stuff. No problem.

tobiasdiez requested changes Nov 9, 2017

View reviewed changes

lenhard suggested changes Nov 9, 2017

View reviewed changes

halirutan added 2 commits November 9, 2017 15:13

Checking Java version on startup

fb351fe

Update Localization messages

0173d89

halirutan force-pushed the feature-JavaVersionCheck branch from 23c9003 to f668fbe Compare November 10, 2017 02:09

Refactor code, make it more stable and add test

552b7dd

halirutan force-pushed the feature-JavaVersionCheck branch from f668fbe to 552b7dd Compare November 10, 2017 02:51

halirutan closed this Nov 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Checking Java version on startup #3418

Checking Java version on startup #3418

halirutan commented Nov 9, 2017

tobiasdiez left a comment

tobiasdiez Nov 9, 2017

halirutan Nov 9, 2017

tobiasdiez Nov 9, 2017

tobiasdiez Nov 9, 2017 •

edited

Loading

halirutan Nov 9, 2017

lenhard left a comment

lenhard Nov 9, 2017

Siedlerchr Nov 9, 2017

halirutan Nov 10, 2017

halirutan Nov 10, 2017

Siedlerchr Nov 10, 2017

halirutan Nov 10, 2017

halirutan commented Nov 10, 2017

halirutan commented Nov 11, 2017

tobiasdiez commented Nov 11, 2017

halirutan commented Nov 11, 2017

tobiasdiez commented Nov 11, 2017

halirutan commented Nov 11, 2017

tobiasdiez commented Nov 11, 2017

Siedlerchr commented Nov 11, 2017

Checking Java version on startup #3418

Checking Java version on startup #3418

Conversation

halirutan commented Nov 9, 2017

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasdiez Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lenhard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halirutan commented Nov 10, 2017

halirutan commented Nov 11, 2017

tobiasdiez commented Nov 11, 2017

halirutan commented Nov 11, 2017

tobiasdiez commented Nov 11, 2017

halirutan commented Nov 11, 2017

tobiasdiez commented Nov 11, 2017

Siedlerchr commented Nov 11, 2017

tobiasdiez Nov 9, 2017 •

edited

Loading