adding a gradle build system by lbergelson · Pull Request #14 · samtools/htsjdk-next-beta

lbergelson · 2018-07-18T22:04:04Z

Since I'm a proponent of using gradle over mill, here's a pr creating a stub project structure and a simple gradle build.

magicDGS

We need to setup for the very first build system a multi-module project; otherwise, we will need to move files around to accomodate to the modules once we are close to the release - and this is bad for the git history.

In addition, I believe that stub files are better if they stay in the repository, so I propose some meaningful ones to add.

magicDGS · 2018-07-19T06:17:15Z

gradle/wrapper/gradle-wrapper.properties

@@ -0,0 +1,5 @@
+distributionBase=GRADLE_USER_HOME
+distributionPath=wrapper/dists
+distributionUrl=https\://services.gradle.org/distributions/gradle-4.9-bin.zip


We need a explicit wrapper version on the build script

It's not technically necessary, but it's probably clearer to add it.

I prefer so, because it is easier to read the requires only with the gradle build script.

Version 4.10 is already here - does it break something that we need?

Oh, no, I just didn't realize 4.10 was out. Will update.

magicDGS · 2018-07-19T06:20:48Z

build.gradle

@@ -0,0 +1,12 @@
+plugins {


Add at the top the buildscript repositories. I would say to use jcenter() for the plugins...

Lets add the optional stuff when we add our first non-included.

magicDGS · 2018-07-19T06:21:14Z

build.gradle

@@ -0,0 +1,12 @@
+plugins {
+    id 'java-library'


Also jacoco, to use later

likewise, we can add when we actually get some tests :)

magicDGS · 2018-07-19T06:21:40Z

build.gradle

+sourceCompatibility = 1.8
+
+repositories {
+    mavenCentral()


Do we want also jcenter() here?

Lets add it when we hit a dependency that requires it?

I was thinking to include only jcenter as it contains all maven central, plus other libraries. Anyway, I am fine with maven central too.

magicDGS · 2018-07-19T06:27:43Z

settings.gradle

+
+rootProject.name = 'htsjdk-next-beta'
+enableFeaturePreview('IMPROVED_POM_SUPPORT')
+


We would like a multi-module project. See discusion in samtools/htsjdk#896 - this requires changes in this file and probably the build.gradle

We should decide if sub-modules are called in the form htsjdk-* or simply the package name (e.g., htsjdk-core vs. core)

That's a good idea to set it up as a multimodule project... I'm not actually certain the best way to do it though. Without some code to play with I don't think I'll get it right off the bat. Do you have a suggestion for how to stub that?

magicDGS · 2018-07-19T06:38:47Z

src/test/resources/README.md

@@ -0,0 +1 @@
+test resources go here


We can add a bunch of test files for core functionality (maybe in a large source directory tracked with git-lfs) in a different PR: a matched SAM/BAM/FASTA/VCF might be ideal. In that case, do we really need a stub file?

magicDGS · 2018-07-19T06:40:49Z

src/test/java/README.md

@@ -0,0 +1 @@
+test files go here


I would add a base test class instead as a stub here, where we can start adding common methods as they are needed. So the structure here will be adding a core/src/test/java/org/htsjdk/core as following:

/** * Base test class for HTSJDK. * * <p>All tests should implement this base class. */ public class HtsjdkBaseTest { }

magicDGS · 2018-07-19T06:44:38Z

gradlew

@@ -0,0 +1,172 @@
+#!/usr/bin/env sh


I wish that the new framework is completely multi-platform, so I would like to have the gradlew.bat also tracked in the repository. Lately, we can setup an AppVeyor run for testing the build on Windows. This will be useful for check if we are correctly setting up paths (I don't want hardcoded path separators, but rather use filesystem methods to generate them).

That's fine with me as long as you are willing to be the champion of the windows builds. I have essentially no interest in supporting windows but I have no objections.

Using java.nio and its method for path construction/resolution should be enough to support windows, and I am willing to check the builds and test them in my computer. I would like to have a complete multi-platform support to allow development and usability of the library in different contexts (e.g., multi-platform GUIs like IGV or command line java tools).

But anyway, I can setup the builds with AppVeyor and the gradle wrapper once this is in.

magicDGS · 2018-07-19T06:48:04Z

data/README.md

@@ -0,0 +1 @@
+test data goes here


Do we need a test data directory? I believe that this is better tracked in the ${module_name}/src/test/resources unless it is share by several modules.

A proper way of sharing the resources and test helpers is to create another module htsjdk-test; this also will allow to include test dependencies in the artifact (e.g., JUnit/TestNG dependencies, or HDFS/GCS to test in other filesystems).

So I think the htsjdk/gatk project structure of having test/resources directory that includes all the test data files was probably a mistake. I believe the test resources is really meant to be for resources that should be packaged into the jar for access at test runtime. This includes things like log4j.xml files and configuration files for mocks, etc.

It takes a significant amount of time to package test data into the jar, and we don't actually want it there, since we want to be referring to it by file path. In gatk we have modifications to the test packaging commands that strips out all of the test data files when packaging the test jar, but leaves in the resource files. It's crufty and causes confusion, so I would rather just have them separated.

I think that to some extend makes sense to package the data in the test resources to be able to distribute the packaged jar with all the content - also true that we should be more careful how we access the data in that case.

Another option is to set an integration-test separate for the unit test, for data-related testing (see, for example, https://www.petrikainulainen.net/programming/gradle/getting-started-with-gradle-integration-testing/)

Let's open another issue to decide this...

magicDGS · 2018-07-19T06:57:44Z

build.gradle

+
+repositories {
+    mavenCentral()
+}


We can also add some basic dependencies already: concretely, I would propose to add SLF4J as a logging system (users can choose whatever logging they want, and how to set the log level) and maybe Apache commons-compress to support different compression for files out-of-the-box (we can also create a compressor in the library for bgzip and the gzip compressor from commons-compress might be a better solution to gzip due to the bugs in the java.util.zip implementation).

If this is too controversial, we can let the dependencies for another PR (I will open an issue for the logging system anyway for initiate discussion).

Open #15 for slf4j

Adding core and sam modules. Adding a stub core/Record and sam/SamRecord to demonstrate that the multiproject dependencies are working correctly. Adding a stub test to show tests are working.

lbergelson · 2018-08-09T20:48:37Z

@magicDGS I've made it into a multiproject build with some stub classes.

You're not going to like where HtsjdkBaseTest is. We have this weird circular problem where we might want to end up publishing multiple artifacts from the same module to deal with the core test helpers.

The test framework classes are going to probably need to depend on the basic API classes, so unless we either want a circular dependency between modules, or to put the tests for the core module in a third of it, they need to be in the core module

Core   <-    CoreTestHelpers 
^              ^
 \            /
   CoreTests

I'm not quite sure what to do about it. I have a pr in gatk that splits out a separate test artifact from the same module, maybe we could do that same thing here.

magicDGS · 2018-08-10T12:24:29Z

@magicDGS I've made it into a multiproject build with some stub classes.

I've already realized when I was exploring the multi-project configuration that it might be a bit complicated (compared to maven), but it does not look that bad. Nice job!

You're not going to like where HtsjdkBaseTest is. We have this weird circular problem where we might want to end up publishing multiple artifacts from the same module to deal with the core test helpers.

You are right, I don't like the idea of puluting the main sources of the core package with the test framework, but I understand the circular problem. I read this stackoverflow discussion about it and I came up with these possible solutions:

As you suggested in Decide how to separate modules #17, we can have the API without any implementation in a module (even if it has some implementations for some classes, such as the proposal in Cigar interface #12) and the rest of implementation in another (e.g., sam/bam/vcf). I would suggest to have the htsjdk-api with the definitions, with the tests without depending in the test framework (and thus, independent of the other tests). If we assume that the core helper clases migh depend only on htsjdk-api (and not in implementations), then we have the htsjdk-test depending on htsjdk-api alone, and htsjdk-core dependening on runtime/compilation on htsjdk-api and on test-scope on htsjdk-test.
If we don't want to split api/implementation (or it cannot be disentangled from the test framework), another solution is to have htsjdk-core without tests and htsjdk-test depending on it, implementing in the main sources the framework itself and on the test sources the tests for the core functionality. This design also allows to package common test files in the main/resources to be included in downstream modules/projects using the package for testing (e.g., a canonical SAM/BAM/CRAM/VCF/etc. file for different kind of fomats).
Another solution is to treat the test jar from core as an artifact (e.g. for treat as an artifact in the multi-project system here). The test framework will live in core/src/test and it will become an artifact to re-use in the other projects. For downstream projects, the test jar can be published in maven central and added as a test dependency.

At the end, what is important about the modular design for downstream users is:

Keep out test from main sources (not to populate testing clases in their main)
Import only a subset of functionality when "heavy" dependencies are not needed (e.g., SRA/CRAM).

For the library itself, the multi-project is just a separation of concerns, and should be careful with maintenance issues:

Clear guidelines of where new tests should be implemented
How to build single artifacts and the uber-jar
How to test each module and the overall coverage

Do you think that any of the previous solutions can fit in both the dowstream and maintainers needs?

jacarey

Sorry for the late response. I'm fine with this, though it has been open long enough that gradle 4.10 is out. I think it makes sense to keep current.

lbergelson · 2018-09-04T17:04:14Z

@magicDGS Did you have further changes requested here? Are you ok pushing the decision of what to do with test files off for a future pr?
@tfenne Gave a +1 on slack.
@jacarey Do you have any comments?

I would like to get this merged soon if possible since it's hard to do much without a build system.

magicDGS

Ok, no problem to move forward and decide later about test classes. Just some minor comments, and once they are solve any of you can press the merge button.

Once it is in, I will update my PRs and branches to discuss also about code. Thanks @lbergelson!

magicDGS · 2018-09-05T05:19:55Z

.travis.yml

@@ -0,0 +1,10 @@
+language: java
+jdk:
+  - openjdk8


We should open an issue for deciding if we are going to support/test other versions of java (unrelated with the acceptance of this build, but just came in this pass)

magicDGS · 2018-09-05T05:22:16Z

build.gradle

+    apply plugin: 'java-library'
+
+    group 'org.htsjdk'
+    version '0.0.1'


Versioning with gradle will be also nice (for another issue). I am interested in this one at the moment, which looks nice also to release to github: https://github.com/ajoberstar/reckon

opening #20

magicDGS · 2018-09-05T05:23:55Z

build.gradle

+
+    group 'org.htsjdk'
+    version '0.0.1'
+    sourceCompatibility = 1.8


Should we also add explicitly targetCompatibility (https://docs.gradle.org/current/userguide/java_plugin.html#other_convention_properties), to be able to change them independently (related with my first comment in this review)?

Yes, good idea.

magicDGS · 2018-09-05T05:27:50Z

build.gradle

+
+}
+
+project(':sam') {


I prefer to have here cram, as we know almost for sure that it will be a different module (I thought that we were thinking to put sam into core, no?)

good point, done

magicDGS · 2018-09-05T05:30:07Z

core/src/main/java/org/htsjdk/core/test/HtsjdkBaseTest.java

@@ -0,0 +1,7 @@
+package org.htsjdk.core.test;


I agree to decide later how to package test-helpers, but better if at least the classes live in org.htsjdk.test instead of inside core (easier to split later and separate from the concept of core)

magicDGS · 2018-09-05T05:30:34Z

data/README.md

@@ -0,0 +1 @@
+test data goes here


Let's open another issue to decide this...

magicDGS · 2018-09-05T05:31:32Z

gradle/wrapper/gradle-wrapper.properties

@@ -0,0 +1,5 @@
+distributionBase=GRADLE_USER_HOME
+distributionPath=wrapper/dists
+distributionUrl=https\://services.gradle.org/distributions/gradle-4.9-bin.zip


Version 4.10 is already here - does it break something that we need?

magicDGS · 2018-09-05T05:32:01Z

gradlew.bat

@@ -0,0 +1,84 @@
+@if "%DEBUG%" == "" @echo off


Thanks for adding!

magicDGS · 2018-09-05T05:32:43Z

sam/src/main/java/org/htsjdk/sam/SamRecord.java

+
+import org.htsjdk.core.api.Record;
+
+public class SamRecord implements Record {


Change to RecordImpl, as this placeholder will be removed instead of implement (the same as the Record interface).

I changed it to CramStubRecord which I think gets the point across that's not intended to be a final thing.

I think that you forgot to add/commit that change!

magicDGS · 2018-09-05T05:33:14Z

settings.gradle

+rootProject.name = 'htsjdk-next-beta'
+enableFeaturePreview('IMPROVED_POM_SUPPORT')
+
+include ("core", "sam")


Here should be change too: sam -> cram

lbergelson · 2018-09-05T15:43:27Z

@jacarey Sorry, I missed your comment before some how. Thanks for taking a look.

lbergelson · 2018-09-05T16:19:19Z

Ok, merging this one!

magicDGS · 2018-09-06T05:14:41Z

🎉 (but you forgot the CramStubRecord)

lbergelson · 2018-09-06T15:11:29Z

@magicDGS Ack! You're right. I don't know how I missed that... Sorry!

lbergelson added 3 commits July 18, 2018 17:51

adding initial gradle build and wrapper

088df73

adding placeholder files to show project structure

da42329

adding .travis.yml and updating Readme with basic build commands

92cbb3b

magicDGS requested changes Jul 19, 2018

View reviewed changes

magicDGS mentioned this pull request Jul 19, 2018

Add Mill build system and HtsjdkException #13

Closed

Converting to a multi-project build.

41ec468

Adding core and sam modules. Adding a stub core/Record and sam/SamRecord to demonstrate that the multiproject dependencies are working correctly. Adding a stub test to show tests are working.

replace central with jcenter

2ceb5cd

magicDGS mentioned this pull request Aug 13, 2018

Use SLF4J logging system for the library #15

Open

jacarey approved these changes Sep 1, 2018

View reviewed changes

magicDGS approved these changes Sep 5, 2018

View reviewed changes

responding to commments

4a52d13

lbergelson merged commit 643057a into master Sep 5, 2018

lbergelson deleted the lb_add_gradle_build branch September 5, 2018 16:20

magicDGS mentioned this pull request Sep 9, 2018

Decide how to package/organize test-helper sources #23

Open


		rootProject.name = 'htsjdk-next-beta'
		enableFeaturePreview('IMPROVED_POM_SUPPORT')


		import org.htsjdk.core.api.Record;

		public class SamRecord implements Record {


		}

		project(':sam') {

Conversation

lbergelson commented Jul 18, 2018

Uh oh!

magicDGS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lbergelson commented Aug 9, 2018

Uh oh!

magicDGS commented Aug 10, 2018

Uh oh!

jacarey left a comment

Choose a reason for hiding this comment

Uh oh!

lbergelson commented Sep 4, 2018

Uh oh!

magicDGS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lbergelson Sep 5, 2018 •

edited

Loading