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.

Sign up for GitHub

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

Jump to bottom

Add CI command alias derived from the sbt-github-actions matrix #210

Open
wants to merge 1 commit into
base: series/0.4
Choose a base branch
from
Open

Add CI command alias derived from the sbt-github-actions matrix #210

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 93 additions & 4 deletions ci/src/main/scala/org/typelevel/sbt/TypelevelCiPlugin.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@

package org.typelevel.sbt

import sbt._
import com.typesafe.tools.mima.plugin.MimaPlugin
import org.typelevel.sbt.gha.GenerativePlugin
import org.typelevel.sbt.gha.GitHubActionsPlugin
import org.typelevel.sbt.gha.GenerativePlugin.autoImport._
import com.typesafe.tools.mima.plugin.MimaPlugin
import org.typelevel.sbt.gha.GitHubActionsPlugin
import sbt._

object TypelevelCiPlugin extends AutoPlugin {

Expand All @@ -46,12 +46,101 @@ object TypelevelCiPlugin extends AutoPlugin {
cond = Some(primaryJavaCond.value)
)
),
githubWorkflowJavaVersions := Seq(JavaSpec.temurin("8"))
githubWorkflowJavaVersions := Seq(JavaSpec.temurin("8")),
GlobalScope / Keys.onLoad := {
Copy link
Member

Choose a reason for hiding this comment

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

I actually don't know what these GlobalScope / Keys.onLoad incantations are 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.

Those are sbt's global load hooks - they add the alias when all of the project's keys have loaded and remove it when unloading the projects. Setting up these specific hooks is what addCommandAlias does under the hood. If you try to use onLoad in the default scope you get 'reference to undefined setting' unfortunately.

val oses = githubWorkflowOSes.value.toList
val javas = githubWorkflowJavaVersions.value.toList
val scalas = githubWorkflowScalaVersions.value.toList
val additions = githubWorkflowBuildMatrixAdditions.value
val inclusions = githubWorkflowBuildMatrixInclusions.value.toList
val exclusions = githubWorkflowBuildMatrixExclusions.value.toList
val stepPreamble = githubWorkflowBuildSbtStepPreamble.value.toList

(GlobalScope / Keys.onLoad).value.compose { (state: State) =>
addCiAlias(
state,
oses,
javas,
scalas,
additions,
inclusions,
exclusions,
stepPreamble,
githubWorkflowBuild.value
)
}
},
GlobalScope / Keys.onUnload := {
(GlobalScope / Keys.onUnload)
.value
.compose((state: State) => BasicCommands.removeAlias(state, "ci"))
}
)

private val primaryJavaCond = Def.setting {
val java = githubWorkflowJavaVersions.value.head
s"matrix.java == '${java.render}'"
}

private def addCiAlias(
state: State,
oses: List[String],
javaVersions: List[JavaSpec],
scalaVersions: List[String],
matrixAdditions: Map[String, List[String]],
matrixInclusions: List[MatrixInclude],
matrixExclusions: List[MatrixExclude],
sbtStepPreamble: List[String],
workflowSteps: Seq[WorkflowStep]) = {

val buildMatrix = GenerativePlugin.expandMatrix(
// Cannot meaningfully iterate OS or Java version here
oses = oses.take(1),
javas = javaVersions.take(1),
scalas = scalaVersions,
matrixAdds = matrixAdditions,
includes = matrixInclusions,
excludes = matrixExclusions
)

val keys = "os" :: "scala" :: "java" :: matrixAdditions.keys.toList.sorted

val commands = for {
matrixRow <- buildMatrix
matrixValues = keys.zip(matrixRow).toMap
step <- workflowSteps.collect {
case sbt: WorkflowStep.Sbt if matchingCondition(sbt, matrixValues) => sbt
}
command <- sbtStepPreamble ++ step.commands
Copy link
Member

Choose a reason for hiding this comment

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

so one of the annoying things is that sbt is stateful. So I wonder if we need to reload or something between steps as a palate-cleanser ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm I think that should be OK - from what I understand State is the global command loop. Anything that you feed into there is essentially interpreted in the same way as if you ran it interactively - so set commands such as setting JsEnv should work fine as long as the key is scoped correctly.
As long as there is nothing that happens as part of those commands that modifies the project configuration on disk, that statefulness should not necessarily cause a problem, which I think is the case for something like the ci command where we do lots of linting, compiling and tests.
However, my understanding of sbt is not 100% so take that with a pinch of salt🤞

Copy link
Member

Choose a reason for hiding this comment

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

For sure! Except when these steps run in CI it's not part of a single interactive session—each workflow step reboots sbt essentially.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahhhh I see, so you are concerned about potential session value leakage in some of the existing commands. FWIW I think interspersing reload would make things extremely slow!

Copy link
Member Author

@DavidGregory084 DavidGregory084 Mar 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Okay this is cool. Just a brief comment though: is Global even going to work here? Most of the github actions configuration is in ThisBuild.

Yeah I wondered about that too, however onLoad and onUnload actually seem to be undefined in other scopes - I guess it makes sense that command aliases are global. Those are the scopes that are used in the source of addCommandAlias from what I can tell.

Remind me not to try replying to comments on my phone, who knows how this ended up here!

Copy link
Member

Choose a reason for hiding this comment

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

FWIW I think interspersing reload would make things extremely slow!

Oh, absolutely agree! But the if the goal is to faithfully replicate CI then I really don't see another way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, what if we do session clear instead of a full reload? That would revert any settings manipulated using set.

Copy link
Member

@armanbilge armanbilge Mar 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Oh yeah, that's probably just as good. Not sure what reload does that session clear doesn't (besides reload the build config which obviously doesn't apply here).

} yield replaceMatrixVars(command, matrixValues)

val commandAlias = TypelevelKernelPlugin.mkCommand(commands)

BasicCommands.addAlias(
state,
"ci",
commandAlias
)
}

private def matchingCondition(
command: WorkflowStep.Sbt,
matrixValues: Map[String, String]) = {
// For all matrix values
matrixValues.forall {
case (k, v) =>
val renderedCond = s"matrix.$k == '$v'"

command.cond.forall { cond =>
Copy link
Member Author

Choose a reason for hiding this comment

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

Is it likely that the condition itself might refer to a matrix variable? Perhaps cond needs to have its matrix vars replaced too.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's very likely. In fact we do it from sbt-typelevel itself.

sbt-typelevel/ci/src/main/scala/org/typelevel/sbt/CrossRootProject.scala

Line 154 in a4282ae

cond = Some("matrix.project == 'rootJS'")

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah that should work OK - what I was worried about was things like

Some("something == '${{ matrix.project }}'")

In that scenario we would need to make sure to replace any Github Actions ${{ }} variables before checking the conditions.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I might be confused but in conditions you should assume everything is wrapped in ${{ }}. Like, the matrix.project in that condition should definitely be substituted, no? :)

Copy link
Member

Choose a reason for hiding this comment

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

https://docs.github.com/en/actions/learn-github-actions/expressions

When you use expressions in an if conditional, you may omit the expression syntax (${{ }}) because GitHub automatically evaluates the if conditional as an expression.

// If the condition starts with this matrix variable, whole condition must be equal
if (cond.startsWith(s"matrix.$k")) cond == renderedCond else true
}
}
}

private def replaceMatrixVars(command: String, matrixValues: Map[String, String]): String =
matrixValues.foldLeft(command) {
case (cmd, (matrixVar, matrixVarValue)) =>
cmd.replaceAll(s"\\$$\\{\\{ matrix.${matrixVar} \\}\\}", matrixVarValue)
}
}