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

Migrate to sbt-typelevel-ci-release #63

Merged
merged 8 commits into from
Jan 22, 2022

Conversation

armanbilge
Copy link
Member

@armanbilge armanbilge commented Jan 20, 2022

I reached for sbt-typelevel-ci-release instead of sbt-typelevel, since the latter includes scalac settings, formatting, headers, etc. which you are not using/don't want here.

@armanbilge armanbilge changed the title Migrate to sbt-typelevel Migrate to sbt-typelevel-ci-release Jan 20, 2022
Copy link
Member Author

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Ready for review. Thanks!


ThisBuild / githubWorkflowJavaVersions := List(JavaSpec.temurin("8"))
ThisBuild / githubWorkflowArtifactUpload := false
ThisBuild / tlCiReleaseBranches := Seq("main")
Copy link
Member Author

Choose a reason for hiding this comment

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

This sets up snapshots on main.

.settings(commonSettings)
.settings(crossScalaVersions := Seq())
.settings(noPublishSettings)
lazy val root = tlCrossRootProject
Copy link
Member Author

Choose a reason for hiding this comment

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

This automatically sets up the CI matrix for JVM/JS/Native.

Comment on lines -194 to -204
lazy val mimaSettings = Seq(
mimaPreviousArtifacts := Set("org.typelevel" %% moduleName.value % previousVersion),
mimaBinaryIssueFilters := Seq()
)

lazy val publishSettings: Seq[Setting[_]] = Seq(
Test / publishArtifact := false,
pomIncludeRepository := (_ => false),
homepage := Some(url("https://github.com/typelevel/shapeless-3")),
licenses := Seq("Apache 2" -> url("http://www.apache.org/licenses/LICENSE-2.0.txt")),
scmInfo := Some(ScmInfo(url("https://github.com/typelevel/shapeless-3"), "scm:git:git@github.com:typelevel/shapeless-3.git")),
Copy link
Member Author

Choose a reason for hiding this comment

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

sbt-typelevel-ci-release sets this automatically.

@armanbilge armanbilge marked this pull request as ready for review January 20, 2022 16:25
Copy link
Member

@joroKr21 joroKr21 left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on - looks good but I have some questions that I don't understand

build.sbt Outdated

ThisBuild / githubWorkflowJavaVersions := List(JavaSpec.temurin("8"))
ThisBuild / githubWorkflowArtifactUpload := false
ThisBuild / tlCiReleaseBranches := Seq("main")
ThisBuild / githubWorkflowBuildMatrixFailFast := Some(false)
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this? It seems like it's the only setting left from githubWorkflow

Copy link
Member Author

Choose a reason for hiding this comment

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

The default is None which is essentially true—fail fast is enabled (the first failing job cancels the rest). Up to you how you want to run your build :)

Copy link
Member

Choose a reason for hiding this comment

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

Let's remove it 😄

)
)
val jsSettings = Def.settings(
tlVersionIntroduced := Map("3" -> "3.0.1")
Copy link
Member

Choose a reason for hiding this comment

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

What's this for?

Copy link
Member Author

Choose a reason for hiding this comment

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

"The Scala 3 cross-build was introduced in version 3.0.1 of my project". Most projects are using it to indicate when they added Scala 3. Here, since you added the Scala.js cross in v3.0.1 it goes in JS settings. It tells MiMa how far back to look for artifacts.

Copy link
Member

Choose a reason for hiding this comment

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

Ok so when we publish with Scala native we can make it a common setting?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I don't think this can ever be a common setting. Your JVM cross-build was introduced in 3.0.0, your JS cross-build was introduced in 3.0.1, and your Native cross-build will be introduced in 3.?.?. So they are all different.

build.sbt Outdated
@@ -3,55 +3,24 @@ import com.typesafe.tools.mima.core.{ProblemFilters, ReversedMissingMethodProble
val scala3Version = "3.1.0"

ThisBuild / organization := "org.typelevel"
ThisBuild / tlBaseVersion := "3.0"
Copy link
Member

Choose a reason for hiding this comment

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

What is this for? Do we have to bump it after releasing 3.1?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's part of the versioning scheme inherited from sbt-spiewak.
https://github.com/djspiewak/sbt-spiewak/blob/main/versioning.md#compatibility-version

Actually, you should bump it at the time you introduce "3.1-worthy" changes. E.g. if you follow strict semver and only do bugfixes in 3.0.x, your first PR adding a new feature should bump tlBaseVersion to 3.1. This is not enforced, but it's good practice and it makes sure all snapshots from that point are versioned with 3.1-hash-SNAPSHOT.

tlBaseVersion is also important if you ever decide you want to make breaking changes. At that point you would bump it to 4.0, and this would indicate to MiMa to stop checking bincompat against the 3.x artifacts.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks for explaining - I think we should be already on 3.1 (since Scala native forces us to use Scala 3.1) but that can be bumped later as well.

Comment on lines +80 to +90
- name: Make target directories
run: mkdir -p modules/deriving/.js/target modules/test/.jvm/target modules/test/.native/target target .js/target modules/typeable/.js/target modules/deriving/.jvm/target modules/typeable/.native/target .jvm/target .native/target local/.native/target local/.js/target modules/test/.js/target modules/typeable/.jvm/target local/.jvm/target modules/deriving/.native/target project/target

- name: Compress target directories
run: tar cf targets.tar modules/deriving/.js/target modules/test/.jvm/target modules/test/.native/target target .js/target modules/typeable/.js/target modules/deriving/.jvm/target modules/typeable/.native/target .jvm/target .native/target local/.native/target local/.js/target modules/test/.js/target modules/typeable/.jvm/target local/.jvm/target modules/deriving/.native/target project/target

- name: Upload target directories
uses: actions/upload-artifact@v2
with:
name: target-${{ matrix.os }}-${{ matrix.scala }}-${{ matrix.java }}
path: targets.tar
Copy link
Member

Choose a reason for hiding this comment

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

Why is this running on PR?

Copy link
Member Author

@armanbilge armanbilge Jan 22, 2022

Choose a reason for hiding this comment

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

That's a good point. It's not harmful but probably not necessary. We can fix this upstream.

@joroKr21 joroKr21 merged commit aa13627 into typelevel:main Jan 22, 2022
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.

2 participants