-
Notifications
You must be signed in to change notification settings - Fork 108
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Update to Java 11 #932
Update to Java 11 #932
Conversation
jpackage requires a version of JDK13 installed with jpackage from https://jdk.java.net/jpackage/ and passed with -Pjdk13=/path/to/jdk-13 Still needs testing on Linux and Mac Remove Grgit Update PMD to 6.7.0 for Java 11 support Disable spotbugs and errorprone Disable UI tests, since they no longer work when headless and are flaky otherwise
Bump supported source version to 11
Instead of letting jpackage create the image, which would result in a Java 13 runtime
Double-click on foo.grip to open it in GRIP
Add file association support for windows
*/ | ||
open class JpackageExec : DefaultTask() { | ||
|
||
private val objectFactory = project.objects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Annotate as @Internal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the property is private
, shouldn't that be unnecessary? There's no public accessor method like there are on no-modifier (public) properties.
fileAssociations.ifPresent { propsFile -> | ||
args.addAll("--file-associations", propsFile.asFile.absolutePath) | ||
} | ||
args.addAll("--installer-type", installerType.get()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting all of this to a private function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The arg creation.
core/src/main/java/edu/wpi/grip/core/operations/composite/PublishVideoOperation.java
Outdated
Show resolved
Hide resolved
Is there any way to preserve running the UI tests with Travis on JDK 8 if we make it so the build can compile on both platforms? |
Running UI tests on Java 8 would probably be possible - just don't add the JavaFX deps and use the appropriate versions of controlsfx and testfx. I'll have to do some digging |
Several new false positives have popped up, though
I believe that preserving UI tests is worth the extra overhead. Without them, we lose a significant percentage of our test coverage. |
Update travis and appveyor to only run checks, not do native builds
Codecov Report
@@ Coverage Diff @@
## master #932 +/- ##
=========================================
Coverage ? 52.75%
Complexity ? 1
=========================================
Files ? 327
Lines ? 8853
Branches ? 564
=========================================
Hits ? 4670
Misses ? 3980
Partials ? 203 |
@@ -75,6 +77,7 @@ private SocketHints() { /* no op */ } | |||
|
|||
/* PRIVATE PARTIALLY CONSTRUCTED BUILDERS BELOW */ | |||
|
|||
@SuppressFBWarnings(value = "UPM", justification = "False positive") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"UMP"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused Private Method. This method is clearly used elsewhere in this file, so it's a false positive. Seems that spotbugs doesn't notice usages from inner classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh. Okay then. Thanks!
Why the 14% code coverage drop? Also, it's good to see the @codecov-io bot back on this PR. |
Codecov is currently running on Azure, so UI tests aren't included in the reports |
😢 Ah well. |
Status on this? |
Currently getting UI tests and JaCoCo running on its own VM with Java 8. I think this PR should be good to merge once that's done |
Ping me when you want me to take a final look. Thanks for investing the time to keep the UI tests running. They are really valuable to have! |
Since the native packager tool was removed in Java 11, building native installers requires JDK 13 with a
jpackage
tool to be installed. Builds can be downloaded here for 64-bit Windows, Mac, and Linux. No win32 build is yet available.Updates PMD, since older versions cannot run on Java 11. Most of the changes from this PR are due to new PMD rules.
Errorprone and spotbugs are disabled due to problems with Java 11
Grgit has been removed and replaced with a manual call to
git describe --tags
to get the version number.UI tests are disabled due to TestFX not working on Java 11 when headless, and is bugged even when not headless (some tests require manual input).
Native installers can be created with
./gradlew :ui:jpackage
. Generates a.exe
for Windows,.dmg
for Mac, and.deb
for Linux inui/build/installer/
. The location of the JDK install can be specified with the propertyjdk13
, eg./gradlew :ui:jpackage -Pjdk13=/path/to/jdk13/home
File associations are available on Windows (so opening a
.grip
file will launch GRIP), but are not yet working for Mac or Linux. Updates to thejpackage
tool will hopefully let us add that functionality further down the line.Closes #890