Setting shell default for Windows hosts in cross-platform matrix

[ferdnyc]

So, I'm kind of banging my head against a wall with something, and I wanted to see if anyone has any clever solutions.

I have a project which builds in CMake, and the cmake commands are actually all the same whether running on Linux, macOS, or Windows. I've worked very hard to keep things that way, in fact, so that the build tooling could be as platform-agnostic as possible.

So, I have a workflow written with build steps that are largely shared between all platforms in the matrix. (Each OS has its own dependency step, gated with an if: ${{ runner.os == 'Linux' }} (or 'Windows' or 'macos'), but that's the only non-shared config.)

To differentiate the -G argument to cmake, which selects the generator to use for the build system, I'm using action-cond from @haya14busa which works great:

     - name: Select CMake generator
        uses: haya14busa/action-cond@v1
        id: generator
        with:
          cond: ${{ runner.os == 'Windows' }}
          if_true: 'MinGW Makefiles'
          if_false: 'Unix Makefiles'
      - name: Build
        run: |
          cmake -B build -S . -G "${{ steps.generator.outputs.value }}" ...

The problem is, I can't think of any way to do the same thing for the MSYS2 shell. If I wanted to take advantage of the "Default shell" option from the README, I'd end up setting it as the default for every shell, including the ones that run on Linux and macOS, which obviously won't work! I've tried 100 different ways to conditionalize the setting, and they all failed:

1. Add an if: to the defaults: section of the config: Syntax error
2. Add another haya14busa/action-cond@v1 step that sets either msys2 {0} or bash, and use shell: ${{ steps.select_shell.outputs.value }} in subsequent steps: Syntax error, apparently the steps object is not accessible from the metaconfiguration of subsequent steps.
3. Capture the output into an environment variable, the same way I can with CC: ${{ matrix.compiler }}:

   env:
     RUNSHELL: ${{ steps.select_shell.outputs.value }}
   steps:
      - name: Select shell...
        id: select_shell
        with:
         cond: ${{ runner.os == 'Windows' }}
         if_true: 'msys2 -c'
         if_false: 'bash -c'
        ...
     - name: Build
       run: |
         $RUNSHELL 'cmake...'

   (In my defense, I never expected that one to work, and it doesn't. Same issue with accessing steps before it's defined.)
4. The same thing, but set the env in every step that needs it:

   steps:
      - name: Select shell...
        id: select_shell
        with:
         cond: ${{ runner.os == 'Windows' }}
         if_true: 'msys2 -c'
         if_false: 'bash -c'
        ...
     - name: Build
       env:
         RUNSHELL: ${{ steps.select_shell.outputs.value }}
       run: |
         $RUNSHELL 'cmake...'

   That actually came close to working, believe it or not! Actually worked on Linux and macOS. Blew up in the default Powershell on Windows, though.
5. Set up a completely separate job config before the build job, with the same matrix, and have it relay the results of action-cond into an outputs. Have the second job needs: the first, and set shell: ${{ needs.job1.outputs.shell }} on each step: Syntax error, it seems the needs context isn't any more accessible for defining steps than the steps context is.

At this point I feel like I've exhausted all possibilities. Any suggestions, or do I really have to take my nice, cross-platform workflows and duplicate all of the run steps, just so I can get Windows to run them using the correct shell? 😞

[eine]

  strategy:
    matrix:
      include:
        - { os: windows, shell: msys2 },
        - { os: ubuntu,  shell: bash  },
        - { os: macos,   shell: bash  },
  runs-on: ${{ matrix.os }}
  defaults:
    run:
      shell: ${{ matrix.shell} {0}
  steps:

WRT selecting the CMake generator, I don't really understand why you are using an specific Action for that, given that all you need to do is set an environment variable. Unless the Action is aware of how MSYS2 works, the envvar won't be visible. You'd need to inherit the system PATH in msys2 shells, which is discouraged.

      - name: Build
        run: |
          CMAKE_GENERATOR='Unix Makefiles'
          if [ "${{ matrix.org }}" = "windows" ]; then
            CMAKE_GENERATOR='MSYS Makefiles'
          fi
          cmake -B build -S . -G "$CMAKE_GENERATOR" ...

[ferdnyc]

  strategy:
    matrix:
      include:
        - { os: windows, shell: msys2 },
        - { os: ubuntu,  shell: bash  },
        - { os: macos,   shell: bash  },

Oooh, clevvvverrrr! If the ${{ matrix }} context is fair game for shell: arguments, then yeah that could be the ticket!

It kind of takes the "matrix" out of it, having to list each permutation individually, which is kind of too bad. (I currently have an 8-way matrix set up, four OS builds × the two compiler toolchains, and am considering adding more dimensions.) But, still, it's way more workable than anything I was trying! Thanks, this should be pretty much exactly what I need. I'll just have to get over my purity-of-configuration hangup. 😉

WRT selecting the CMake generator, I don't really understand why you are using an specific Action for that, given that all you need to do is set an environment variable. Unless the Action is aware of how MSYS2 works, the envvar won't be visible. You'd need to inherit the system PATH in msys2 shells, which is discouraged.

      - name: Build
        run: |
          CMAKE_GENERATOR='Unix Makefiles'
          if [ "${{ matrix.org }}" = "windows" ]; then
            CMAKE_GENERATOR='MSYS Makefiles'
          fi
          cmake -B build -S . -G "$CMAKE_GENERATOR" ...

Oh, sure, now that I'm conditionalizing the scripts anyway I can do that, but remember that originally I was trying to keep the scripts OS-agnostic.

Plus, having the selected generator available in a yaml context means that I could use it for other purposes.

I already use the matrix dimensions as part of the asset naming in my actions/upload-artifact steps, which can't access shell environment variables. My build assets from the runners get named 'Identifier-${{ matrix.os }}-${{ matrix.compiler }}', so they can all upload them separately without trampling each other. (I don't include the generator name because it's implicit in each combination of OS and compiler, but keeping it available in a yaml context as job metadata meant that option was open to me if I'd needed it.)

In fact, I stopped setting the CC environment variable from the job setup, because I realized I also had to configure CXX to match, and also set some additional flags for macOS as well. So currently that part reads much like your generator-selection:

- name: Build (Unix)
  if: ${{ runner.os != 'windows' }}
  run: |
    if [ "X${{ runner.os }}X${{ matrix.compiler }}X" == "XmacOSXclangX" ]; then
      export CMAKE_EXTRA="-DCMAKE_EXE_LINKER_FLAGS=-stdlib=libc++ -DCMAKE_SHARED_LINKER_FLAGS=-stdlib=libc++ -DCMAKE_OSX_DEPLOYMENT_TARGET=10.9"; fi
    if [ "x${{ matrix.compiler }}" == "xgcc" ]; then
      export CC=gcc CXX=g++; else export CC=clang CXX=clang++; fi
    cmake -B build -S . $(etc...) ${CMAKE_EXTRA}

(I currently have the Windows scripting split out to a separate job step, exactly the thing I was trying not to do because it's 90% identical to the Unix job step. But when I posted this it seemed unavoidable, so I surrendered. But now you've provided a way I can undo that duplication, with the relatively tiny cost being a bit more conditional logic in the build setup.)

Thanks again!

[ferdnyc]

In fact, I stopped setting the CC environment variable from the job setup, because I realized I also had to configure CXX to match, and also set some additional flags for macOS as well.

(Of course, thanks to your explicit-strategy-lanes idea, I could also be setting both compiler envvars that way...)

strategy:
  matrix:
    include:
        - { os: windows, shell: msys2, cc: gcc, cxx: g++ },
        - { os: windows, shell: msys2, cc: clang, cxx: clang++ },
        - { os: ubuntu,  shell: bash, cc: gcc, cxx: g++ },
        - { os: ubuntu,  shell: bash, cc: clang, cxx: clang++ },
        - { os: macos,   shell: bash, cc: gcc, cxx: g++ },
        - { os: macos,   shell: bash, cc: clang, cxx: clang++ },
    env:
      CC: ${{ matrix.cc }}
      CXX: ${{ matrix.cxx }}

[eine]

I agree that using matrices is not always as intuitive as it should be. Yet, I believe that main missing feature preventing proper code reuse is YAML anchors. That would make composing matrices, steps or jobs much easier.

Regarding the proposed solution, you read too fast and skipped the key concept! Pay attention: we replaced a value of type string with an object containing two fields of type string. The fact that I used include happened to be misleading for you. See the following rewrite of your last comment:

strategy:
  matrix:
    sys:
      - { os: windows, shell: msys2 },
      - { os: ubuntu,  shell: bash  },
      - { os: macos,   shell: bash  },
    comp:
      - { cc: gcc,   cxx: g++ },
      - { cc: clang, cxx: clang++ },
  env:
    CC: ${{ matrix.comp.cc }}
    CXX: ${{ matrix.comp.cxx }}
  runs-on: ${{ matrix.sys.os }}
  defaults:
    run:
      shell: ${{ matrix.sys.shell }} {0}

Moreover, you might want to set matrix, matrix.sys and/or matrix.comp dynamically in a previous step. See https://github.com/verilator/verilator/blob/master/.github/workflows/build.yml#L24-L51.

Overall, although not ideal, you can make pretty complex setups to be readable.

WRT setting environment variables, the point is not the syntax, or whether to have them defined as YAML values (that's ok). The point is that msys2 shells are "isolated" by default; i.e. variables from the environment are not inherited. Therefore, it's rather pointless to set them outside. The CC and CXX envvars in the code block above won't be seen by msys2. See https://github.com/msys2/setup-msys2#path-type and #98.

[eyal0]

@ferdnyc Regard the "matrix", you can still somewhat pull off the matrix but it'll be a little ugly:

strategy:
  matrix:
    os: [windows, ubuntu, macos], shell: msys2 ]
    include:
      - os: macos
        shell: bash
      - os: windows
        shell: msys2 {0}
      - os: ubuntu
        shell: bash

And then instead of ${{ matrix.sys.os }} use ${{ matrix.os }}. But I agree that it's kind of verbose. There's nothing much to do about that.

As for cross-platform builds, you'll have trouble with some environment variables. The correct way to add environment variable in one step for them to be used in another step is to add a line like FOO=BAR into $GITHUB_ENV. And the correct way to add to the PATH environment variable is to add a line into $GITHUB_PATH.

However, that modifies the environment variables in Windows. msys2 will inherit all all those environment variables into the bash environment except for PATH, LD_LIBRARY_PATH, PKG_CONFIG_PATH, and maybe others, too. So if you want to use GITHUB_ENV and GITHUB_PATH but have them work inside of msys, you need to kludge something. I wrote that kludge and it's horrific but it works. Here's how it looks:

        shell: |
          powershell -command "[System.Environment]::SetEnvironmentVariable('GITHUB_SCRIPT', ('{0}' -replace '\\','\\')); [System.Environment]::SetEnvironmentVariable('PKG_CONFIG_PATH_MSYS', $Env:PKG_CONFIG_PATH); [System.Environment]::SetEnvironmentVariable('PATH_MSYS', $Env:Path);"
          msys2 -c 'if [[ -v MSYSTEM ]]; then sed -i 1d $(cygpath $GITHUB_SCRIPT); sed -i \$d $(cygpath $GITHUB_SCRIPT); if [[ -v PKG_CONFIG_PATH_MSYS ]]; then export PKG_CONFIG_PATH=$PKG_CONFIG_PATH_MSYS; fi; if [[ -v PATH_MSYS && ! ( $PATH_MSYS =~ ^D:\\\\a\\\\_temp\\\\msys\\;C:\\\\Users ) ]]; then export PATH=$(echo $PATH_MSYS | sed s/D:\\\\\\\\a\\\\\\\\_temp\\\\\\\\msys\;// | sed s/\;/:/g); fi; fi; source $(cygpath $GITHUB_SCRIPT)'

It's in use currently in the actions: https://github.com/pcb2gcode/pcb2gcode/actions

I didn't think of @eine solution for specifying the shell in the matrix and I'm kicking myself for not realizing it! Wow! I think that with my above kludge along with the matrix shell variable, I think that I might be able to have a single CI that does both Windows and Linux. Wow! I'm excited to try it.

[ferdnyc]

@eine

Regarding the proposed solution, you read too fast and skipped the key concept! Pay attention: we replaced a value of type string with an object containing two fields of type string. The fact that I used include happened to be misleading for you.

No, trust me, no matter how slowly I'd read your response, I'd never have made it there all on my own. Not all the way from listing out combinations explicitly with include: (something I knew was possible, but had never thought of using that way) to augmenting the matrix with additional dimensions defined by complex objects, something I had no dea was even possible.

In fact, it feels almost like you shouldn't be able to do that. It certainly doesn't work for most other matrix: properties. (Try setting os to an object or list of objects, see how far you get.) Github's online workflow editor even red-squiggles compiler: in the following code, claiming it's a syntax error as, 'Matrix options must only contain primitive values'.

jobs:
  build:
    runs-on: ${{ matrix.os }}
    strategy:
      matrix:
        os: [windows-latest, macos-latest, ubuntu-latest]
        compiler:
          - { cc: gcc, cxx: g++ }
          - { cc: clang, cxx: clang++ }

Nevertheless, it does indeed work.

In fact, so does this (which I find even more confounding), and despite the roundabout approach to it, results in exactly the correct six-way matrix of two compiler suites per OS, with the correct shell being assigned to each OS:

jobs:
  # This workflow contains a single job called "build"
  build:
    # The type of runner that the job will run on
    runs-on: ${{ matrix.os }}
    strategy:
      matrix:
        os: [macos-latest, ubuntu-latest]
        shell: [bash]
        compiler:
          - { cc: gcc, cxx: g++ }
          - { cc: clang, cxx: clang++ }
        include:
          - os: windows-latest
            shell: 'msys2 {0}'
            compiler: { cc: gcc, cxx: g++ }
          - os: windows-latest
            shell: 'msys2 {0}'
            compiler: { cc: clang, cxx: clang++ }

...But Github screams really mightily about the syntax of the compiler: { ... } entries in the include: items, red-squiggling across that entire line of YAML. ("Invalid type found, one of string, number boolean were expected but an object was found")

And if you define the include:s without those compiler: objects, matrix.compiler ends up completely unset for the Windows-hosted jobs, which leads to a failed run.

But it's a daft thing to do anyway. It's certainly not any better than any of the previous iterations, and in fact it's significantly worse than some of them.

My point here isn't to present this as anything a sane person would use. I'm just illustrating that matrix syntax is not merely confusing, it's actually non-conformant to the officially-documented grammar and syntax checking. It's even possible that the syntax in question is exploiting a bug, not making use of a feature.

[ferdnyc]

In fact, it feels almost like you shouldn't be able to do that. It certainly doesn't work for most other matrix: properties. (Try setting os to an object or list of objects, see how far you get.)

Huh, I take that part back. Must've been some other syntax error, first time I tried it. In truth, even this works exactly the way you'd want it to. (But still gets flagged as a pair of syntax errors, by the editor.):

jobs:
  build:
    runs-on: '${{ matrix.os.os }}-latest'
    strategy:
      matrix:
        os:
          - { os: windows, shell: msys2 }
          - { os: ubuntu,  shell: bash  }
          - { os: macos,   shell: bash  }
        comp:
          - { cc: gcc,   cxx: g++ }
          - { cc: clang, cxx: clang++ }
    defaults:
      run:
        shell: '${{ matrix.os.shell }} {0}'
    env:
      CC: ${{ matrix.comp.cc }}
      CXX: ${{ matrix.comp.cxx }}

[ferdnyc]

Obligatory:

[eyal0]

Check it out, windows/macos/ubuntu in a unified list of GitHub workflow steps: #98 (comment)

[eine]

Github's online workflow editor even red-squiggles compiler: in the following code, claiming it's a syntax error as, 'Matrix options must only contain primitive values'.

...But Github screams really mightily about the syntax of the compiler: { ... } entries in the include: items, red-squiggling across that entire line of YAML. ("Invalid type found, one of string, number boolean were expected but an object was found")

I'm just illustrating that matrix syntax is not merely confusing, it's actually non-conformant to the officially-documented grammar and syntax checking. It's even possible that the syntax in question is exploiting a bug, not making use of a feature.

@ferdnyc, you might be correct thinking it's not a feature that GitHub expected to be used. However, I wouldn't call it a bug in the workflow, but in the documentation and/or the grammar/syntax checking. If the syntax is valid YAML and the runner can understand it, limiting it because missing semantic support in the checker is not sensible. I'd say, use it carefully, but don't expect workflow syntax checking to be complete/up to date.

Huh, I take that part back. Must've been some other syntax error, first time I tried it. In truth, even this works exactly the way you'd want it to. (But still gets flagged as a pair of syntax errors, by the editor.):

I forgot to remove trailing , in #102 (comment). Sorry about that.

[ferdnyc]

Check it out, windows/macos/ubuntu in a unified list of GitHub workflow steps: #98 (comment)

Same here (after I shake out some last bugs, tho, related to compilation failures on Windows, rather than workflow failures).

Oh, and to answer the question @eine asked a while back, I now remember why I was using action-cond to select the generator, and why I've now restored that step to my workflows: The value has a space in it. Getting the space to come through environment variable substitution intact is a giant pain in my a--. Whereas, just setting the value with this:

      - name: Select CMake generator
        uses: haya14busa/action-cond@v1
        id: generator
        with:
          cond: ${{ runner.os == 'Windows' }}
          if_true: 'MSYS Makefiles'
          if_false: 'Unix Makefiles'

and then dropping it directly into the run script as:

cmake -G "${{ steps.generator.outputs.value }}"

is simplicity itself. I am very lazy and very willing to take the easy route, on things like that. 😁

@eine

I wouldn't call it a bug in the workflow, but in the documentation and/or the grammar/syntax checking.

That's fair. And, I wasn't calling it a bug in the workflow exactly — rather, it may be a bug in their yaml processing, that it works when it's not intended to. No harm in taking advantage of it while it does! But because it's an undocumented, possibly-unintended feature, there's every possibility that a future update to the platform could "fix" the processing so that trick no longer works!

I definitely plan to keep taking advantage of it for as long as it does work, though. (Again: lazy.)

[SlySven]

🤔 Just a thought - but perhaps the syntax checker (that is red-underlining the unexpected object when a string/number/boolean only was expected) is itself defective and the grammar is fine?

https://github.com/yaml/yaml-grammar

[ferdnyc]

@SlySven More than possible; Were GitHub to fix the checker so that it stops flagging those configs as syntax errors, that would certainly convey at least tacit support for using the matrix configs that way, and I know I'd be a lot more confident in assuming that I can rely on it and that it's not going to suddenly stop working in a future update.

There's another thing they could do, though, to send that message even more clearly: document that syntax.

My main concern so far isn't just that the syntax checker flags those configs. It's that the documentation contains no indication that the matrix is intended to be used that way, and the checker flags it which "might" be a sign that it's not.

The documented functionality is ultimately a much more authoritative guide to what is and isn't officially supported. The syntax checker just provides some (very weak) hints as to what they might have intended, in the absence of officially documented support.

[ferdnyc]

(And, really, I think at this point they're stuck with supporting it whether they intended to or not, they'd break way too many workflows if they turned it off now.)