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

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

Conversation

DavidGregory084
Copy link
Member

@DavidGregory084 DavidGregory084 commented Mar 14, 2022

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

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.

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.

@armanbilge armanbilge linked an issue Mar 14, 2022 that may be closed by this pull request
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

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

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

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

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.

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

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 DavidGregory084 force-pushed the ci-command-alias branch 2 times, most recently from 89dab7b to 450c07e Compare March 15, 2022 13:41
@DavidGregory084
Copy link
Member Author

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
Copy link
Member

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

@mergify mergify bot added the ci label Jun 4, 2022
@armanbilge armanbilge mentioned this pull request Jun 19, 2023
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.

A ci command alias auto-derived from workflow steps
3 participants