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

Changing to the mill build system #48

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Changing to the mill build system #48

wants to merge 16 commits into from

Conversation

nh13
Copy link
Member

@nh13 nh13 commented May 27, 2019

Note: this isn't ready until:

@codecov-io
Copy link

codecov-io commented Jun 11, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
+ Coverage   93.43%   93.78%   +0.34%     
==========================================
  Files          23       23              
  Lines         823      820       -3     
  Branches      122       70      -52     
==========================================
  Hits          769      769              
+ Misses         54       51       -3
Impacted Files Coverage Δ
.../com/fulcrumgenomics/commons/util/StringUtil.scala 100% <ø> (ø)
...m/fulcrumgenomics/commons/io/AsyncStreamSink.scala 90.9% <ø> (ø)
...com/fulcrumgenomics/commons/collection/BiMap.scala 100% <ø> (ø)
...ommons/src/com/fulcrumgenomics/commons/io/Io.scala 90.47% <ø> (ø)
.../fulcrumgenomics/commons/async/AsyncIterator.scala 100% <ø> (ø)
...lcrumgenomics/commons/reflect/ReflectionUtil.scala 92% <ø> (ø)
...umgenomics/commons/reflect/ReflectiveBuilder.scala 90.32% <ø> (ø)
...omics/commons/collection/SelfClosingIterator.scala 100% <ø> (ø)
.../fulcrumgenomics/commons/util/NumericCounter.scala 97.5% <ø> (ø)
...ns/src/com/fulcrumgenomics/commons/io/Writer.scala 0% <ø> (ø)
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3087de3...f36be31. Read the comment docs.

build.sc Outdated

object commons extends ScalaModule with ScoverageModule with PublishModule {
def artifactName = "commons"
def gitHash = Process("git rev-parse --short HEAD").lineStream.head
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can avoid using Process and instead use ammonite os.proc

@nh13 nh13 marked this pull request as ready for review June 11, 2019 18:08
Copy link
Member

@tfenne tfenne left a comment

Choose a reason for hiding this comment

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

A few questions and comments.

after_success:
- codecov
- test "$TRAVIS_REPO_SLUG" == 'fulcrumgenomics/commons' && test "$TRAVIS_PULL_REQUEST"
== 'false' && test "$TRAVIS_BRANCH" == 'master' && sbt +publish
== 'false' && test "$TRAVIS_BRANCH" == 'master' && mill mill.scalalib.PublishModule/publishAll "${SONATYPE_USERNAME}:${SONATYPE_PASSWORD}" __.publishArtifacts --release true
Copy link
Member

Choose a reason for hiding this comment

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

This is a little cumbersome. Is there a way to put some of this line into the build file so that the command is closer to mill publishAll? Thinking about when we're doing official releases and doing this by hand.

Copy link
Member

Choose a reason for hiding this comment

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

Also, is this doing cross-builds?

@@ -3,7 +3,6 @@
[![Code Review](https://api.codacy.com/project/badge/grade/52e1d786d9784c7192fae2f8e853fa34)](https://www.codacy.com/app/contact_32/commons)
[![Maven Central](https://maven-badges.herokuapp.com/maven-central/com.fulcrumgenomics/commons_2.11/badge.svg)](https://maven-badges.herokuapp.com/maven-central/com.fulcrumgenomics/commons_2.11)
[![Javadocs](http://javadoc.io/badge/com.fulcrumgenomics/commons_2.12.svg)](http://javadoc.io/doc/com.fulcrumgenomics/commons_2.12)
[![Dependency Status](https://www.versioneye.com/user/projects/56b2d2d593b95a003c714340/badge.svg)](https://www.versioneye.com/user/projects/56b2d2d593b95a003c714340#dialog_dependency_badge)
Copy link
Member

Choose a reason for hiding this comment

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

Could we also add a little to the README about how to build the project, so how to get the minimum version of mill and what the command is to compile/test/build jar? Perhaps how to publish locally?

Copy link
Member

Choose a reason for hiding this comment

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

We should also think about how one gets setup in IntelliJ since IntelliJ doesn't (yet) understand mill build files.

Copy link
Member Author

Choose a reason for hiding this comment

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

mill mill.scalalib.GenIdea/idea;

def scoverageVersion = "1.3.1"
def scalacOptions = Seq("-target:jvm-1.8", "-deprecation", "-unchecked")

// TODO: start year (2015)
Copy link
Member

Choose a reason for hiding this comment

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

Can we resolve these TODOs?

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 could use some help 👍

import scala.sys.process.Process

object commons extends ScalaModule with ScoverageModule with PublishModule {
def artifactName = "commons"
Copy link
Member

Choose a reason for hiding this comment

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

Could we do something like getClass.getSimpleName here to standardize object == artifact name? Or do we want to embed the version name here?

object commons extends ScalaModule with ScoverageModule with PublishModule {
def artifactName = "commons"
def gitHash = os.proc("git", "rev-parse", "--short", "HEAD").call().out.trim
def publishVersion = s"0.8.0-${gitHash}-SNAPSHOT"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there's an easy way to pass custom params to a mill build such that we could define the version as if (release) "0.8.0" else (s"....")

def artifactName = "commons"
def gitHash = os.proc("git", "rev-parse", "--short", "HEAD").call().out.trim
def publishVersion = s"0.8.0-${gitHash}-SNAPSHOT"
def scalaVersion = "2.12.8"
Copy link
Member

Choose a reason for hiding this comment

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

So do we lose cross-building? That seems less than ideal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, I'll want to do add that support. Mill has facilities to do this http://www.lihaoyi.com/mill/page/cross-builds.html

@@ -35,7 +35,7 @@ import scala.collection.mutable
*/
class AsyncStreamSinkTest extends UnitSpec {
"AsyncStreamSink" should "capture all the output" in {
val file = Paths.get("src/test/resources/com/fulcrumgenomics/commons/io/async-stream-sink-test.txt")
val file = Paths.get("commons/test/resources/com/fulcrumgenomics/commons/io/async-stream-sink-test.txt")
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit surprised by this. Does mill require even for single-module builds that the main and test sources are within a module directory?

Copy link
Member Author

Choose a reason for hiding this comment

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

@nh13
Copy link
Member Author

nh13 commented Jun 13, 2019

@tfenne I'll address your feedback, but I added a few comments for your future reading.

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