-
Notifications
You must be signed in to change notification settings - Fork 245
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
Conversation
90a264c
to
afc2367
Compare
@cmnbroad Could you take a look at this when you get a chance? It has a bunch of gradle changes because the |
Codecov Report
@@ 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
|
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.
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' |
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.
Unrelated and tangential, but this seems like ancient history. And if you look there, you see nothing about htsjdk.
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.
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") } |
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.
Is the uppercase 'H' in "Htsjdk" significant? It seems to work ok locally (it winds up in "htsjdk"ok).
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.
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"' |
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.
Wondering what triggered this change ?
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.
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 <louisb@broadinstitute.org>
afc2367
to
7cee305
Compare
Description
This is an extension of #1269 which resolves some problems with the build in gradle 5+
Checklist