Upgrade to Java 17. #1833

cmnbroad · 2022-10-04T14:29:24Z

Upgrades to Java 17 and gradle 7.5.1. Changes the docker to use eclipse-temurin:17-jdk as a base image (??). Uses https://github.com/openjdk/nashorn (GPL-2.0 license) as a temporary replacement for the deprecated builtin Nashorn script engine until we settle on a permanent replacement.

Als fixes #1840.

lbergelson · 2022-10-05T16:55:10Z

build.gradle

+    implementation 'org.apache.logging.log4j:log4j-api:2.17.1'
+    implementation 'org.apache.logging.log4j:log4j-core:2.17.1'
+
+    // replacement for jdk nashorn engine GPL-2.0 license


nashorn is gpl with the classpath exception so it shouldn't be an issue

"
As a special exception, the copyright holders of this library give you
permission to link this library with independent modules to produce an
executable, regardless of the license terms of these independent modules,
and to copy and distribute the resulting executable under terms of your
choice, provided that you also meet, for each linked independent module,
the terms and conditions of the license of that module. An independent
module is a module which is not derived from or based on this library. If
you modify this library, you may extend this exception to your version of
the library, but you are not obligated to do so. If you do not wish to do
so, delete this exception statement from your version."

I would remove this note, alternatively amend it to specify that it's GPL with classpath exception so that there is no issue using it with non-gpl software.

lbergelson · 2022-10-05T17:02:27Z

build.gradle

+    implementation 'org.openjdk.nashorn:nashorn-core:15.4'
+
+    //NOTE: this dependency spec causes doc gen to fail on FingerPrintChecker since doc gen is not
+    // a compile time task
    compileOnly(googleNio)


Do we know what configuraton javadoc uses by default? I'm sure we can set the javadoc task to use the compile configuration if necessary.

lbergelson · 2022-10-05T17:29:41Z

build.gradle

@@ -111,8 +100,14 @@ configurations.all {
    }
 }

-sourceCompatibility = 1.8
-targetCompatibility = 1.8
+sourceCompatibility = 1.17


We should think about moving to toolchain definitions.

lbergelson · 2022-10-05T17:30:16Z

build.gradle

@@ -10,7 +8,7 @@ buildscript {

 plugins {
    id "java"


We might want to migrate to java-library instead of plain java so we support the API configuration.

lbergelson · 2022-10-05T17:31:14Z

build.gradle

+
+run {
+    jvmArgs = [
+            '--add-opens', 'java.base/java.io=ALL-UNNAMED'


What in picard is requiring this? Is there any way we can avoid it? I feel like this requirement is going to cause a ton of confusion for picard users.

MergeBamAlignmentTest - I've changed it now so its only for test tasks.

Seems fine for tests.

lbergelson · 2023-02-02T05:31:39Z

README.md

@@ -17,8 +17,7 @@ Picard is implemented using the HTSJDK Java library [HTSJDK][1] to support
 accessing file formats that are commonly used for high-throughput
 sequencing data such as [SAM][2] and [VCF][3].  

-Picard now builds and passes tests under Java 11. This should be considered to be a *Beta* feature. 
-As of version 2.0.1 (Nov. 2015) Picard requires Java 1.8 (jdk8u66). The last version to support Java 1.7 was release 1.141.
+As of version 3.0 (Nov. 2022) Picard requires Java 1.17.


We need to remember to bump this.

Wow, that is way off.

lbergelson

@cmnbroad A few minor issues plus a lot of rambling about how we can improve the gradle file in the future. Good to merge when the comments are resolved!

lbergelson · 2023-02-08T15:54:15Z

.github/workflows/tests.yml

        experimental: [false]
        run_barclay_tests: [true, false]
-#        include:
-#          - java: 17
-#            experimental: true
      fail-fast: false


This is my doing, but I noticed because of your change below, can we switch the java installation to temurin instead of `adopt?

lbergelson · 2023-02-08T15:55:56Z

build.gradle

-        throw new GradleException(
-                "Java 8 or later is required to build Picard, but ${JavaVersion.current()} was found. "
-                        + "$buildPrerequisitesMessage")
+    if (!JavaVersion.current().equals(JavaVersion.VERSION_17)) {


Much simpler now. It will be even simpler when we switch to toolchains.

lbergelson · 2023-02-08T15:56:11Z

build.gradle

@@ -68,40 +60,37 @@ ensureBuildPrerequisites(buildPrerequisitesMessage)
 final htsjdkVersion = System.getProperty('htsjdk.version', '3.0.1')
 final googleNio = 'com.google.cloud:google-cloud-nio:0.123.25'

-// Get the jdk files we need to run javaDoc. We need to use these during compile, testCompile,
-// test execution, and gatkDoc generation, but we don't want them as part of the runtime
-// classpath and we don't want to redistribute them in the uber jar.


lbergelson · 2023-02-08T15:57:47Z

build.gradle

+    implementation 'org.apache.logging.log4j:log4j-api:2.17.1'
+    implementation 'org.apache.logging.log4j:log4j-core:2.17.1'
+
+    // replacement for jdk nashorn engine GPL-2.0 license


I would remove this note, alternatively amend it to specify that it's GPL with classpath exception so that there is no issue using it with non-gpl software.

lbergelson · 2023-02-08T15:58:37Z

build.gradle

+
+run {
+    jvmArgs = [
+            '--add-opens', 'java.base/java.io=ALL-UNNAMED'


Seems fine for tests.

lbergelson · 2023-02-08T16:03:55Z

build.gradle

+                password = project.findProperty("sonatypePassword")
+            }
+        }
+        maven {


Does this work? In gatk / htsjdk we do this:

repositories { maven { credentials { username = isRelease ? project.findProperty("sonatypeUsername") : System.env.ARTIFACTORY_USERNAME password = isRelease ? project.findProperty("sonatypePassword") : System.env.ARTIFACTORY_PASSWORD } def release = "https://oss.sonatype.org/service/local/staging/deploy/maven2/" def snapshot = "https://broadinstitute.jfrog.io/broadinstitute/libs-snapshot-local/" url = isRelease ? release : snapshot } }

I'll give it a test and see what happens.

lbergelson · 2023-02-08T16:22:06Z

src/test/java/picard/util/help/DocumentationGenerationIntegrationTest.java

+
+        // Run javadoc in the current JVM with the custom doclet, and make sure it succeeds (this is a smoke test;
+        // we just want to make sure it doesn't blow up).
+        ToolProvider jdProvider = null;


Would it be cleaner to use ToolProvider.findFirst("javadoc")?

ToolProvider jdProvider = ToolProvider.findFirst("javadoc") .orElseThrow(() -> new IllegalStateException("Can't find javadoc tool")); ``` ?

Yeah, better. I need to remember to do this in GATK as well.

lbergelson · 2023-02-08T16:34:44Z

src/test/java/picard/util/help/DocumentationGenerationIntegrationTest.java

+    /**
+     * Entry point for manually running the picardDoc process on a subset of packages from within picard.
+     */
+    private static String[] docTestPackages = {


We could probably fix this issue by re-arranging some things but lets not do it now.

lbergelson · 2023-02-08T16:35:09Z

src/test/java/picard/util/help/DocumentationGenerationIntegrationTest.java

+            "picard.metrics"
+    };
+
+    // suppress deprecation warning on Java 11 since we're using deprecated javadoc APIs


This note is out of date now?

lbergelson · 2023-02-08T16:36:42Z

src/test/java/picard/util/help/DocumentationGenerationIntegrationTest.java

+        Assert. assertEquals(retCode, 0);
+
+        final File outputDir = docTestTarget.toFile();
+        final Set pathNames = new HashSet<>(Arrays.stream(outputDir.list()).toList());


Set -> Set<String>

…e 0 in the GZIP header "OS" value, but now uses -1 (OS_UNKNOWN)).

…ub actions.

lbergelson

@cmnbroad Changes look good to me.

cmnbroad force-pushed the cn_java_17 branch 2 times, most recently from c25c230 to cd7e6b7 Compare October 4, 2022 15:03

lbergelson reviewed Oct 5, 2022

View reviewed changes

cmnbroad mentioned this pull request Nov 7, 2022

testBamToBfq output changed when switching from OpenJDK11 to OpenJDK17 #1840

Closed

cmnbroad force-pushed the cn_java_17 branch from 9e8cc88 to 5305921 Compare November 7, 2022 20:56

cmnbroad force-pushed the cn_java_17 branch from 5305921 to 970f9d3 Compare January 10, 2023 14:07

lbergelson reviewed Feb 2, 2023

View reviewed changes

lbergelson force-pushed the cn_java_17 branch from 970f9d3 to 3e77667 Compare February 2, 2023 19:35

cmnbroad force-pushed the cn_java_17 branch from 3e77667 to 7223f91 Compare February 6, 2023 20:06

cmnbroad marked this pull request as ready for review February 7, 2023 17:30

lbergelson approved these changes Feb 8, 2023

View reviewed changes

cmnbroad added 17 commits February 14, 2023 08:53

Upgrade to Java 17.

8dd74ee

Java 17 source changes.

edce60f

Try Nashorn engine replacement, add --add-opens.

f4ceb1a

Update BamToBfq test data (GZIPOutputStream previously wrote the valu…

7703c6a

…e 0 in the GZIP header "OS" value, but now uses -1 (OS_UNKNOWN)).

Port doclet to Java 17 and new Barclay doclet API.

e17f16b

Temp commit to remove doclet until a Java 17 Barclay is released.

6b7c759

Upgrade docker.

fb4f259

Fix repos.

b3ce399

Move --add-opens to tet tasks only.

e026abf

Fix maven components.

cdff3cc

Restore doclet code; upgrade to Barclay 5.0.0; rebase to pick up gith…

321f7f7

…ub actions.

Remove .travis.yml, which is now obsolete.

c967e68

Restore DocumentationGenerationIntegrationTest.

96640fe

Check Java version (gradle plugin must use the correct JVM).

a5bf984

Code review comments.

eb6efd4

Update README.md Java reqs.

3f9505a

Fix publishing.

4894fdc

cmnbroad force-pushed the cn_java_17 branch from 8246203 to e99b5c9 Compare February 14, 2023 14:13

Remove redundant test directive.

ea7e900

cmnbroad force-pushed the cn_java_17 branch from e99b5c9 to ea7e900 Compare February 14, 2023 14:29

lbergelson approved these changes Feb 14, 2023

View reviewed changes

lbergelson merged commit 1944ab2 into master Feb 14, 2023

lbergelson deleted the cn_java_17 branch February 14, 2023 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Java 17. #1833

Upgrade to Java 17. #1833

cmnbroad commented Oct 4, 2022 •

edited

Loading

lbergelson Oct 5, 2022

lbergelson Feb 8, 2023

lbergelson Oct 5, 2022

lbergelson Oct 5, 2022

lbergelson Oct 5, 2022

lbergelson Oct 5, 2022

cmnbroad Oct 11, 2022

lbergelson Feb 8, 2023

lbergelson Feb 2, 2023

cmnbroad Feb 6, 2023

lbergelson left a comment

lbergelson Feb 8, 2023

cmnbroad Feb 13, 2023

lbergelson Feb 8, 2023

lbergelson Feb 8, 2023

lbergelson Feb 8, 2023

lbergelson Feb 8, 2023

lbergelson Feb 8, 2023

cmnbroad Feb 13, 2023

lbergelson Feb 8, 2023

cmnbroad Feb 13, 2023

lbergelson Feb 8, 2023

lbergelson Feb 8, 2023

cmnbroad Feb 13, 2023

lbergelson Feb 8, 2023

cmnbroad Feb 13, 2023

lbergelson left a comment

Upgrade to Java 17. #1833

Upgrade to Java 17. #1833

Conversation

cmnbroad commented Oct 4, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson left a comment

Choose a reason for hiding this comment

cmnbroad commented Oct 4, 2022 •

edited

Loading