Re: [Tarantool-patches] [PATCH luajit] ci: extend tarantool integration testing

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the review!

On Mon, Mar 04, 2024 at 07:45:14PM +0300, Sergey Kaplun wrote:
> Hi, Maxim!
> Thanks for the patch!
> Please consider my comments below.
>
> On 01.03.24, Maxim Kokryashkin wrote:
> > This patch makes use of reusable workflows from the
> > Tarantool repository, so now LuaJIT CI tests integration
> > in all relevant workflows.
> >
> > Also, this patch disables MacOS testing for Tarantool
> > integration since those runs were made nightly in the
> > Tarantool repo.
> >
> > As noted in the documentation[1], it is impossible for a single
>
> Typo: s/[1]/ [1]/
Fixed.
>
> > workflow call tree to include more than 20 workflows. To
> > overcome this limitation, workflows are started in a bunch of
> > separate files. This makes it impossible to depend on LuaJIT-only
> > jobs for integration workflow since it is impossible to make
> > cross-dependencies between workflow files.
> >
> > The name of a callee workflow cannot be substituted using a
> > matrix[2], so workflow calls are copied and pasted instead.
>
> Please add the corresponding comment to the workflow file too.
>
> Typo: s/[2]/ [2]/
Fixed.
>
> >
> > Table below shows which workflows are enabled and why.
> >
> > Workflow name                   |+/-| Reason
> > -----------------------------------------------------------------
>
> <snipped>
>
> > default_gcc_centos_7            | + | Ancient gcc build.
>
> Agree with Sergey here -- "gcc version (7)" looks better.
Fixed.
>
> <snipped>
>
> > luajit-integration              | + | Exotic LuaJIT options.
> > memtx_allocator_based_on_malloc | + | Not relevant to LuaJIT.
>
> Typo: s/+/-/
Fixed.
>
> > osx                             | - | Nightly build.
> > out_of_source                   | - | Not relevent to LuaJIT.
>
> I don't agree here. We want to check that our tests work in OOS builds
> too. It may be an issue for some libraries to be built or for some
> tests that use generated files, etc.
Enabled.
>
> If we enable this, we can skip static_build_cmake_linux workflow, as
> Sergey suggested (since it is the same as static_build, except it is OOS
> build). But I'm not against too leave that workflow as well.
Disabled static_build_cmake_linux.
>
> Also, looks like it is used, see below [*].
>
> <snipped>
>
> > static_build                    | + | Tarantool static build.
> > static_build_cmake_linux        | + | Tarantool static build.
>
> <snipped>
>
> > [1]: https://docs.github.com/en/actions/using-workflows/reusing-workflows#limitations
> > [2]: https://github.com/orgs/community/discussions/45342#discussioncomment-4779360
> > ---
> > Branch: https://github.com/tarantool/luajit/tree/fckxorg/integration-testing
> >
> >  .github/workflows/exotic-tarantool.yml    |  62 ++++++++++++
> >  .github/workflows/tarantool-ecosystem.yml |  35 +++++++
> >  .github/workflows/testing.yml             | 116 ++++++++++++++++------
> >  3 files changed, 183 insertions(+), 30 deletions(-)
> >  create mode 100644 .github/workflows/exotic-tarantool.yml
> >  create mode 100644 .github/workflows/tarantool-ecosystem.yml
> >
> > diff --git a/.github/workflows/exotic-tarantool.yml b/.github/workflows/exotic-tarantool.yml
> > new file mode 100644
> > index 00000000..be55291a
> > --- /dev/null
> > +++ b/.github/workflows/exotic-tarantool.yml
>
> Minor: I suggest using tarantool-exoting to be consistent with
> tarantool-ecosystem. Within this rename, don't forget to rename the job
> for consistency.
Renamed.
>
> OTOH, maybe it is better to name this workflow like the
> tarantool-integration or luajit-integration to show that we run
> **LuaJIT** tests under Tarantool. What name do you like the most?
I would leave it as it is, since the last time you've mentioned
that `tarantool-integration` is misleading. The other option
of name `luajit-integration` is even more inconsistent with the
contents of the other workflows. Honestly, this separation of
workflows is absolutely artificial and exists only because of
GitHub actions limitations.
>
> <snipped>
>
> > +jobs:
> > +  test-exotic-tarantool:
>
> Why do you separate this job from the testing.yml workflow? Also, it
> should depend on luajit-testing, so it still should be placed in the
> <testing.yml> shouldn't it? If not, please add a rationale in the
> comment.

As I've said in the commit message, it is impossible for workflow call
tree to have more than 20 nodes, so moving those to the testing.yml is
pointless. I've added the specific mention of this workflow to the
commit message and a comment here.
>
> > +    strategy:
> > +      fail-fast: false
> > +      matrix:
> > +        ARCH: [ARM64, x86_64]
> > +        BUILDTYPE: [Debug, Release]
> > +        GC64: [ON, OFF]
> > +        OS: [Linux, macOS]
> > +        include:
> > +          - BUILDTYPE: Debug
> > +            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug
> > +          - BUILDTYPE: Release
> > +            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
> > +        exclude:
> > +          - ARCH: ARM64
> > +            GC64: OFF
> > +          - OS: macOS
> > +            GC64: OFF
> > +    name: >
> > +      Tarantool
> > +      (${{ matrix.OS }}/${{ matrix.ARCH }})
> > +      ${{ matrix.BUILDTYPE }}
> > +      GC64:${{ matrix.GC64 }}
> > +    # XXX: Only LuaJIT-tests are running in this workflow.
>
> Side note: What do you think about adding the FreeBSD runs to that
> workflow?
> It doesn't cost much since LuaJIT-tests are tiny. And we already run
> such tests for OSX.
It's a quite rare situation for them to fail exclusively. Also, freebsd
builds are nightly, and because of that there is no integration support
for them on the Tarantool side, as we discussed with Igor and Yaroslav.
I think, we can add it later, if there will be need for that, but I
really doubt it.
>
> > +    uses: tarantool/tarantool/.github/workflows/luajit-integration.yml@master
> > +    with:
> > +      CMAKE_EXTRA_PARAMS: >
> > +        -G Ninja
> > +        ${{ matrix.CMAKEFLAGS }}
> > +        -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
> > +      arch: ${{ matrix.ARCH }}
> > +      os: ${{ matrix.OS }}
> > +      revision: ${{ github.sha }}
> > diff --git a/.github/workflows/tarantool-ecosystem.yml b/.github/workflows/tarantool-ecosystem.yml
> > new file mode 100644
> > index 00000000..82a47fe1
> > --- /dev/null
> > +++ b/.github/workflows/tarantool-ecosystem.yml
>
> <snipped>
>
> > +jobs:
> > +  test-tarantool-integration:
> > +    uses: tarantool/tarantool/.github/workflows/integration.yml@master
> > +    with:
> > +      submodule: luajit
> > +      revision: ${{ github.sha }}
>
> Do I understand correctly that this workflow is separated from testing
> integration list since it has reached the limit mentioned above (20
> workflows)? If so, please mention this here as a comment and in the
> testing.yml too.
Yep, you're right. Added a comment.
>
> > +
>
> Nit: Excess empty line.
Removed.
>
> > diff --git a/.github/workflows/testing.yml b/.github/workflows/testing.yml
> > index cb4ba57b..7bd527be 100644
> > --- a/.github/workflows/testing.yml
> > +++ b/.github/workflows/testing.yml
> > @@ -83,37 +83,93 @@ jobs:
> >          run: cmake --build . --parallel --target LuaJIT-test
> >          working-directory: ${{ env.BUILDDIR }}
> >
> > +  test-tarantool-coverage:
>
> Maybe it is better to add names for better readablility.
> Something like Tarantool intergration coverage, here and below.
Do we really need to repeat the name of the job once again
but without dashes? Ignoring.
>
> > +    needs: test-luajit
> > +    uses: tarantool/tarantool/.github/workflows/coverage.yml@master
> > +    with:
> > +      submodule: luajit
> > +      revision: ${{ github.sha }}
>
> <snipped>
>
> > +  test-tarantool-debug:
> > +    needs: test-luajit
> > +    uses: tarantool/tarantool/.github/workflows/debug.yml@master
> > +    with:
> > +      submodule: luajit
> > +      revision: ${{ github.sha }}
> > +
> > +  test-tarantool-debug_aarch64:
> > +    needs: test-luajit
> > +    uses: tarantool/tarantool/.github/workflows/debug_aarch64.yml@master
> > +    with:
> > +      submodule: luajit
> > +      revision: ${{ github.sha }}
> > +
> > +  test-tarantool-debug_asan_clang:
> > +    needs: test-luajit
> > +    uses: tarantool/tarantool/.github/workflows/debug_asan_clang.yml@master
> > +    with:
> > +      submodule: luajit
> > +      revision: ${{ github.sha }}
> > +
> > +  test-tarantool-default_gcc_centos_7:
> > +    needs: test-luajit
> > +    uses: tarantool/tarantool/.github/workflows/default_gcc_centos_7.yml@master
> > +    with:
> > +      submodule: luajit
> > +      revision: ${{ github.sha }}
> > +
> > +  test-tarantool-out_of_source:
> > +    needs: test-luajit
> > +    uses: tarantool/tarantool/.github/workflows/out_of_source.yml@master
> > +    with:
> > +      submodule: luajit
> > +      revision: ${{ github.sha }}
>
> [*] This workflow is mentioned as not used.
Now it is mentioned as used.
>
> > +
> > +  test-tarantool-release:
> > +    needs: test-luajit
> > +    uses: tarantool/tarantool/.github/workflows/release.yml@master
> > +    with:
> > +      submodule: luajit
> > +      revision: ${{ github.sha }}
> > +
> > +  test-tarantool-release_asan_clang:
> > +    needs: test-luajit
> > +    uses: tarantool/tarantool/.github/workflows/release_asan_clang.yml@master
> > +    with:
> > +      submodule: luajit
> > +      revision: ${{ github.sha }}
> > +
> > +  test-tarantool-release_clang:
> > +    needs: test-luajit
> > +    uses: tarantool/tarantool/.github/workflows/release_clang.yml@master
> > +    with:
> > +      submodule: luajit
> > +      revision: ${{ github.sha }}
> > +
> > +  test-tarantool-release_lto:
> > +    needs: test-luajit
> > +    uses: tarantool/tarantool/.github/workflows/release_lto.yml@master
> > +    with:
> > +      submodule: luajit
> > +      revision: ${{ github.sha }}
> > +
> > +  test-tarantool-release_lto_clang:
> > +    needs: test-luajit
> > +    uses: tarantool/tarantool/.github/workflows/release_lto_clang.yml@master
> > +    with:
> > +      submodule: luajit
> > +      revision: ${{ github.sha }}
> > +
> > +  test-tarantool-static_build:
> > +    needs: test-luajit
> > +    uses: tarantool/tarantool/.github/workflows/static_build.yml@master
> > +    with:
> > +      submodule: luajit
> > +      revision: ${{ github.sha }}
> > +
> > +  test-tarantool-static_build_cmake_linux:
> >      needs: test-luajit
> > -    uses: tarantool/tarantool/.github/workflows/luajit-integration.yml@master
> > +    uses: tarantool/tarantool/.github/workflows/static_build_cmake_linux.yml@master
> >      with:
> > -      CMAKE_EXTRA_PARAMS: >
> > -        -G Ninja
> > -        ${{ matrix.CMAKEFLAGS }}
> > -        -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
> > -      arch: ${{ matrix.ARCH }}
> > -      os: ${{ matrix.OS }}
> > +      submodule: luajit
> >        revision: ${{ github.sha }}
> > --
> > 2.43.0
> >
>
> --
> Best regards,
> Sergey Kaplun