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

Setup sbt-typelevel #434

Merged
merged 3 commits into from
Jun 28, 2023
Merged

Conversation

armanbilge
Copy link
Member

@armanbilge armanbilge commented Jun 28, 2023

Closes #430.

plugin/scripted runs as its own cell in the matrix, and we also parallelize on platform (JVM/JS) and Scala version. docs/mdoc and docs/laikaSite run as part of the JVM/2.12 job.

I made minimal changes required for source code to compile under the new compiler flags, but disabled fatal warnings for now (mostly unused imports I think). I suppose these can be addressed in a follow-up PR?

@jenshalm jenshalm added this to the 1.0.0-M1 milestone Jun 28, 2023
@jenshalm
Copy link
Contributor

jenshalm commented Jun 28, 2023

Amazing, thank you so much. It's cool that the new build even appears somewhat simpler than the old one.

I have only a few (hopefully not too stupid) questions:

  • Just to ensure I can operate this in the future: what are the required steps when the build is updated? I assume it's just one command to run that generates all the YAML?
  • How does the job know the intended release number? Is it simply deduced from the tag name?
  • It looks like release tags now need the v prefix? Just confirming because I did not use this pattern for 0.x releases.
  • I can see that there is a mima step. I assume there is some magic that this is a no-op as long as there are only milestone releases for 1.0?
  • It looks like everything only runs on Java 8? Is this common for Typelevel projects? Given this is a very old version I am a bit surprised. Does the choice simply reflect that this is the minimum required version for Scala and we simply rely on everything still working in Java 11/17?
  • I am wondering about the purpose of these exclusions: branches: ['**', '!update/**', '!pr/**']. The update/ prefix is used by scala-steward, does it mean we want to skip running the build for dependency updates? And I've never seen the pr/ prefix for branches, what is this usually used for?
  • I assume the comment don't include the plugin in rootJVM, only in root refers to the fact that the project matrix is [rootJS, rootJVM, plugin]?
  • I assume all required secrets are set up org-wide? Does it mean we could immediately run a release after merging this and everything will just work?

From my side we can immediately run a release after this is merged. If you want, you can also trigger it yourself, in case you want to observe something about the sbt-typelevel RC.

Regarding the warnings, I only get one for Scala 2.12/2.13, which is the deprecation warning for EmberServerBuilder.default[F] - I am not aware of the recommended way to address this (it's in ServerBuilder:95 in the preview module).
All other warnings are deprecations for Scala 3 which I think I'd prefer to address with @nowarn (I think that's working now in Scala 3?). One warning is about the deprecated case class bug we talked about and one is for toVector which needs to stay because of Scala 2.12.
I don't see any warnings about unused imports though, is this a flag only switched on in the new build?.

@armanbilge
Copy link
Member Author

  • Just to ensure I can operate this in the future: what are the required steps when the build is updated? I assume it's just one command to run that generates all the YAML?

Yes, to re-generate the YAML use githubWorkflowGenerate. There is also a more general command prePR that will do that as well as run scalafmt.


  • How does the job know the intended release number? Is it simply deduced from the tag name?

Yes, exactly. The exact logic is here.

https://github.com/typelevel/sbt-typelevel/blob/3c11cd840eccb505457d1480f9e3ffb0d0dd1b24/versioning/src/main/scala/org/typelevel/sbt/TypelevelVersioningPlugin.scala#L74


  • It looks like release tags now need the v prefix? Just confirming because I did not use this pattern for 0.x releases.

Yes, that's right.


  • I can see that there is a mima step. I assume there is some magic that this is a no-op as long as there are only milestone releases for 1.0?

Yes, the plugin looks at previous tags in the repo to determine the previous artifcacts for MiMa. So long as there are no stable 1.x tags (i.e. Ms and RCs do not count) there is nothing to compare against. As soon as you tag v1.0.0 it will begin checking against that automatically, and so forth.


  • It looks like everything only runs on Java 8? Is this common for Typelevel projects? Given this is a very old version I am a bit surprised. Does the choice simply reflect that this is the minimum required version for Scala and we simply rely on everything still working in Java 11/17?

Yeah, it is common and unfortunate 😕 see some discussion in typelevel/sbt-typelevel#215. tl;dr is that projects such as Cats, Cats Effect, and http4s are going to be stuck on JDK 8 for a while, although maybe in the future we can run their builds on a newer JDK while continuing to run the tests on JDK 8. This is relevant because Laika is mostly a build-time dependency.

If you want, we can:

  • just run everything on JDK 17. The compiler flags will make sure your artifacts are still compatible with JDK 8. But if one of your dependencies starts requiring e.g. JDK 11+, we won't catch that.

  • add JDK 11 and/or 17 alongside JDK 8 to the CI matrix, just to make sure they work. We could do this just for e.g. the JVM/2.12 job and the plugin job to keep from growing the matrix too much.


  • am wondering about the purpose of these exclusions: branches: ['**', '!update/**', '!pr/**']. The update/ prefix is used by scala-steward, does it mean we want to skip running the build for dependency updates? And I've never seen the pr/ prefix for branches, what is this usually used for?

The YAML declares that CI should run in three situations:

  1. When a branch is pushed to the repo.
  2. When you open/push to a PR against a branch in the repo. (Note that this CI job actually runs on a synthetic merge commit between the PR branch and the target branch).
  3. When a tag is pushed to the repo.

So if you push to a branch on the repo and then use that same branch to open a PR, you actually get two CI jobs: one for the branch push, and one for the PR push, which effectively doubles your CI consumption.

This is exactly the situation the Typelevel Steward is in i.e. it creates an update branch in the repo, not in a fork, and then opens a PR. So to mitigate this we disable CI on pushes to the update/ branch, since CI will run on the PR that Steward will open.

The pr/ prefix is something I made up, to be used similarly: if you are developing on a branch on the repo and intend to open a PR with that branch, you can prefix the branch with pr/ to avoid the redundant CI job. In practice I think I am the only person who uses this, maybe it was a dumb idea 😅 further reading in typelevel/sbt-typelevel#177 (comment).

Btw, this is all configurable: you can decide that you want to disable CI on all branches except for e.g. 0.19 and main, so the only way to get CI is to open a PR against one of those.


  • I assume the comment don't include the plugin in rootJVM, only in root refers to the fact that the project matrix is [rootJS, rootJVM, plugin]?

Yes, that's right. The CrossRootProject sorts its aggregates into rootJVM, rootJS (and rootNative) automatically. In this specific case, we don't want to do that: the idea is that rootJVM should be all the JVM projects except the plugin, and then the plugin is handled separately. However, we still want the plugin to be aggregated into the overall root which covers all the projects in the build. Admittedly, it's a bit of a bespoke setup, but builds mixing cross-compiled libraries with sbt plugins are always bespoke unfortunately.


  • I assume all required secrets are set up org-wide? Does it mean we could immediately run a release after merging this and everything will just work?

Yes, that's right. Actually as soon as this PR merges it will attempt to publish a snapshot, as a sort of semi-dry run of publishing. Assuming you are okay with that—if you don't like snapshots, we can disable that too.

From my side we can immediately run a release after this is merged. If you want, you can also trigger it yourself, in case you want to observe something about the sbt-typelevel RC.

If the snapshot publishes ok, then good chances are everything will be working fine, and you can run the proper release whenever you are ready. I will of course keep an eye on it for mishaps 😅


I don't see any warnings about unused imports though, is this a flag only switched on in the new build?.

Yes, it's part of the standard Typelevel scalac settings.

https://github.com/typelevel/sbt-typelevel/blob/3c11cd840eccb505457d1480f9e3ffb0d0dd1b24/settings/src/main/scala/org/typelevel/sbt/TypelevelSettingsPlugin.scala#L106-L135


I think that's all your questions, let me know if you have more or I missed anything. Thanks for looking so quickly!

@jenshalm
Copy link
Contributor

jenshalm commented Jun 28, 2023

Many thanks for all the explanations. I'm just responding to the few remaining unresolved topics:

  • add JDK 11 and/or 17 alongside JDK 8 to the CI matrix, just to make sure they work. We could do this just for e.g. the JVM/2.12 job and the plugin job to keep from growing the matrix too much.

If you think it's generally advisable I'm fine with adding JDK 17, but ideally only for JVM/2.12 and not the plugin axis if that is possible. Given how excessively slow the step is and that it's only 2% of the code base I feel like this step should be fairly safe to run only on one JVM version.

This is exactly the situation the Typelevel Steward is in i.e. it creates an update branch in the repo, not in a fork, and then opens a PR. So to mitigate this we disable CI on pushes to the update/ branch, since CI will run on the PR that Steward will open.

I think I'm still missing something. I interpreted the YAML as to also exclude any PRs created from an update/ branch since the exclusion is on both push and pull_request. Doesn't that prevent both CI runs and not just one of them?

Yes, that's right. Actually as soon as this PR merges it will attempt to publish a snapshot, as a sort of semi-dry run of publishing. Assuming you are okay with that—if you don't like snapshots, we can disable that too.

I don't mind publishing snapshots, but also personally don't feel like I need them. Happy to just follow recommendations in this regard.

From my side we can immediately run a release after this is merged. If you want, you can also trigger it yourself, in case you want to observe something about the sbt-typelevel RC.

If the snapshot publishes ok, then good chances are everything will be working fine, and you can run the proper release whenever you are ready. I will of course keep an eye on it for mishaps 😅

Ok, I will run a release then as soon as this lands.

@armanbilge
Copy link
Member Author

If you think it's generally advisable I'm fine with adding JDK 17, but ideally only for JVM/2.12

Sure, let's add it :)


I think I'm still missing something. I interpreted the YAML as to also exclude any PRs created from an update/ branch since the exclusion is on both push and pull_request. Doesn't that prevent both CI runs and not just one of them?

Nope. Branches listed under pull_request are not the names of the branch the PR is originating from, they are the name of the branch that the PR is targeting. So in this case, it means a PR to a branch prefixed update/ or pr/ will not have CI (actually this is mildly annoying, and I'd like to fix that some point. But PRs against PRs are not common).

@jenshalm jenshalm merged commit 1c9175e into typelevel:main Jun 28, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting up CI releases for version 1.0
2 participants