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

Try to update to sbt-typelevel #449

Merged
merged 17 commits into from
Mar 6, 2022
Merged

Try to update to sbt-typelevel #449

merged 17 commits into from
Mar 6, 2022

Conversation

johnynek
Copy link
Collaborator

@johnynek johnynek commented Mar 5, 2022

address #448

We should try to verify that we are still testing everything especially that we didn't break scala-native

cc @armanbilge

build.sbt Outdated
Comment on lines 122 to 127
lazy val commonJsSettings = Seq(
scalaJSStage in Global := FastOptStage,
Global / scalaJSStage := FastOptStage,
parallelExecution := false,
jsEnv := new org.scalajs.jsenv.nodejs.NodeJSEnv(),
coverageEnabled := false
)
Copy link
Member

Choose a reason for hiding this comment

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

basically none of these settings are needed, except for coverage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there are only four right? So you are saying "in commonJsSettings you may remove the first three settings"

The "basically" is confusing me since it sounds like you are saying something imprecise by adding that word.

Copy link
Member

@armanbilge armanbilge Mar 5, 2022

Choose a reason for hiding this comment

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

Yes you caught me :) To be precise:

  • Global / scalaJSStage := FastOptStage is the default and unneeded
  • jsEnv := new org.scalajs.jsenv.nodejs.NodeJSEnv() is the default and unneeded
  • parallelExecution := false is not the default, but I'd be surprised if it's actually needed
  • coverageEnabled := false is also quite likely not needed, there is coverage on SJS but its occasionally buggy. safe bet is to disable

build.sbt Outdated
Comment on lines 136 to 140
lazy val noPublish = commonSettings ++ Seq(
skip in publish := true,
publish / skip := true,
mimaPreviousArtifacts := Set.empty,
coverageEnabled := false
)
Copy link
Member

Choose a reason for hiding this comment

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

We can remove these settings, anywhere you'd want them use NoPublishPlugin instead.

@armanbilge
Copy link
Member

We should skip the Native/Scala3 job in CI:

githubWorkflowBuildMatrixExclusions +=
    MatrixExclude(Map("project" -> "rootNative", "scala" -> Scala3Version))

build.sbt Outdated
Comment on lines 96 to 97
.enablePlugins(MdocPlugin)
.enablePlugins(NoPublishPlugin)
Copy link
Member

Choose a reason for hiding this comment

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

I can do it in a follow-up PR, but I think if you add

addSbtPlugin("org.typelevel" % "sbt-typelevel-site" % "0.4.6")

we can get a website for free, since it looks like the docs are not currently published.

Suggested change
.enablePlugins(MdocPlugin)
.enablePlugins(NoPublishPlugin)
.enablePlugins(TypelevelSitePlugin)

Copy link
Member

Choose a reason for hiding this comment

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

Also

ThisBuild / tlSitePublishBranch := Some("master")

Copy link
Member

Choose a reason for hiding this comment

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

Oh, and intro.md should be renamed to index.md :)

Comment on lines +7 to +11
ThisBuild / scalaVersion := Scala213
ThisBuild / tlVersionIntroduced := Map("3" -> "0.4.2")
ThisBuild / crossScalaVersions := Seq(Scala213, Scala212, Scala3Version)
ThisBuild / githubWorkflowBuildMatrixExclusions +=
MatrixExclude(Map("project" -> "rootNative", "scala" -> Scala3Version))
Copy link
Member

Choose a reason for hiding this comment

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

We'll need this to get snapshots since the primary branch is master not main.

ThisBuild / tlCiReleaseBranches := Seq("master")

@armanbilge
Copy link
Member

armanbilge commented Mar 6, 2022

Adapting this from the cats-parse build to restore the coverage job.

ThisBuild / githubWorkflowAddedJobs +=
  WorkflowJob(
    id = "coverage",
    name = "Generate coverage report",
    scalas = List("2.13.8"),
    steps = List(WorkflowStep.Checkout) ++ WorkflowStep.SetupJava(
      githubWorkflowJavaVersions.value.toList
    ) ++ githubWorkflowGeneratedCacheSteps.value ++ List(
      WorkflowStep.Sbt(List("coverage", "rootJVM/test", "coverageAggregate")),
      WorkflowStep.Run(List("bash <(curl -s https://codecov.io/bash)"))
    )
  )

@codecov-commenter
Copy link

codecov-commenter commented Mar 6, 2022

Codecov Report

Merging #449 (28a15c7) into master (2962551) will decrease coverage by 0.24%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #449      +/-   ##
==========================================
- Coverage   99.51%   99.27%   -0.25%     
==========================================
  Files           8        8              
  Lines         412      411       -1     
  Branches       41       37       -4     
==========================================
- Hits          410      408       -2     
- Misses          2        3       +1     
Impacted Files Coverage Δ
...main/scala/org/typelevel/paiges/CatsDocument.scala 100.00% <ø> (ø)
...rc/main/scala/org/typelevel/paiges/instances.scala 100.00% <ø> (ø)
....13+/org/typelevel/paiges/ScalaVersionCompat.scala 100.00% <ø> (ø)
...re/src/main/scala/org/typelevel/paiges/Chunk.scala 100.00% <ø> (ø)
...src/main/scala/org/typelevel/paiges/Document.scala 100.00% <ø> (ø)
...re/src/main/scala/org/typelevel/paiges/Style.scala 100.00% <ø> (ø)
.../src/main/scala/org/typelevel/paiges/package.scala 0.00% <ø> (ø)
core/src/main/scala/org/typelevel/paiges/Doc.scala 99.15% <100.00%> (-0.43%) ⬇️
....12-/org/typelevel/paiges/ScalaVersionCompat.scala
... and 1 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 2962551...28a15c7. Read the comment docs.

)
)

ThisBuild / tlCiReleaseBranches := Seq("master")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ThisBuild / tlCiReleaseBranches := Seq("master")
ThisBuild / tlCiReleaseBranches := Seq("master")
ThisBuild / tlSitePublishBranch := Some("master")

build.sbt Outdated
.settings(
noPublish,
crossScalaVersions := List(Scala212),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
crossScalaVersions := List(Scala212),
crossScalaVersions := List(Scala213),

Copy link
Member

Choose a reason for hiding this comment

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

Or actually, you can remove this setting completely.

Copy link
Member

@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.

👍 I think it looks good, thanks for taking this on :)

Final steps:

  1. Before merging, make sure to delete any publishing secrets from this repository, since these are now set at the org-level.
  2. After merging, it will create a gh-pages branch for the site. Then, you'll need to flip the switch in repo settings to enable site deploy from that branch.

@johnynek johnynek merged commit 2e2480d into master Mar 6, 2022
@johnynek johnynek deleted the oscar/use_sbt_typelevel branch March 6, 2022 03:08
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.

3 participants