Re: [Tarantool-patches] [PATCH luajit 3/3] ci: add Tarantool integration testing

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergey,

Thanks for your review!

On 23.06.22, Sergey Kaplun wrote:
> Hi, Igor!
> 
> Thanks for the patch!
> Nice to see our CI improving!

Glad this makes somebody a little happier :)

> 
> Please consider my comments below.
> 
> On 22.06.22, Igor Munkin wrote:
> > This patch introduces additional steps with Tarantool integration
> > testing to all existing LuaJIT workflows. All steps invoke reusable
> > workflow[1] from Tarantool repository for each matrix entry being set
> > for particular LuaJIT workflow.
> 
> AFAICS, there are no workflows for different OS like Fedora, FreeBSD,
> etc.. Same for integration testing workflow for Tarantool itself, plus
> workflows with different compilers (gcc/clang).
> 
> Should developer still bump LuaJIT in Tarantool repo and create PR to
> test full Tarantool CI?

Well, I'm trying my best to make tarantool/luajit CI as close to the one
in tarantool/tarantool as it's possible. However, I'm afraid we will
face two challenges soon:
1. As far as we're going to add some custom LuaJIT configurations
   testing and instrumenting testing to LuaJIT CI, it will expand
   already extensive Tarantool testing.
2. tarantool/tarantool is the main repository and we share runners with
   it, so we should not forget about already overloaded infrastructure.

All in all we need to create the sufficient set of workflows that will
alleviate LuaJIT developer suffering without dropping patches quality.

If you're asking about this particular patch, then yes, you still need
to create a draft PR in tarantool/tarantool to check if everything is
fine. By the way, I hear many opinions whether developers should use
'full-ci' tag for PRs or not. If you don't use it, you can just run all
pipelines in tarantool/luajit and leave everything else to a mainainer.
Otherwise, there is no other option than draft PR (I believe you can
create a single PR in tarantool/tarantool for a bunch of your patches,
but I'm not a big fan of it).

Anyway, here is the list for the upcoming enhancements:
* FreeBSD workflow
* Various compilers matrix (GCC, Clang)
* JIT enabled/disabled matrix * Valgrind testing
* luarocks v3 world testing

> 
> > 
> > [1]: https://github.com/tarantool/tarantool/blob/master/.github/workflows/luajit-integration.yml
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> >  .github/workflows/linux-aarch64.yml | 25 +++++++++++++++--
> >  .github/workflows/linux-x86_64.yml  | 43 +++++++++++++++++++++++++++--
> >  .github/workflows/macos-m1.yml      | 25 +++++++++++++++--
> >  .github/workflows/macos-x86_64.yml  | 43 +++++++++++++++++++++++++++--
> >  4 files changed, 124 insertions(+), 12 deletions(-)
> > 
> > diff --git a/.github/workflows/linux-aarch64.yml b/.github/workflows/linux-aarch64.yml
> > index de360b12..293b572e 100644
> > --- a/.github/workflows/linux-aarch64.yml
> > +++ b/.github/workflows/linux-aarch64.yml
> > @@ -1,4 +1,4 @@
> > -name: "LuaJIT test workflow (Linux/aarch64)"
> > +name: "Linux/aarch64 test workflow"
> >  
> >  on:
> >    push:
> > @@ -29,7 +29,7 @@ concurrency:
> >    cancel-in-progress: true
> >  
> >  jobs:
> > -  test-linux-aarch64:
> > +  test-luajit:
> >      runs-on: graviton
> >      strategy:
> >        fail-fast: false
> > @@ -40,7 +40,7 @@ jobs:
> >              CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
> >            - BUILDTYPE: Release
> >              CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
> > -    name: Linux/aarch64 ${{ matrix.BUILDTYPE }} GC64:ON
> > +    name: LuaJIT ${{ matrix.BUILDTYPE }} GC64:ON
> >      steps:
> >        - uses: actions/checkout@v2.3.4
> >          with:
> > @@ -56,3 +56,22 @@ jobs:
> >          run: cmake --build . --parallel $(($(nproc) + 1))
> >        - name: test
> >          run: cmake --build . --parallel $(($(nproc) + 1)) --target test
> > +
> > +  test-tarantool-debug-w-GC64:
> > +    name: Tarantool Debug GC64:ON
> > +    needs: test-luajit
> > +    uses: tarantool/tarantool/.github/workflows/luajit-integration.yml@master
> 
> I suggest to set one variable for this to reduce copy-paste.
> Also, why do you prefer copy workflows instead matrix usage?
> The only thing changes here -- buildtype.

Unfortunately, reusable workflow can't use test matrices, so we have no
other option than copy-paste this several times. I might be doing
something wrong, but I've failed with this exercise. You can try by
yourself, but now you're warned.

> 
> > +    with:
> > +      GC64: ON
> > +      buildtype: Debug
> > +      host: graviton
> > +      revision: ${{ github.sha }}
> > +  test-tarantool-release-w-GC64:
> > +    name: Tarantool Release GC64:ON
> > +    needs: test-luajit
> > +    uses: tarantool/tarantool/.github/workflows/luajit-integration.yml@master
> > +    with:
> > +      GC64: ON
> > +      buildtype: RelWithDebInfo
> > +      host: graviton
> > +      revision: ${{ github.sha }}
> > diff --git a/.github/workflows/linux-x86_64.yml b/.github/workflows/linux-x86_64.yml
> > index d6434afe..3eaf436d 100644
> > --- a/.github/workflows/linux-x86_64.yml
> > +++ b/.github/workflows/linux-x86_64.yml
> > @@ -1,4 +1,4 @@
> > -name: "LuaJIT test workflow (Linux/x86_64)"
> > +name: "Linux/x86_64 test workflow"
> >  
> >  on:
> >    push:
> > @@ -29,7 +29,7 @@ concurrency:
> >    cancel-in-progress: true
> >  
> >  jobs:
> > -  test-linux-x86_64:
> > +  test-luajit:
> >      runs-on: ubuntu-20.04-self-hosted
> >      strategy:
> >        fail-fast: false
> > @@ -41,7 +41,7 @@ jobs:
> >              CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
> >            - BUILDTYPE: Release
> >              CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
> > -    name: Linux/x86_64 ${{ matrix.BUILDTYPE }} GC64:${{ matrix.GC64 }}
> > +    name: LuaJIT ${{ matrix.BUILDTYPE }} GC64:${{ matrix.GC64 }}
> >      steps:
> >        - uses: actions/checkout@v2.3.4
> >          with:
> > @@ -57,3 +57,40 @@ jobs:
> >          run: cmake --build . --parallel $(($(nproc) + 1))
> >        - name: test
> >          run: cmake --build . --parallel $(($(nproc) + 1)) --target test
> > +
> > +  test-tarantool-debug-wo-GC64:
> > +    name: Tarantool Debug GC64:OFF
> > +    needs: test-luajit
> > +    uses: tarantool/tarantool/.github/workflows/luajit-integration.yml@master
> > +    with:
> > +      GC64: OFF
> 
> Same question here, but also with GC64 mode.

Ditto.

> 
> > +      buildtype: Debug
> > +      host: ubuntu-20.04-self-hosted
> > +      revision: ${{ github.sha }}
> > +  test-tarantool-debug-w-GC64:
> > +    name: Tarantool Debug GC64:ON
> > +    needs: test-luajit
> > +    uses: tarantool/tarantool/.github/workflows/luajit-integration.yml@master
> > +    with:
> > +      GC64: ON
> > +      buildtype: Debug
> > +      host: ubuntu-20.04-self-hosted
> > +      revision: ${{ github.sha }}
> > +  test-tarantool-release-wo-GC64:
> > +    name: Tarantool Release GC64:OFF
> > +    needs: test-luajit
> > +    uses: tarantool/tarantool/.github/workflows/luajit-integration.yml@master
> > +    with:
> > +      GC64: OFF
> > +      buildtype: RelWithDebInfo
> > +      host: ubuntu-20.04-self-hosted
> > +      revision: ${{ github.sha }}
> > +  test-tarantool-release-w-GC64:
> > +    name: Tarantool Release GC64:ON
> > +    needs: test-luajit
> > +    uses: tarantool/tarantool/.github/workflows/luajit-integration.yml@master
> > +    with:
> > +      GC64: ON
> > +      buildtype: RelWithDebInfo
> > +      host: ubuntu-20.04-self-hosted
> > +      revision: ${{ github.sha }}

<snipped>

> > -- 
> > 2.34.0
> > 
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM