Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test on Java 11 #106

Merged
merged 4 commits into from
Jul 2, 2019
Merged

Test on Java 11 #106

merged 4 commits into from
Jul 2, 2019

Conversation

tomwhite
Copy link
Member

@tomwhite tomwhite commented Jun 10, 2019

This change runs all the tests on Travis using Java 11 (in addition to the existing Java 8 tests).

Copy link
Contributor

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One suggestion and a few questions for my own edification.

.travis.yml Outdated
- jdk: openjdk8
env: JAVA_RELEASE=8 JAVA_VERSION=1.8 SCALA_VERSION=2.11 NO_GCE_CHECK=true
- jdk: openjdk11
env: JAVA_RELEASE=11 JAVA_VERSION=11 SCALA_VERSION=2.12 NO_GCE_CHECK=true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can add a an env: global: block to always hav NO_GCE_CHECK=true without re-specifying it

env:
  global:
    NO_GCE_CHECK=true

You can probably combine that onto a single line but I'm never that clear about yaml syntax...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion - I'll try that.

script:
- mvn -B verify -Ddisq.test.real.world.files.dir=$TRAVIS_BUILD_DIR/gatk/src/test/resources/large -Ddisq.test.real.world.files.ref=$TRAVIS_BUILD_DIR/gatk/src/test/resources/large/human_g1k_v37.20.21.fasta -Ddisq.samtools.bin=/usr/bin/samtools -Ddisq.bcftools.bin=/usr/bin/bcftools
- mvn -B verify -Djava.release=$JAVA_RELEASE -Djava.version=$JAVA_VERSION -Dscala.version=$SCALA_VERSION -Ddisq.test.real.world.files.dir=$TRAVIS_BUILD_DIR/gatk/src/test/resources/large -Ddisq.test.real.world.files.ref=$TRAVIS_BUILD_DIR/gatk/src/test/resources/large/human_g1k_v37.20.21.fasta -Ddisq.samtools.bin=/usr/bin/samtools -Ddisq.bcftools.bin=/usr/bin/bcftools
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hate that it's sometimes 1.8 and sometimes 8, but what can you do...

@@ -30,15 +30,17 @@
</developers>

<properties>
<maven.compiler.target>1.8</maven.compiler.target>
<maven.compiler.source>1.8</maven.compiler.source>
<java.release>8</java.release>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that if you specify a property on the command line it automatically overrules the one that's included in the file?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right

<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dumb maven question: why do we need a java11 profile? to specify a different compiler plugin?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's to set the maven-compiler-plugin's release configuration property. Annoyingly this is not recognized by the Java 8 compiler, so it can only be set in the Java 11 configuration. Using a profile was the only way I could see to do this.

@tomwhite tomwhite merged commit 4bb4334 into disq-bio:master Jul 2, 2019
@tomwhite tomwhite deleted the java11 branch July 2, 2019 09:28
@heuermh heuermh added this to the 0.4.0 milestone Jul 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants