[GitHub] [daffodil] tuxji commented on a diff in pull request #883: Add macOS runner

[GitBox <gi...@apache.org>]

tuxji commented on code in PR #883:
URL: https://github.com/apache/daffodil/pull/883#discussion_r1035466437


##########
.github/workflows/main.yml:
##########
@@ -38,6 +42,13 @@ jobs:
         scala_version: [ 2.12.17 ]
         os: [ ubuntu-20.04, windows-2019 ]
         include:
+          - os: macos-12
+            shell: bash
+            distribution: temurin
+            java_version: 17
+            scala_version: 2.12.17
+            env_cc: cc
+            env_ar: ar

Review Comment:
   FYI, the reason why we used include was because we couldn't add macos-12 to `os: [ ubuntu-20.04, windows-2019 ]` without also getting `java_version: [ 8, 11, 17 ]` combinations as well, and we found out the rest of the variables would be empty if we didn't define them in include as well.
   
   However, I've just noticed that we can use [exclude](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstrategymatrixexclude) which could be more concise (we'd need only 4 exclude lines instead of 7 include lines, although we'd have to define env_cc, env_ar, shell in the matrix itself too).  I guess it's a matter of taste, but here goes anyway:
   
   ```yaml
         matrix:
           os: [ macos-12, ubuntu-20.04, windows-2019 ]
           distribution: [ temurin ]
           java_version: [ 8, 11, 17 ]
           env_cc: [ clang ]
           env_ar: [ ar ]
           scala_version: [ 2.12.17 ]
           shell: [ bash ]
           exclude:
             - os: macos-12
               java_version: 8
             - os: macos-12
               java_version: 11
           include:
             - os: ubuntu-20.04
               env_ar: llvm-ar-10
             - os: windows-2019
               env_ar: llvm-ar
               shell: msys2 {0}
   ```
   
   We can go with the above if it looks better; it does seem to be more concise.



##########
.github/workflows/main.yml:
##########
@@ -24,6 +24,10 @@ on:
   pull_request:
     types: [opened, synchronize, reopened]
 
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref || github.run_id }}
+  cancel-in-progress: true

Review Comment:
   You can define a [concurrency](https://docs.github.com/en/actions/using-jobs/using-concurrency) group to ensure that only a single workflow using the same concurrency group will run at a time.  That is, if you update your pull request after a failed CI check, GitHub might as well cancel the last workflow since you usually don't care about the last workflow's remaining CI checks, only whether the next workflow's CI checks pass.
   
   By the way, I saw a StackOverflow [post](https://stackoverflow.com/questions/70928424/limit-github-action-workflow-concurrency-on-push-and-pull-request) that says `group: ${{ github.workflow }}-${{ github.head_ref || github.ref_name }}` would work better for both pull requests and pushes (e.g., pushes to one's personal fork of the Apache Daffodil repo where one has enabled Actions) than `group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}` since head_ref is empty for pushes and run_id is always unique.  We're using github.ref above which is simply a longer version of github.ref_name and I believe github.ref would work for both pull requests and pushes, but let's go with `group: ${{ github.workflow }}-${{ github.head_ref || github.ref_name }}` to avoid falling back on run_id in any case.  Here's more [documentation](https://docs.github.com/en/actions/learn-github-actions/contexts) about the github context.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org