From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id C0A159D9FBC; Wed, 6 Mar 2024 23:34:43 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C0A159D9FBC DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1709757283; bh=bClxWSoa6EVU/KocPSItP1u12ZE+O8mFolPXiTlVkkY=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=pLiMhVWRT4GYib7fPUIG2ceyv9ir3dDziY9lb/MJ58qCvS1wmVC7BHobG2QRqSpVK GLf76BUjEmCW+nRSLBtM0iV8NBLA2nlNnpdusN96l4VLpPBCx4tSFyJfSfLreaBUGg 9+hR1pyct3p+BBk7HZh0LqU6pIpsZ1XORV/KZpo4= Received: from smtp17.i.mail.ru (smtp17.i.mail.ru [95.163.41.70]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 886CE9D9F93 for ; Wed, 6 Mar 2024 23:34:42 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 886CE9D9F93 Received: by smtp17.i.mail.ru with esmtpa (envelope-from ) id 1rhxyL-000000022cs-1t3g; Wed, 06 Mar 2024 23:34:42 +0300 Date: Wed, 6 Mar 2024 23:34:39 +0300 To: Sergey Kaplun Message-ID: References: <20240301082538.107373-1-m.kokryashkin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD987C0EE6E7F0A597DF2E6F6001F0923FF54E1DF110EA51DBE182A05F5380850404496DF0EF5067EB9F378A8CA21F699D6042960B90C3A9EBE868312D9CECC96AFDB2FAAB0A8254537 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE78BAADB77C21FF6F2EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637993F4D087AF024C68638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D853CB24C3402D2DFD7085ED22BF48AFEDB416B0BE051D4885CC7F00164DA146DAFE8445B8C89999728AA50765F7900637D0FEED2715E18529389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC82FFDA4F57982C5F4F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947CE753FA5741D1AD02302FCEF25BFAB3454AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C30CA335BCDFF8BE43BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF17B107DEF921CE791DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C3483320834B361D1F089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5349A0219238F19C55002B1117B3ED6964A476990A4C22471466072E6821086B3823CB91A9FED034534781492E4B8EEAD14747542773C033FBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFAD9331BFA688792946931113C7BAA7F586E25681A24558B7830F4B42E48E0AF3E547CA128C9EF438A2A143A52978B9DD63CA3102E86DEC3DFC04EE1E7FAFD6DBCD83847626F3D0AEC226CC413062362A913E6812662D5F2A54F6898A6FDCBDC72A617DFBE5FEC2C6383653B6C8D9AE0FD16FCAA6493B703A X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj9auBsoWJeY8U5FVihpBq+w== X-Mailru-Sender: 7940E2A4EB16C9976F80DCDCD59EC22780B105CC2F743675F378A8CA21F699D6042960B90C3A9EBEE2527C969975515CFF9FCECFB8D89CB6C77752E0C033A69E235A20A81F3B0E39AB3C5F247CB2F7F93A5DB60FBEB33A8A0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] ci: extend tarantool integration testing X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Maxim Kokryashkin via Tarantool-patches Reply-To: Maxim Kokryashkin Cc: Maxim Kokryashkin , tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 > > ----------------------------------------------------------------- > > > > > default_gcc_centos_7 | + | Ancient gcc build. > > Agree with Sergey here -- "gcc version (7)" looks better. Fixed. > > > > > 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 [*]. > > > > > static_build | + | Tarantool static build. > > static_build_cmake_linux | + | Tarantool static build. > > > > > [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. > > > > > +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 > 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 > > > > > +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 }} > > > > > + 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