-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
new better release settings #2732
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2732 +/- ##
==========================================
+ Coverage 95.13% 95.14% +0.01%
==========================================
Files 365 365
Lines 6799 6816 +17
Branches 304 297 -7
==========================================
+ Hits 6468 6485 +17
Misses 331 331
Continue to review full report at Codecov.
|
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.
Thanks, @kailuowang! Generally sounds good to me; I just had a question or two.
I've also wondered whether we should switch to using sbt-ci-release. With cross-versioning we'd have to get a bit wordier in our Travis config, and I think that we might need to make our builds a bit more stable first, but it may be able to reduce the developer burden of publishing.
build.sbt
Outdated
.settings(moduleName := "root") | ||
.settings(catsSettings) | ||
.settings(moduleName := "root", crossScalaVersions := Nil) | ||
.settings(publishSettings) |
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.
It looks a little weird to have publishSettings
followed by noPublishSettings
. This is intentional, right? If so, it might be helpful to have a brief comment about why this is being done.
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.
Good catch. I added some comment here. The basic thing is that sbt release
command is run through aggregation from this root module. Thus it requires some release settings, then the noPublishSettings
makes sure the module itself is not published.
@@ -825,12 +824,11 @@ lazy val sharedReleaseProcess = Seq( | |||
releaseProcess := Seq[ReleaseStep]( | |||
checkSnapshotDependencies, | |||
inquireVersions, | |||
runClean, | |||
releaseStepCommand("validate"), | |||
releaseStepCommandAndRemaining("+validate"), |
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 this a switch from running validate
from the current version to all versions? If so, does this lead to OOMs or anything?
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.
Not really. Before the validate is still run against all scala versions because the crossScalaVersions
settings on the root module and the releaseCrossBuild := true
. sbt release automatically runs certain steps on each modules against all crossScalaVersions
set on the root module. Now, with the new sbt version , the +validate
will performe on each aggregated module correctly on their own crossScalaVersions
. The root module's crossScalaVersions
should set as Nil
, otherwise it will conflict with the aggregrated module's crossScalaVersion
.
Verifying this does indeed fix #2730. Thanks @kailuowang! |
Thanks, @kailuowang! LGTM but it looks like scalafmt is complaining about formatting in build.sbt. |
build.sbt
Outdated
.settings(noPublishSettings) | ||
.settings(moduleName := "root", crossScalaVersions := Nil) | ||
.settings(publishSettings) // these settings are needed to release all aggregated modules under this root module | ||
.settings(noPublishSettings) // this is to exclue the root module itself from being published. |
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.
.settings(noPublishSettings) // this is to exclue the root module itself from being published. | |
.settings(noPublishSettings) // this is to exclue the root module itself from being published. |
There is a dangling space at the end of this line. I've removed it in this suggestion.
@kailuowang once you fix the build.sbt formatting issue, you may run into an issue where now SBT complains about the build's |
@LukaJCB any feedback? |
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'm really not an sbt expert, but the changes do make sense to me :)
The old release steps are from the sbt 0.13 era. Now that modular crossScalaVersions issue is improved in sbt 1.x, we need to switch to this new release steps - i.e. the root module should have a
Nil
crossScalaVersions
setting, then each module can have their owncrossScalaVersions
settings and release accordingly.This change also reduced significantly the number of unnecessary settings on the root module - which fixes #2730
To test this build code, I released a
0.0.1-DONOTUSE
, and tested with cats-tagless