Tarantool development patches archive
 help / color / mirror / Atom feed
From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit] ci: introduce GitHub Actions
Date: Wed, 2 Mar 2022 20:13:49 +0300	[thread overview]
Message-ID: <Yh+lzQUXntySwWZT@tarantool.org> (raw)
In-Reply-To: <Yh8iYTUH3fzpBEQh@root>

Sergey,

Thanks for your review! I hold your LGTM a bit, until I resolve all your
"nits" (that actually are quite vital comments to the patch).

On 02.03.22, Sergey Kaplun wrote:
> Hi, Igor!
> 
> Thanks for the patch!
> 
> LGTM (considering I'm not the specialist in GitHub-CI), except a few
> nits and ignorable comments below:

Considering the changes made below, here is the new commit message:
| ci: introduce GitHub Actions
|
| This patch introduces the following testing matrix for our LuaJIT fork:
|
| +---------------------+------------------+-------------+
| |                     |      Linux       |    macOS    |
| |                     +--------+---------+--------+----+
| |                     | x86_64 | aarch64 | x86_64 | M1 |
| +----------+----------+--------+---------+--------+----+
| |          | Release  |   +    |    +    |   +    | +  |
| + GC64 on  +----------+--------+---------+--------+----+
| |          | Debug(*) |   +    |    +    |   +    | +  |
| +----------+----------+--------+---------+--------+----+
| |          | Release  |   +    |    +    |   +    | +  |
| + GC64 off +----------+--------+---------+--------+----+
| |          | Debug(*) |   +    |    +    |   +    | +  |
| +----------+----------+--------+---------+--------+----+
|
| (*) Debug build also uses LuaJIT internal assertions.
|
| For each OS there is a separate workflow file, since it requires a
| OS-specific setup routine. For each column particular runner family is
| chosen based on the architecture name from <matrix.ARCH> options. For
| each row workflow toggles GC64 mode and build type flags and triggers
| the run with the chosen configuration options.
|
| There is one more workflow running only static analysis check. This
| division is caused by specific requirements to be installed (i.e.
| luarocks and luacheck). For other workflows <LuaJIT-luacheck> rule is
| just a noop step (see test/CMakeLists.txt for more info).
|
| All workflows are not triggered for the branches with "-notest" suffix
| and also "upstream-" prefix (but they are unlikely to be merged into the
| latter ones anyway). Nothing is also triggered for all tags.
|
| There is also one nit to be mentioned: actively running workflows on all
| branches except the long-term ones (i.e. tarantool, tarantool-1.10, etc)
| are canceled if "force push" occurs.

Here is the new CI results[1] (unfortunately, I was kinda in rage and
deleted the runs from the previous version... hope nobody needs them).

> 
> On 01.03.22, Igor Munkin wrote:
> > This patch introduces the following testing matrix for our LuaJIT fork:
> > 
> > +--------------------------------+------------------+-------------+
> > |                                |      Linux       |    macOS    |
> > |                                +--------+---------+--------+----+
> > |                                | x86_64 | aarch64 | x86_64 | M1 |
> > +---------------+----------------+--------+---------+--------+----+
> > |               | RelWithDebInfo |   +    |    +    |   +    | +  |
> > + GC64 disabled +----------------+--------+---------+--------+----+
> > |               | Debug(*)       |   +    |    +    |   +    | +  |
> > +---------------+----------------+--------+---------+--------+----+
> > |               | RelWithDebInfo |   +    |    +    |   +    | +  |
> > + GC64 enabled  +----------------+--------+---------+--------+----+
> > |               | Debug(*)       |   +    |    +    |   +    | +  |
> > +---------------+----------------+--------+---------+--------+----+
> > 
> > (*) Debug build also uses LuaJIT internal assertions.
> > 
> > For each column there is a separate workflow file, since it requires a
> > specific runner and setup routine. For each row workflow toggles GC64
> > mode and build type flags and triggers the run with the chosen
> > configuration options.
> > 
> > There is one more workflow running only static analysis check. This
> > division is caused by specific requirements to be installed (i.e.
> > luarocks and luacheck). For other workflows <LuaJIT-luacheck> rule is
> 
> Typo: s/other/others/

Nope, it's not. Ignoring.

> 
> > just a noop step (see test/CMakeLists.txt for more info).
> > 
> > All workflows are not triggered for the branches with "-notest" suffix
> > and also "upstream-" prefix (but they are unlikely to be merged into the
> > latter ones anyway). Nothing is also triggered for all tags.
> > 
> > There is also one nit to be mentioned: actively running workflows on all
> > branches except the long-term ones (i.e. tarantool, tarantool-1.10, etc)
> > are canceled if "force push" occurs.
> 
> Side note: This is great!
> 
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> >  .github/workflows/lint.yml          | 47 ++++++++++++++++++++
> >  .github/workflows/linux-aarch64.yml | 57 +++++++++++++++++++++++++
> >  .github/workflows/linux-x86_64.yml  | 57 +++++++++++++++++++++++++
> >  .github/workflows/macos-m1.yml      | 66 +++++++++++++++++++++++++++++
> >  .github/workflows/macos-x86_64.yml  | 66 +++++++++++++++++++++++++++++
> >  5 files changed, 293 insertions(+)
> >  create mode 100644 .github/workflows/lint.yml
> >  create mode 100644 .github/workflows/linux-aarch64.yml
> >  create mode 100644 .github/workflows/linux-x86_64.yml
> >  create mode 100644 .github/workflows/macos-m1.yml
> >  create mode 100644 .github/workflows/macos-x86_64.yml
> > 
> > diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
> > new file mode 100644
> > index 00000000..e0cb8eff
> > --- /dev/null
> > +++ b/.github/workflows/lint.yml
> > @@ -0,0 +1,47 @@
> > +name: "Static analysis"
> > +
> > +on:
> > +  push:
> > +    branches-ignore:
> > +      - '**-notest'
> > +      - 'upstream-**'
> > +    tags-ignore:
> > +      - '**'
> > +
> > +concurrency:
> > +  # Update of a developer branch cancels the previously scheduled
> 
> Typo: s/Update/An update/
> Here and below.

Fixed for all entries. Diff is below:

================================================================================

diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
index e0cb8eff..4d53171c 100644
--- a/.github/workflows/lint.yml
+++ b/.github/workflows/lint.yml
@@ -9,10 +9,10 @@ on:
       - '**'
 
 concurrency:
-  # Update of a developer branch cancels the previously scheduled
-  # workflow run for this branch. However, the default branch,
-  # and long-term branch (tarantool-1.10, tarantool-2.8, etc.)
-  # workflow runs are never canceled.
+  # An update of a developer branch cancels the previously
+  # scheduled workflow run for this branch. However, the default
+  # branch, and long-term branch (tarantool-1.10, tarantool-2.8,
+  # etc.) workflow runs are never canceled.
   #
   # We use a trick here: define the concurrency group as 'workflow
   # run ID' + # 'workflow run attempt' because it is a unique
diff --git a/.github/workflows/linux-aarch64.yml b/.github/workflows/linux-aarch64.yml
index a43d22ac..4799e8bc 100644
--- a/.github/workflows/linux-aarch64.yml
+++ b/.github/workflows/linux-aarch64.yml
@@ -9,10 +9,10 @@ on:
       - '**'
 
 concurrency:
-  # Update of a developer branch cancels the previously scheduled
-  # workflow run for this branch. However, the default branch,
-  # and long-term branch (tarantool-1.10, tarantool-2.8, etc.)
-  # workflow runs are never canceled.
+  # An update of a developer branch cancels the previously
+  # scheduled workflow run for this branch. However, the default
+  # branch, and long-term branch (tarantool-1.10, tarantool-2.8,
+  # etc.) workflow runs are never canceled.
   #
   # We use a trick here: define the concurrency group as 'workflow
   # run ID' + # 'workflow run attempt' because it is a unique
diff --git a/.github/workflows/linux-x86_64.yml b/.github/workflows/linux-x86_64.yml
index 0f32e56d..656fb515 100644
--- a/.github/workflows/linux-x86_64.yml
+++ b/.github/workflows/linux-x86_64.yml
@@ -9,10 +9,10 @@ on:
       - '**'
 
 concurrency:
-  # Update of a developer branch cancels the previously scheduled
-  # workflow run for this branch. However, the default branch,
-  # and long-term branch (tarantool-1.10, tarantool-2.8, etc.)
-  # workflow runs are never canceled.
+  # An update of a developer branch cancels the previously
+  # scheduled workflow run for this branch. However, the default
+  # branch, and long-term branch (tarantool-1.10, tarantool-2.8,
+  # etc.) workflow runs are never canceled.
   #
   # We use a trick here: define the concurrency group as 'workflow
   # run ID' + # 'workflow run attempt' because it is a unique
diff --git a/.github/workflows/macos-m1.yml b/.github/workflows/macos-m1.yml
index c34953be..b8c43bf6 100644
--- a/.github/workflows/macos-m1.yml
+++ b/.github/workflows/macos-m1.yml
@@ -9,10 +9,10 @@ on:
       - '**'
 
 concurrency:
-  # Update of a developer branch cancels the previously scheduled
-  # workflow run for this branch. However, the default branch,
-  # and long-term branch (tarantool-1.10, tarantool-2.8, etc.)
-  # workflow runs are never canceled.
+  # An update of a developer branch cancels the previously
+  # scheduled workflow run for this branch. However, the default
+  # branch, and long-term branch (tarantool-1.10, tarantool-2.8,
+  # etc.) workflow runs are never canceled.
   #
   # We use a trick here: define the concurrency group as 'workflow
   # run ID' + # 'workflow run attempt' because it is a unique
diff --git a/.github/workflows/macos-x86_64.yml b/.github/workflows/macos-x86_64.yml
index cbf8a285..911ee66c 100644
--- a/.github/workflows/macos-x86_64.yml
+++ b/.github/workflows/macos-x86_64.yml
@@ -9,10 +9,10 @@ on:
       - '**'
 
 concurrency:
-  # Update of a developer branch cancels the previously scheduled
-  # workflow run for this branch. However, the default branch,
-  # and long-term branch (tarantool-1.10, tarantool-2.8, etc.)
-  # workflow runs are never canceled.
+  # An update of a developer branch cancels the previously
+  # scheduled workflow run for this branch. However, the default
+  # branch, and long-term branch (tarantool-1.10, tarantool-2.8,
+  # etc.) workflow runs are never canceled.
   #
   # We use a trick here: define the concurrency group as 'workflow
   # run ID' + # 'workflow run attempt' because it is a unique

================================================================================

> 
> > +  # workflow run for this branch. However, the default branch,
> > +  # and long-term branch (tarantool-1.10, tarantool-2.8, etc.)
> > +  # workflow runs are never canceled.
> > +  #
> > +  # We use a trick here: define the concurrency group as 'workflow
> > +  # run ID' + # 'workflow run attempt' because it is a unique
> > +  # combination for any run. So it effectively discards grouping.
> > +  #
> > +  # XXX: we cannot use `github.sha` as a unique identifier because
> > +  # pushing a tag may cancel a run that works on a branch push
> > +  # event.
> > +  group: ${{ (
> > +    github.ref == 'refs/heads/tarantool' ||
> > +    startsWith(github.ref, 'refs/heads/tarantool-')) &&
> > +    format('{0}-{1}', github.run_id, github.run_attempt) ||
> > +    format('{0}-{1}', github.workflow, github.ref) }}
> > +  cancel-in-progress: true
> > +
> > +jobs:
> > +  test-luacheck:
> > +    runs-on: ubuntu-20.04-self-hosted
> > +    steps:
> > +      - uses: actions/checkout@v2.3.4
> > +        with:
> > +          fetch-depth: 0
> > +          submodules: recursive
> > +      - name: setup
> > +        run: |
> > +          sudo apt -y update
> > +          sudo apt -y install cmake make lua5.1 luarocks
> 
> Side note: Do we need to install Lua for luacheck? We can use LuaJIT
> (because it slightly faster).

Well... We both know the situations with LuaJIT distribution, so I
prefer to use Lua here even if we spend a little more time for
bootstrap. Ignoring.

> 
> | time lua /usr/bin/luacheck --codes --config ./.luacheckrc . | tail -n 1
> | Total: 0 warnings / 0 errors in 34 files
> |
> | real    0m0.236s
> | user    0m0.223s
> | sys     0m0.015s
> 
> | time luajit /usr/bin/luacheck --codes --config ./.luacheckrc . | tail -n 1
> | Total: 0 warnings / 0 errors in 34 files
> |
> | real    0m0.204s
> | user    0m0.176s
> | sys     0m0.030s
> 
> > +          sudo luarocks install luacheck
> > +      - name: configure
> > +        run: cmake .
> > +      - name: test
> > +        run: make -j LuaJIT-luacheck
> > diff --git a/.github/workflows/linux-aarch64.yml b/.github/workflows/linux-aarch64.yml
> > new file mode 100644
> > index 00000000..a43d22ac
> > --- /dev/null
> > +++ b/.github/workflows/linux-aarch64.yml
> > @@ -0,0 +1,57 @@
> > +name: "LuaJIT test workflow (Linux/AArch64)"
> 
> I suggest decrease the name here. It's too long. For example, I can't
> see the difference between these two jobs [1][2] without reading
> configuring section.

This is name of the workflow, not the job. Job naming issue is fixed
below (with your help). I prefer to leave workflow name untouched.
Ignoring.

> As for me, obviously that all run workflows are about LuaJIT testing, so
> we can just drop this part of the description. Linux-AArch64 looks nice.
> 
> > +
> > +on:
> > +  push:
> > +    branches-ignore:
> > +      - '**-notest'
> > +      - 'upstream-**'
> > +    tags-ignore:
> > +      - '**'
> > +
> > +concurrency:
> > +  # Update of a developer branch cancels the previously scheduled
> > +  # workflow run for this branch. However, the default branch,
> > +  # and long-term branch (tarantool-1.10, tarantool-2.8, etc.)
> > +  # workflow runs are never canceled.
> > +  #
> > +  # We use a trick here: define the concurrency group as 'workflow
> > +  # run ID' + # 'workflow run attempt' because it is a unique
> > +  # combination for any run. So it effectively discards grouping.
> > +  #
> > +  # XXX: we cannot use `github.sha` as a unique identifier because
> > +  # pushing a tag may cancel a run that works on a branch push
> > +  # event.
> > +  group: ${{ (
> > +    github.ref == 'refs/heads/tarantool' ||
> > +    startsWith(github.ref, 'refs/heads/tarantool-')) &&
> > +    format('{0}-{1}', github.run_id, github.run_attempt) ||
> > +    format('{0}-{1}', github.workflow, github.ref) }}
> > +  cancel-in-progress: true
> > +
> > +jobs:
> > +  test-linux-aarch64:
> > +    runs-on: graviton
> > +    strategy:
> > +      matrix:
> > +        BUILDTYPE:
> > +          - -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
> > +          - -DCMAKE_BUILD_TYPE=RelWithDebInfo
> > +        GC64:
> > +          - -DLUAJIT_ENABLE_GC64=ON
> > +          - -DLUAJIT_ENABLE_GC64=OFF
> > +
> > +    steps:
> > +      - uses: actions/checkout@v2.3.4
> > +        with:
> > +          fetch-depth: 0
> > +          submodules: recursive
> > +      - name: setup
> > +        run: |
> > +          sudo apt -y update
> > +          sudo apt -y install cmake gcc make perl
> 
> Side note: Will we add clang compiler in future CI jobs?

I guess we will later (e.g. when sanitizers support is introduced).

> 
> > +      - name: configure
> > +        run: cmake . ${{ matrix.BUILDTYPE }} ${{ matrix.GC64 }}
> 
> When we run Debug job, we know, that it always sets -DLUA_USE_ASSERT=ON
> -DLUA_USE_APICHECK=ON in cmake flags. Can we use just change names
> Debug/Release and GC64-ON/GC64-OFF for matrix entries and provide the
> necessary information in a build via matrix environment variables[3]?
> 
> So instead dreaded job name
> | test-linux-aarch64 (-DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON -DLUAJIT_ENABLE_GC64=ON)
> 
> we will have (AFAIU)
> | test-linux-aarch64 (Debug GC64-ON)
> 
> IMO, it makes easier finding job we are interested in.

Thanks a lot for your inputs and help (include section makes workflow
file much more fancy): I've fixed the workflow the way you've suggested.
I also change 'runs-on' value to the template, so we can choose the
runner respecting the <strategy.matrix.ARCH> field. As a result amount
of workflow files are reduced by two. Diff is below:

================================================================================

diff --git a/.github/workflows/linux-x86_64.yml b/.github/workflows/linux-x86_64.yml
deleted file mode 100644
index 656fb515..00000000
--- a/.github/workflows/linux-x86_64.yml
+++ /dev/null
@@ -1,57 +0,0 @@
-name: "LuaJIT test workflow (Linux/x86_64)"
-
-on:
-  push:
-    branches-ignore:
-      - '**-notest'
-      - 'upstream-**'
-    tags-ignore:
-      - '**'
-
-concurrency:
-  # An update of a developer branch cancels the previously
-  # scheduled workflow run for this branch. However, the default
-  # branch, and long-term branch (tarantool-1.10, tarantool-2.8,
-  # etc.) workflow runs are never canceled.
-  #
-  # We use a trick here: define the concurrency group as 'workflow
-  # run ID' + # 'workflow run attempt' because it is a unique
-  # combination for any run. So it effectively discards grouping.
-  #
-  # XXX: we cannot use `github.sha` as a unique identifier because
-  # pushing a tag may cancel a run that works on a branch push
-  # event.
-  group: ${{ (
-    github.ref == 'refs/heads/tarantool' ||
-    startsWith(github.ref, 'refs/heads/tarantool-')) &&
-    format('{0}-{1}', github.run_id, github.run_attempt) ||
-    format('{0}-{1}', github.workflow, github.ref) }}
-  cancel-in-progress: true
-
-jobs:
-  test-linux-x86_64:
-    runs-on: ubuntu-20.04-self-hosted
-    strategy:
-      matrix:
-        BUILDTYPE:
-          - -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
-          - -DCMAKE_BUILD_TYPE=RelWithDebInfo
-        GC64:
-          - -DLUAJIT_ENABLE_GC64=ON
-          - -DLUAJIT_ENABLE_GC64=OFF
-
-    steps:
-      - uses: actions/checkout@v2.3.4
-        with:
-          fetch-depth: 0
-          submodules: recursive
-      - name: setup
-        run: |
-          sudo apt -y update
-          sudo apt -y install cmake gcc make perl
-      - name: configure
-        run: cmake . ${{ matrix.BUILDTYPE }} ${{ matrix.GC64 }}
-      - name: build
-        run: make -j
-      - name: test
-        run: make -j test
diff --git a/.github/workflows/linux-aarch64.yml b/.github/workflows/linux.yml
similarity index 65%
rename from .github/workflows/linux-aarch64.yml
rename to .github/workflows/linux.yml
index 4799e8bc..54500f6e 100644
--- a/.github/workflows/linux-aarch64.yml
+++ b/.github/workflows/linux.yml
@@ -1,4 +1,4 @@
-name: "LuaJIT test workflow (Linux/AArch64)"
+name: "LuaJIT test workflow (Linux)"
 
 on:
   push:
@@ -29,17 +29,23 @@ concurrency:
   cancel-in-progress: true
 
 jobs:
-  test-linux-aarch64:
-    runs-on: graviton
+  test-linux:
+    runs-on: ${{ matrix.RUNNER }}
     strategy:
       matrix:
-        BUILDTYPE:
-          - -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
-          - -DCMAKE_BUILD_TYPE=RelWithDebInfo
-        GC64:
-          - -DLUAJIT_ENABLE_GC64=ON
-          - -DLUAJIT_ENABLE_GC64=OFF
-
+        ARCH: [x86_64, aarch64]
+        BUILDTYPE: [Debug, Release]
+        GC64: [ON, OFF]
+        include:
+          - ARCH: x86_64
+            RUNNER: ubuntu-20.04-self-hosted
+          - ARCH: aarch64
+            RUNNER: graviton
+          - BUILDTYPE: Debug
+            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
+          - BUILDTYPE: Release
+            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
+    name: Linux/${{ matrix.ARCH }} ${{ matrix.BUILDTYPE }} GC64:${{ matrix.GC64 }}
     steps:
       - uses: actions/checkout@v2.3.4
         with:
@@ -50,7 +56,7 @@ jobs:
           sudo apt -y update
           sudo apt -y install cmake gcc make perl
       - name: configure
-        run: cmake . ${{ matrix.BUILDTYPE }} ${{ matrix.GC64 }}
+        run: cmake . ${{ matrix.CMAKEFLAGS }} -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
       - name: build
         run: make -j
       - name: test
diff --git a/.github/workflows/macos-x86_64.yml b/.github/workflows/macos-x86_64.yml
deleted file mode 100644
index 911ee66c..00000000
--- a/.github/workflows/macos-x86_64.yml
+++ /dev/null
@@ -1,66 +0,0 @@
-name: "LuaJIT test workflow (macOS/x86_64)"
-
-on:
-  push:
-    branches-ignore:
-      - '**-notest'
-      - 'upstream-**'
-    tags-ignore:
-      - '**'
-
-concurrency:
-  # An update of a developer branch cancels the previously
-  # scheduled workflow run for this branch. However, the default
-  # branch, and long-term branch (tarantool-1.10, tarantool-2.8,
-  # etc.) workflow runs are never canceled.
-  #
-  # We use a trick here: define the concurrency group as 'workflow
-  # run ID' + # 'workflow run attempt' because it is a unique
-  # combination for any run. So it effectively discards grouping.
-  #
-  # XXX: we cannot use `github.sha` as a unique identifier because
-  # pushing a tag may cancel a run that works on a branch push
-  # event.
-  group: ${{ (
-    github.ref == 'refs/heads/tarantool' ||
-    startsWith(github.ref, 'refs/heads/tarantool-')) &&
-    format('{0}-{1}', github.run_id, github.run_attempt) ||
-    format('{0}-{1}', github.workflow, github.ref) }}
-  cancel-in-progress: true
-
-jobs:
-  test-macos-x86_64:
-    runs-on: macos-11
-    strategy:
-      matrix:
-        BUILDTYPE:
-          - -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
-          - -DCMAKE_BUILD_TYPE=RelWithDebInfo
-        GC64:
-          - -DLUAJIT_ENABLE_GC64=ON
-          - -DLUAJIT_ENABLE_GC64=OFF
-
-    steps:
-      - uses: actions/checkout@v2.3.4
-        with:
-          fetch-depth: 0
-          submodules: recursive
-      - name: setup
-        run: |
-          # Install brew using command from Homebrew repository instructions:
-          #   https://github.com/Homebrew/install
-          # XXX: 'echo' command below is required since brew installation
-          # script obliges the one to enter a newline for confirming the
-          # installation via Ruby script.
-          brew update ||
-            echo | /usr/bin/ruby -e "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/master/install)"
-          # try to install the packages either upgrade it to avoid of fails
-          # if the package already exists with the previous version
-          brew install --force cmake gcc make perl ||
-            brew upgrade cmake gcc make perl
-      - name: configure
-        run: cmake . ${{ matrix.BUILDTYPE }} ${{ matrix.GC64 }}
-      - name: build
-        run: make -j
-      - name: test
-        run: make -j test
diff --git a/.github/workflows/macos-m1.yml b/.github/workflows/macos.yml
similarity index 74%
rename from .github/workflows/macos-m1.yml
rename to .github/workflows/macos.yml
index b8c43bf6..fa770650 100644
--- a/.github/workflows/macos-m1.yml
+++ b/.github/workflows/macos.yml
@@ -1,4 +1,4 @@
-name: "LuaJIT test workflow (macOS/M1)"
+name: "LuaJIT test workflow (macOS)"
 
 on:
   push:
@@ -29,17 +29,23 @@ concurrency:
   cancel-in-progress: true
 
 jobs:
-  test-macos-m1:
-    runs-on: macos-11-m1
+  test-macos:
+    runs-on: ${{ matrix.RUNNER }}
     strategy:
       matrix:
-        BUILDTYPE:
-          - -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
-          - -DCMAKE_BUILD_TYPE=RelWithDebInfo
-        GC64:
-          - -DLUAJIT_ENABLE_GC64=ON
-          - -DLUAJIT_ENABLE_GC64=OFF
-
+        ARCH: [x86_64, m1]
+        BUILDTYPE: [Debug, Release]
+        GC64: [ON, OFF]
+        include:
+          - ARCH: x86_64
+            RUNNER: macos-11
+          - ARCH: m1
+            RUNNER: macos-11-m1
+          - BUILDTYPE: Debug
+            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
+          - BUILDTYPE: Release
+            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
+    name: macOS/${{ matrix.ARCH }} ${{ matrix.BUILDTYPE }} GC64:${{ matrix.GC64 }}
     steps:
       - uses: actions/checkout@v2.3.4
         with:
@@ -59,7 +65,7 @@ jobs:
           brew install --force cmake gcc make perl ||
             brew upgrade cmake gcc make perl
       - name: configure
-        run: cmake . ${{ matrix.BUILDTYPE }} ${{ matrix.GC64 }}
+        run: cmake . ${{ matrix.CMAKEFLAGS }} -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
       - name: build
         run: make -j
       - name: test

================================================================================

> 
> > +      - name: build
> > +        run: make -j
> > +      - name: test
> > +        run: make -j test
> 
> IINM, as far as strategy.fail-fast[4] field is set in `true` by default,
> if one job from matrix fails, the others will be canceled. IMO, it's
> inconvenient to stop all jobs if Debug-GC64 job is failed, so I suggest to
> set this value to `false` manually.

I didn't know this option (and such behaviour consequence for the matrix
strategy). Fixed. Diff is below:

================================================================================

diff --git a/.github/workflows/linux.yml b/.github/workflows/linux.yml
index 54500f6e..7e77a93a 100644
--- a/.github/workflows/linux.yml
+++ b/.github/workflows/linux.yml
@@ -32,6 +32,7 @@ jobs:
   test-linux:
     runs-on: ${{ matrix.RUNNER }}
     strategy:
+      fail-fast: false
       matrix:
         ARCH: [x86_64, aarch64]
         BUILDTYPE: [Debug, Release]
diff --git a/.github/workflows/macos.yml b/.github/workflows/macos.yml
index fa770650..4a8b5893 100644
--- a/.github/workflows/macos.yml
+++ b/.github/workflows/macos.yml
@@ -32,6 +32,7 @@ jobs:
   test-macos:
     runs-on: ${{ matrix.RUNNER }}
     strategy:
+      fail-fast: false
       matrix:
         ARCH: [x86_64, m1]
         BUILDTYPE: [Debug, Release]

================================================================================

> 
> Also, you may notice, that all workflows are quite similar. They differ
> by name, 'runs-on' field and (for macOS/Linux) setup step.
> 
> So maybe we can use matrix to cover all these workflows as showing
> here[5], using environment variables to determine setup step for the
> particular OS (macOS or Linux)?

Setup steps differs a lot on macOS and Linux, and moving everything into
<strategy.matrix.include> section makes workflow file totally
unreadable. Ignoring.

> 
> But maybe I am overcomplicating things, so feel free to ignore. :)

Now workflow is much prettier, thanks a lot for your inputs and help!

> 

<snipped>

> > diff --git a/.github/workflows/macos-m1.yml b/.github/workflows/macos-m1.yml
> > new file mode 100644
> > index 00000000..c34953be
> > --- /dev/null
> > +++ b/.github/workflows/macos-m1.yml
> > @@ -0,0 +1,66 @@
> > +name: "LuaJIT test workflow (macOS/M1)"

<snipped>

> > +      - name: setup
> > +        run: |
> > +          # Install brew using command from Homebrew repository instructions:
> 
> Typo: /command/the command/

Fixed. Diff is below:

================================================================================

diff --git a/.github/workflows/macos.yml b/.github/workflows/macos.yml
index 4a8b5893..711a8bd9 100644
--- a/.github/workflows/macos.yml
+++ b/.github/workflows/macos.yml
@@ -54,8 +54,8 @@ jobs:
           submodules: recursive
       - name: setup
         run: |
-          # Install brew using command from Homebrew repository instructions:
-          #   https://github.com/Homebrew/install
+          # Install brew using the command from Homebrew repository
+          # instructions: https://github.com/Homebrew/install.
           # XXX: 'echo' command below is required since brew installation
           # script obliges the one to enter a newline for confirming the
           # installation via Ruby script.

================================================================================

> 
> > +          #   https://github.com/Homebrew/install
> > +          # XXX: 'echo' command below is required since brew installation
> > +          # script obliges the one to enter a newline for confirming the
> > +          # installation via Ruby script.
> > +          brew update ||
> > +            echo | /usr/bin/ruby -e "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/master/install)"
> > +          # try to install the packages either upgrade it to avoid of fails
> 
> Typo: s/try to/Try to/

Fixed. Diff is below:

================================================================================

diff --git a/.github/workflows/macos.yml b/.github/workflows/macos.yml
index 711a8bd9..1299cfd4 100644
--- a/.github/workflows/macos.yml
+++ b/.github/workflows/macos.yml
@@ -61,7 +61,7 @@ jobs:
           # installation via Ruby script.
           brew update ||
             echo | /usr/bin/ruby -e "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/master/install)"
-          # try to install the packages either upgrade it to avoid of fails
+          # Try to install the packages either upgrade it to avoid of fails
           # if the package already exists with the previous version
           brew install --force cmake gcc make perl ||
             brew upgrade cmake gcc make perl

================================================================================

> Typo? s/fails/failing/

Nope, it's not. Ignoring.

> 
> > +          # if the package already exists with the previous version
> 
> Typo: s/version/version./

Fixed. Diff is below:

================================================================================

diff --git a/.github/workflows/macos.yml b/.github/workflows/macos.yml
index 1299cfd4..45908ff2 100644
--- a/.github/workflows/macos.yml
+++ b/.github/workflows/macos.yml
@@ -62,7 +62,7 @@ jobs:
           brew update ||
             echo | /usr/bin/ruby -e "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/master/install)"
           # Try to install the packages either upgrade it to avoid of fails
-          # if the package already exists with the previous version
+          # if the package already exists with the previous version.
           brew install --force cmake gcc make perl ||
             brew upgrade cmake gcc make perl
       - name: configure

================================================================================

> 

<snipped>

> 
> [1]: https://github.com/tarantool/luajit/runs/5375573436?check_suite_focus=true
> [2]: https://github.com/tarantool/luajit/runs/5375573350?check_suite_focus=true
> [3]: https://docs.github.com/en/actions/using-jobs/using-a-build-matrix-for-your-jobs#using-environment-variables-in-a-matrix
> [4]: https://docs.github.com/en/actions/using-jobs/using-a-build-matrix-for-your-jobs#canceling-remaining-jobs-if-a-matrix-job-fails
> [5]: https://docs.github.com/en/actions/using-jobs/using-a-build-matrix-for-your-jobs#example-running-with-multiple-operating-systems
> 
> -- 
> Best regards,
> Sergey Kaplun

[1]: https://github.com/tarantool/luajit/commit/ef0c6ce

-- 
Best regards,
IM

  reply	other threads:[~2022-03-02 17:17 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-01 13:15 Igor Munkin via Tarantool-patches
2022-03-02  7:53 ` Sergey Kaplun via Tarantool-patches
2022-03-02 17:13   ` Igor Munkin via Tarantool-patches [this message]
2022-03-03  8:33     ` Sergey Kaplun via Tarantool-patches
2022-03-02 17:14   ` Maxim Kokryashkin via Tarantool-patches
2022-03-02 17:27     ` Igor Munkin via Tarantool-patches
2022-03-04 14:11       ` Igor Munkin via Tarantool-patches
2022-03-04 11:55 ` Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Yh+lzQUXntySwWZT@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit] ci: introduce GitHub Actions' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox