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

Split CI into multiple steps #65

Merged
merged 25 commits into from
Jan 16, 2022
Merged

Conversation

armanbilge
Copy link
Member

@armanbilge armanbilge commented Jan 9, 2022

Based on #64. Fixes #60.

Overall this is a nice improvement: not only does it make it easier to see which step has failed in CI, it also makes it much, much easier to manipulate these steps within sbt without aliasing and re-aliasing the ci commands.

Opening for CI, more cleanups to come.

@rossabaker
Copy link
Member

Does the ci alias survive in this model?

Another option I wondered about was just leaning into the SBT DAG and making ci a proper task that depends on the various linters. We'd lose the granular CI view, but get maximum parallelism. I think it would also be opaque, which would make it harder to modify existing pipelines other than replacing them wholesale.

@armanbilge
Copy link
Member Author

armanbilge commented Jan 9, 2022

Does the ci alias survive in this model?

At the moment, no. What we can do maybe is automatically generate a ci command by just combining all the Sbt steps in the workflow. Edit: I suppose we'd also have to iterate over the matrix as well 🤔 .

We'd lose the granular CI view, but get maximum parallelism.

Parallelism is not always a good thing. Scala.js and Native have extremely memory intensive linking-steps, that pummel builds and CI. Technically this is a problem in sbt itself—there's tools to control parallelism based on quotas for CPU and IO I think but nothing for memory.

@@ -62,38 +59,12 @@ object TypelevelPlugin extends AutoPlugin {
scala <- githubWorkflowScalaVersions.value.init
java <- githubWorkflowJavaVersions.value.tail
} yield MatrixExclude(Map("scala" -> scala, "java" -> java.render))
},
githubWorkflowBuild ~= { steps =>
WorkflowStep.Sbt(List("headerCheckAll", "scalafmtCheckAll", "scalafmtSbtCheck")) +: steps
Copy link
Member

Choose a reason for hiding this comment

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

Can these steps take arguments, even if just as one string? Some of the scalafix linters require an arg.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good question. Yes, it seems we are already using that in http4s:
https://github.com/http4s/http4s/blob/1baf3d4f5fb135098b6b857a488b36cd39ca4a56/build.sbt#L41

@armanbilge
Copy link
Member Author

I think it would also be opaque, which would make it harder to modify existing pipelines other than replacing them wholesale.

Btw, nothing here makes it hard for a project to define their own opaque ci command and use it to replace all the separate workflow steps here. It's a very easy thing for a project to do, but it's not easy to build a flexible plugin ecosystem on top of it.

@armanbilge armanbilge marked this pull request as ready for review January 9, 2022 22:39
@armanbilge
Copy link
Member Author

I know it's plugin mayhem, but thanks to these changes I was able to extract ci-header and ci-formatting plugins as well that automatically add the necessary check steps to the workflow. This should be friendly to Rob, who I think wants headers but no formatting, and Michael, who I think wants formatting but no headers.

Since we can't merge without #62, I will temporarily put all these PRs together into an experimental branch to publish some snapshots.

@armanbilge
Copy link
Member Author

Hold, until we figure this out: http4s/sbt-http4s-org#120 (comment)

If there's non-determinism (which would surprise me, but anything's possible) then puts a dent in the plans to have CI steps automatically added by plugins.

@armanbilge armanbilge marked this pull request as draft January 10, 2022 00:31
@armanbilge armanbilge marked this pull request as ready for review January 10, 2022 05:14
@armanbilge
Copy link
Member Author

armanbilge commented Jan 10, 2022

I re-integrated the header/formatting plugins into the opinionated plugin. They are simple enough for downstreams to replicate/workaround as needed.

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

👍 I'm surprised by the pushback to either of these checks.

  • A majority of Typelevel projects already have scalafmt (41 out of 74). If people want an outlet for individual artistic expression, consider docs instead of curly brace art.
  • Apache isn't prescriptive about headers for non-ASF projects, but they require it of their own. It's helpful when files get copied and pasted... which happens often... like this repo.

And, at the end of the day, this is the opinionated plugin. There's still the unopinionated one.

@armanbilge armanbilge linked an issue Jan 11, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants