triplea-game / triplea

TripleA is a turn based strategy game and board game engine, similar to Axis & Allies or Risk.
https://triplea-game.org/
GNU General Public License v3.0
1.34k stars 393 forks source link

Java 9 build targets Java 8 but does not link against Java 8 runtime #2803

Closed ssoloff closed 6 years ago

ssoloff commented 6 years ago

This issue was originally raised in #2801.

Our Java 9 build targets Java 8, but is incorrectly linking against the Java 9 runtime rather than the Java 8 runtime. At a minimum, the Java 9 build must specify the -bootclasspath option to javac (or the Gradle Java plugin equivalent) to point to a Java 8 runtime.

We should also investigate using the new --release option to javac added in Java 9. It basically takes the place of -source, -target, and -bootclasspath. Gradle support for this option is unknown at this time.

It would be nice if we can accomplish this in the Travis build without having to download JRE 8 with each build. JDK 8 should already be installed on the Travis build image even when JDK 9 is selected (I believe all that does is change the default JDK used by the build, i.e. what's on the PATH).

The solution also needs to account for dev environments, as devs may choose to use Java 9 to build locally.

Because the location of the Java 8 runtime may differ between Travis and different dev's environments, the solution may require the specification of an environment variable or a Gradle project property to point to the Java 8 runtime.

For devs using Eclipse, I believe there is nothing to be done. IIRC, Eclipse always uses the ECJ compiler it ships with (which is Java 9-compliant in Oxygen). It effectively sets the -bootclasspath option based on the project's selected JRE (which should be a Java 8 JRE or JDK). Eclipse devs probably already have their workspaces configured in this way. I'm not sure what, if anything, would need to be done for IntelliJ.

DanVanAtta commented 6 years ago

Unable to repro #2801 with intellij on latest master (tried to connect to lobby, was success). We may want to check release artifacts, otherwise seems like everyone is good (?) :thinking:

ron-murhammer commented 6 years ago

@DanVanAtta It only occurs with the actual release artifacts not building/running the code from an IDE.

ssoloff commented 6 years ago

Just to clarify... It only occurs when building from Gradle with JDK 9. You should be able to reproduce it locally if you change JAVA_HOME to point to a JDK 9 install and run ./gradlew clean allPlatform. The resulting distro in _build/distributions/triplea-*-allplatforms.zip will not be able to connect to the lobby if you run it with JRE 8.

The last paragraph in the issue description attempts to explain why one doesn't observe the problem when building from Eclipse even though Eclipse uses a Java 9 compiler. Presumably, IntelliJ has a similar setup where it's setting -bootclasspath correctly as long as you associated a Java 8 JRE with the project.

In fact, I was able to reproduce the problem in Eclipse by changing the project's JRE to JRE 9 and rebuilding it. Then I changed the launch configuration to run it using JRE 8. There's probably a way to do the same thing in IntelliJ if you really want to convince yourself. :smile:

DanVanAtta commented 6 years ago

I apologize, I did not read the problem thoroughly enough.

@ssoloff @RoiEXLab when might we get the benefit of Java 9 compilation? (Are we benefitting today?)

ssoloff commented 6 years ago

Just repeating an answer from a different thread to ensure this is followed-up...

My understanding is that the Java 9 build was added to detect Java 9 migration issues early so we don't deal with them all at once. @RoiEXLab, please elaborate if I'm mistaken.

ssoloff commented 6 years ago

I looked into this a bit more...

It turns out that, as postulated above, adding --release 8 to the compiler options when building with JDK 9 produces an artifact that can be run under Java 8 without the errors documented in #2801.

However, there is one caveat. The --release flag only pulls in classes from the public API of the target JDK. Because JavaFX is considered an extension in Java 8, it is not included on the classpath when using --release 8. Therefore, the Java 9 build also needs to include a Java 8-compatible jfxrt.jar on the classpath. The only reason that is a big deal is because, AFAIK, there is no published artifact for JavaFX 8.

To prototype this, I added jfxrt.jar to my fork of the triplea-game/assets repo and download that during the build. The required changes to game-core/build.gradle are then:

diff --git a/game-core/build.gradle b/game-core/build.gradle
index b77fa20f4..2de5a1418 100644
--- a/game-core/build.gradle
+++ b/game-core/build.gradle
@@ -56,10 +56,15 @@ targetCompatibility = 1.8

 tasks.withType(JavaCompile) {
     options.compilerArgs += [ '-Xlint:all', '-Xmaxwarns', '1000' ]
-    
+
     // workaround for: https://github.com/google/error-prone/issues/780
     options.compilerArgs += [ '-Xep:ParameterName:OFF' ]
-    
+
+    // workaround for https://github.com/gradle/gradle/issues/2510
+    if (JavaVersion.current() >= JavaVersion.VERSION_1_9) {
+        options.compilerArgs += ['--release', '8']
+    }
+
     options.incremental = true
     options.encoding = 'UTF-8'
 }
@@ -99,11 +104,14 @@ dependencies {
     compile 'com.yuvimasory:orange-extensions:1.3.0'
     compile 'commons-cli:commons-cli:1.4'
     compile remoteFile('https://github.com/kirill-grouchnikov/substance/raw/f894ef784a2ac20acf13e6ed2c7c26123399787b/drop/8.0.02/substance-8.0.02.jar')
+    if (JavaVersion.current() >= JavaVersion.VERSION_1_9) {
+        // TODO: update link to point to master branch on triplea-game/assets repo
+        compile remoteFile('https://github.com/ssoloff/triplea-game-assets/raw/add-javafx-8-lib/javafx/jfxrt-1.8.0_181.jar')
+    }

     compileOnly 'org.projectlombok:lombok:1.18.0'
     runtime remoteFile('https://github.com/kirill-grouchnikov/substance/raw/f894ef784a2ac20acf13e6ed2c7c26123399787b/drop/8.0.02/trident-1.5.00.jar')

-
     testCompile 'nl.jqno.equalsverifier:equalsverifier:2.4.5'
     testCompile 'org.hamcrest:java-hamcrest:2.0.0.0'
     testCompile 'org.mockito:mockito-core:2.19.1'
@@ -170,7 +178,6 @@ task downloadAssets(group: 'release') {
     }
 }

-
 import com.install4j.gradle.Install4jTask
 task generateInstallers(type: Install4jTask, dependsOn: [shadowJar, downloadAssets], group: 'release') {
     projectFile = file('build.install4j')

@DanVanAtta @RoiEXLab @ron-murhammer Any better ideas other than storing jfxrt.jar in the assets repo?

NB: Hosting jfxrt.jar is only required until we drop support for Java 8.

NB 2: This solution should also work with JDK 10 and 11.

RoiEXLab commented 6 years ago

@ssoloff I wonder if installing openjfx and getting the jfxrt from the installation folder would already do the trick.

In any case any solution will be redundant for any version after JDK 11, because oracle hands javafx over to the open source community, effectively making JavaFX just "an independent", but yet modern UI framework of many. So in the future we're probably going to have to use it via maven anyways.

ssoloff commented 6 years ago

@RoiEXLab

I wonder if installing openjfx and getting the jfxrt from the installation folder would already do the trick.

I considered something like that, but whatever solution we employ should work from a local build, as well. It felt a bit convoluted to have to introduce something like an environment variable pointing to JDK 8 to do that.

EDIT: But not ruling that out. :smile:

So in the future we're probably going to have to use it via maven anyways.

Indeed. JavaFX 11 (early access) is already available via Maven Central.

ssoloff commented 6 years ago

Reopening, as this is a Java 9+ blocker (and we have a proposed solution).