Re: [Tarantool-patches] [PATCH luajit] ci: introduce GitHub Actions

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Igor!

Thanks for the patch!

LGTM (considering I'm not the specialist in GitHub-CI), except a few
nits and ignorable comments below:

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/

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

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

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

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

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

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

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

> diff --git a/.github/workflows/linux-x86_64.yml b/.github/workflows/linux-x86_64.yml
> new file mode 100644
> index 00000000..0f32e56d
> --- /dev/null
> +++ b/.github/workflows/linux-x86_64.yml

<snipped> as far as diffierence with the previous one is minimal.

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

> +
> +jobs:
> +  test-macos-m1:
> +    runs-on: macos-11-m1
> +    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:

Typo: /command/the command/

> +          #   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/
Typo? s/fails/failing/

> +          # if the package already exists with the previous version

Typo: s/version/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-x86_64.yml b/.github/workflows/macos-x86_64.yml
> new file mode 100644
> index 00000000..cbf8a285
> --- /dev/null
> +++ b/.github/workflows/macos-x86_64.yml
> @@ -0,0 +1,66 @@

<snipped> as far as diffierence with the previous one is minimal.

> -- 
> 2.34.0
> 

[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