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

Build and test on java 11 #1291

Merged
merged 1 commit into from
Feb 20, 2019
Merged

Build and test on java 11 #1291

merged 1 commit into from
Feb 20, 2019

Conversation

lbergelson
Copy link
Member

Description

This is an extension of #1269 which resolves some problems with the build in gradle 5+

Checklist

  • Code compiles correctly
  • New tests covering changes and new functionality
  • All tests passing
  • Extended the README / documentation, if necessary
  • Is not backward compatible (breaks binary or source compatibility)

@lbergelson
Copy link
Member Author

@cmnbroad Could you take a look at this when you get a chance? It has a bunch of gradle changes because the maven publishing plugin broke when we moved to gradle 5 and rather than figure out what was going wrong I updated it to use the maven-publish plugin instead which is the new recommended one.

@lbergelson lbergelson mentioned this pull request Feb 19, 2019
5 tasks
@codecov-io
Copy link

codecov-io commented Feb 19, 2019

Codecov Report

Merging #1291 into master will increase coverage by 0.016%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##              master     #1291       +/-   ##
===============================================
+ Coverage     67.495%   67.511%   +0.016%     
  Complexity      8150      8150               
===============================================
  Files            558       558               
  Lines          33364     33396       +32     
  Branches        5608      5632       +24     
===============================================
+ Hits           22519     22546       +27     
- Misses          8657      8666        +9     
+ Partials        2188      2184        -4
Impacted Files Coverage Δ Complexity Δ
...ain/java/htsjdk/samtools/util/MergingIterator.java 85.714% <ø> (ø) 11 <0> (ø) ⬇️
...c/main/java/htsjdk/samtools/util/SnappyLoader.java 78.571% <0%> (-2.198%) 9% <0%> (ø)
src/main/java/htsjdk/samtools/SamReader.java 81.818% <0%> (-0.94%) 0% <0%> (ø)
src/main/java/htsjdk/samtools/util/CigarUtil.java 30.303% <0%> (-0.466%) 14% <0%> (ø)
...java/htsjdk/samtools/cram/ref/ReferenceSource.java 46.364% <0%> (-0.425%) 19% <0%> (ø)
...htsjdk/tribble/readers/LongLineBufferedReader.java 29.944% <0%> (-0.342%) 15% <0%> (ø)
src/main/java/htsjdk/samtools/BamFileIoUtils.java 2.439% <0%> (-0.03%) 3% <0%> (ø)
...ain/java/htsjdk/samtools/SAMFileWriterFactory.java 67.296% <0%> (-0.012%) 50% <0%> (ø)
...riant/variantcontext/VariantContextComparator.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
...t/variantcontext/filter/GenotypeQualityFilter.java 100% <0%> (ø) 7% <0%> (ø) ⬇️
... and 14 more

Copy link
Collaborator

@cmnbroad cmnbroad left a comment

Choose a reason for hiding this comment

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

A couple of questions, but this otherwise looks good.

url 'http://broadinstitute.github.io/picard'
id = 'picard'
name = 'Picard Team'
url = 'http://broadinstitute.github.io/picard'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated and tangential, but this seems like ancient history. And if you look there, you see nothing about htsjdk.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I noticed that too but decided to leave it for later... I'll open a follow up to change it. Not sure to what though...

}

/**
* Sign non-snapshot releases with our secret key. This should never need to be invoked directly.
*/
signing {
required { isRelease && gradle.taskGraph.hasTask("uploadArchives") }
required { isRelease && gradle.taskGraph.hasTask("publishHtsjdkPublicationToMavenRepository") }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the uppercase 'H' in "Htsjdk" significant? It seems to work ok locally (it winds up in "htsjdk"ok).

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't believe it's meaningful. I think it's case insensitive but the examples on the gradle website use camel case so I did as well.

@@ -28,7 +28,7 @@ APP_NAME="Gradle"
APP_BASE_NAME=`basename "$0"`

# Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script.
DEFAULT_JVM_OPTS=""
DEFAULT_JVM_OPTS='"-Xmx64m"'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering what triggered this change ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Rebuilding the wrapper triggered it. I think they reduced gradle's memory requirement so they added a small default one.

* Changes to allow htsjdk to build and run on Java 11.
  * Fix broken javadoc that fail in 11
  * Fix String.lines conflict: see scala/bug#11125
* Changes to the build:
  * Update gradle 4.8.1 -> 5.2.1
  * Update shadow plugin 2.0.4 -> 4.0.4
  * Update scala test plugin 0.22 -> 0.23
  * Switch publishing plugin from maven -> maven-publish
* Add Java 11 to travis matrix

Co-authored-by: Louis Bergelson <[email protected]>
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.

4 participants