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

DavidGregory084 · 2022-03-14T16:29:58Z

This adds a ci command derived from the sbt-github-actions configuration - perhaps that ci command should be namespaced somewhat? tlCi or typelevelCi?

I think this does what folks want but it would be good to get a few eyes on this to sense check it. 😆

In general this feels a little fragile because a lot of the things we need have become Strings by the time we need to evaluate them to build the sbt command.

Unfortunately addCommandAlias doesn't work very well with dynamic data - it produces multiple keys in one go Seq[Setting[State => State]], and sbt would not let me use other key .values within the command definition even while wrapped withinDef.settings. What I have done replicates what addCommandAlias does under the hood.

Contributed on behalf of the @opencastsoftware open source team. 👋

…lves typelevel#202

DavidGregory084 · 2022-03-14T16:31:11Z

ci/src/main/scala/org/typelevel/sbt/TypelevelCiPlugin.scala

+      case (k, v) =>
+        val renderedCond = s"matrix.$k == '$v'"
+
+        command.cond.forall { cond =>


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

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'")

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.

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? :)

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.

armanbilge · 2022-03-14T16:45:30Z

ci/src/main/scala/org/typelevel/sbt/TypelevelCiPlugin.scala

+      step <- workflowSteps.collect {
+        case sbt: WorkflowStep.Sbt if matchingCondition(sbt, matrixValues) => sbt
+      }
+      command <- sbtStepPreamble ++ step.commands


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

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🤞

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

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!

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!

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.

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

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

armanbilge · 2022-03-14T16:48:00Z

ci/src/main/scala/org/typelevel/sbt/TypelevelCiPlugin.scala

@@ -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 := {


I actually don't know what these GlobalScope / Keys.onLoad incantations are for? :)

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.

armanbilge

Right, it's the conditionals that make this problem hard. I think if we want to do this right and not be brittle, we really need a proper interpreter for GH conditional statements without requiring them to be formatted in a particular way.

djspiewak · 2022-03-14T17:10:53Z

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.

DavidGregory084 · 2022-03-15T10:50:53Z

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 it seems that command aliases are global - onLoad and onUnload actually seem to be defined only in GlobalScope. Those are also the scopes that are used in the source of addCommandAlias from what I can tell.
It seems like the best way to think about command aliases is like bash aliases - they are purely syntactic and don't care about scopes.

DavidGregory084 · 2022-03-15T11:47:37Z

Right, it's the conditionals that make this problem hard. I think if we want to do this right and not be brittle, we really need a proper interpreter for GH conditional statements without requiring them to be formatted in a particular way.

Yeah, either that or make the representation of the conditions more restrictive in sbt-github-actions' AST. I think we could fairly easily support a subset of Github Actions expressions but we would most likely want to avoid implementing anything more than literals and operators, as I imagine that the rest is quite a large moving target.

armanbilge · 2022-03-15T12:22:27Z

Yeah, either that or make the representation of the conditions more restrictive in sbt-github-actions' AST.

I don't think we can easily do that in a backwards-compatible way.

Actually, overall there don't seem to be many expression operators/functions. But to start with if we can interpret x.y, ( ), and || && that probably covers nearly all use-cases. We just need to decide how to fallback gracefully.

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

DavidGregory084 · 2022-03-21T13:24:09Z

I've been thinking about this a bit...

If we get into evaluating GitHub Actions expressions there's quite a lot involved; we'd need an AST, a parser, an evaluator, and I guess if we are investing that much effort we probably want to do things properly and worry about the differences in the textual representations of literals on JVM vs JS (escape sequences etc.) and the semantics that GitHub Actions has for its expressions, i.e. the way it deals with type coercions and the fact that string comparisons are case insensitive.

However there is a super gross thing that 80% works for the cases we care about, namely using a Javascript engine like Rhino:

  def toJsLiteral(mapping: Map[String, String]): String = {
    mapping
      .view
      .map {
        case (k, v) =>
          s"""  "$k": "$v""""
      }
      .mkString("{\n", ",\n", "\n}\n")
  }

  private def matchingCondition(command: WorkflowStep.Sbt, matrix: Map[String, String]) = {
    val context = Context.enter()

    try {
      command.cond.fold(true) { cond =>
        val scope = context.initStandardObjects()

        val script = s"""|var matrix = ${toJsLiteral(matrix)};
                         |var env = ${toJsLiteral(command.env)}
                         |java.lang.System.out.println('matrix = ' + JSON.stringify(matrix));
                         |java.lang.System.out.println('env = ' + JSON.stringify(env));
                         |java.lang.System.out.println("cond = $cond");
                         |var result = $cond;
                         |java.lang.System.out.println('result = ' + JSON.stringify(result));""".stripMargin

        val result = context.evaluateString(
          scope,
          script,
          "",
          1,
          null
        )

        scope.get("result") match {
          case lang.Boolean.FALSE => false
          case lang.Boolean.TRUE => true
          case _ => true
        }
      }
    } finally {
      Context.exit()
    }
  }

====>

matrix = {"project":"rootJVM","os":"ubuntu-latest","java":"temurin@8","jsenv":"NodeJS","scala":"2.13.8"}
env = {}
cond = matrix.java == 'temurin@8'
result = true
-----------------
matrix = {"project":"rootJVM","os":"ubuntu-latest","java":"temurin@8","jsenv":"NodeJS","scala":"2.13.8"}
env = {}
cond = matrix.project == 'rootJS'
result = false

I feel like the right answer here is probably somewhere in between these two extremes but I'm not sure I know what that middle ground is. Do you guys have any ideas?

armanbilge · 2022-03-21T13:43:46Z

Ooh, that is grossly brilliant! That's super tempting tbh 😁

In fact, there are even JS libs that can actually evaluate these expressions, e.g.:
https://github.com/cschleiden/github-actions-parser#evaluate-an-expression

Add CI command alias derived from the sbt-github-actions matrix, reso…

450c07e

…lves typelevel#202

DavidGregory084 commented Mar 14, 2022

View reviewed changes

armanbilge linked an issue Mar 14, 2022 that may be closed by this pull request

A ci command alias auto-derived from workflow steps #202

Open

armanbilge reviewed Mar 14, 2022

View reviewed changes

DavidGregory084 force-pushed the ci-command-alias branch 2 times, most recently from 89dab7b to 450c07e Compare March 15, 2022 13:41

mergify bot added the ci label Jun 4, 2022

armanbilge mentioned this pull request Jul 2, 2022

Add doc to prePR command alias #322

Merged

armanbilge mentioned this pull request Jun 19, 2023

Better prePR command #542

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

DavidGregory084 commented Mar 14, 2022 •

edited

DavidGregory084 Mar 14, 2022

armanbilge Mar 14, 2022

DavidGregory084 Mar 14, 2022

armanbilge Mar 14, 2022

armanbilge Mar 14, 2022

armanbilge Mar 14, 2022

DavidGregory084 Mar 14, 2022

armanbilge Mar 14, 2022

DavidGregory084 Mar 14, 2022

DavidGregory084 Mar 14, 2022 •

edited

armanbilge Mar 14, 2022

DavidGregory084 Mar 15, 2022

armanbilge Mar 15, 2022 •

edited

armanbilge Mar 14, 2022

DavidGregory084 Mar 14, 2022

armanbilge left a comment

djspiewak commented Mar 14, 2022

DavidGregory084 commented Mar 15, 2022

DavidGregory084 commented Mar 15, 2022

armanbilge commented Mar 15, 2022

DavidGregory084 commented Mar 21, 2022

armanbilge commented Mar 21, 2022

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

Are you sure you want to change the base?

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

Conversation

DavidGregory084 commented Mar 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidGregory084 Mar 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge Mar 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge left a comment

Choose a reason for hiding this comment

djspiewak commented Mar 14, 2022

DavidGregory084 commented Mar 15, 2022

DavidGregory084 commented Mar 15, 2022

armanbilge commented Mar 15, 2022

DavidGregory084 commented Mar 21, 2022

armanbilge commented Mar 21, 2022

DavidGregory084 commented Mar 14, 2022 •

edited

DavidGregory084 Mar 14, 2022 •

edited

armanbilge Mar 15, 2022 •

edited