[Tarantool-patches] [PATCH luajit] ci: make GitHub workflows more CMake-ish

[Tarantool-patches] [PATCH luajit] ci: make GitHub workflows more CMake-ish

[Igor Munkin via Tarantool-patches]

Using explicit <make> commands makes changing the build system generator
to another one (e.g. Ninja) too verbose. Fortunately, we can use
generator-agnostic command provided by CMake, so the only spot where
generator name occurs is the configuration step of GitHub workflow
(UNIX Makefiles are generated for UNIX-like platforms by default).

Signed-off-by: Igor Munkin <imun@tarantool.org>
---

Branch: https://github.com/tarantool/luajit/tree/imun/cmake-ish-workflow
CI: https://github.com/tarantool/luajit/commit/8826142
Tarantool CI results ares excess, since the changeset relates only to
tarantool/luajit GitHub Actions.

 .github/workflows/lint.yml          | 2 +-
 .github/workflows/linux-aarch64.yml | 4 ++--
 .github/workflows/linux-x86_64.yml  | 4 ++--
 .github/workflows/macos-m1.yml      | 4 ++--
 .github/workflows/macos-x86_64.yml  | 4 ++--
 5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
index 9e7cc1f1..ff4b0ac2 100644
--- a/.github/workflows/lint.yml
+++ b/.github/workflows/lint.yml
@@ -45,4 +45,4 @@ jobs:
       - name: configure
         run: cmake .
       - name: test
-        run: make -j LuaJIT-luacheck
+        run: cmake --build . -j -t LuaJIT-luacheck
diff --git a/.github/workflows/linux-aarch64.yml b/.github/workflows/linux-aarch64.yml
index 09b85b56..2f62394f 100644
--- a/.github/workflows/linux-aarch64.yml
+++ b/.github/workflows/linux-aarch64.yml
@@ -54,6 +54,6 @@ jobs:
       - name: configure
         run: cmake . ${{ matrix.CMAKEFLAGS }} -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
       - name: build
-        run: make -j
+        run: cmake --build . -j
       - name: test
-        run: make -j test
+        run: cmake --build . -j -t test
diff --git a/.github/workflows/linux-x86_64.yml b/.github/workflows/linux-x86_64.yml
index a1e3a618..4e6e3d9b 100644
--- a/.github/workflows/linux-x86_64.yml
+++ b/.github/workflows/linux-x86_64.yml
@@ -54,6 +54,6 @@ jobs:
       - name: configure
         run: cmake . ${{ matrix.CMAKEFLAGS }} -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
       - name: build
-        run: make -j
+        run: cmake --build . -j
       - name: test
-        run: make -j test
+        run: cmake --build . -j -t test
diff --git a/.github/workflows/macos-m1.yml b/.github/workflows/macos-m1.yml
index eb822142..a272e39b 100644
--- a/.github/workflows/macos-m1.yml
+++ b/.github/workflows/macos-m1.yml
@@ -68,6 +68,6 @@ jobs:
       - name: configure
         run: ${ARCH} cmake . ${{ matrix.CMAKEFLAGS }} -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
       - name: build
-        run: ${ARCH} make -j
+        run: ${ARCH} cmake --build . -j
       - name: test
-        run: ${ARCH} make -j test
+        run: ${ARCH} cmake --build . -j -t test
diff --git a/.github/workflows/macos-x86_64.yml b/.github/workflows/macos-x86_64.yml
index 09f391db..d639e872 100644
--- a/.github/workflows/macos-x86_64.yml
+++ b/.github/workflows/macos-x86_64.yml
@@ -63,6 +63,6 @@ jobs:
       - name: configure
         run: cmake . ${{ matrix.CMAKEFLAGS }} -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
       - name: build
-        run: make -j
+        run: cmake --build . -j
       - name: test
-        run: make -j test
+        run: cmake --build . -j -t test
-- 
2.34.0

Re: [Tarantool-patches] [PATCH luajit] ci: make GitHub workflows more CMake-ish

[Sergey Kaplun via Tarantool-patches]

Hi, Igor!
Thanks for the patch!
LGTM.

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] ci: make GitHub workflows more CMake-ish

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for your review!

On 02.06.22, Sergey Kaplun wrote:
> Hi, Igor!
> Thanks for the patch!
> LGTM.

Added your tag:
| Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>

> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit] ci: make GitHub workflows more CMake-ish

[Sergey Bronnikov via Tarantool-patches]

Igor, thanks for the patch!

See my comments below.

1. I propose to use long-options for clarity. In particular I mean 
long-option for "-j", it is better to replace it with "--parallel", see [1].

2. I propose to define a number of threads explicitly, because Ninja and 
Make have different meaning for -j without a number of threads:

Ninja:

       -j N   run N jobs in parallel (0 means infinity) [default=#CPUs]

Make:

        -j [jobs], --jobs[=jobs]
             Specifies the number of jobs (commands) to run 
simultaneously.  If
             there is more than one -j option, the last one is 
effective.  If
             the -j option is given without an argument, make will not limit
             the number of jobs that can run simultaneously. When make 
invokes
             a sub-make, all instances of make will coordinate to run the
             specified number of jobs at a time; see the section 
PARALLEL MAKE
             AND THE JOBSERVER for details.

It is recommended to use a number (CPU cores + 1), so we can explicitly 
set "$(nproc) + 1".

There is a CMake option "" [2], but we couldn't use it as it is 
available since CMake 3.12,

when LuaJIT build system has a minimal version only 3.1.


1. https://cmake.org/cmake/help/v3.12/manual/cmake.1.html#build-tool-mode

2. 
https://cmake.org/cmake/help/latest/envvar/CMAKE_BUILD_PARALLEL_LEVEL.html

Sergey

On 01.06.2022 18:54, Igor Munkin wrote:
> Using explicit <make> commands makes changing the build system generator
> to another one (e.g. Ninja) too verbose. Fortunately, we can use
> generator-agnostic command provided by CMake, so the only spot where
> generator name occurs is the configuration step of GitHub workflow
> (UNIX Makefiles are generated for UNIX-like platforms by default).
>
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
>
> Branch: https://github.com/tarantool/luajit/tree/imun/cmake-ish-workflow
> CI: https://github.com/tarantool/luajit/commit/8826142
> Tarantool CI results ares excess, since the changeset relates only to
> tarantool/luajit GitHub Actions.
>
>   .github/workflows/lint.yml          | 2 +-
>   .github/workflows/linux-aarch64.yml | 4 ++--
>   .github/workflows/linux-x86_64.yml  | 4 ++--
>   .github/workflows/macos-m1.yml      | 4 ++--
>   .github/workflows/macos-x86_64.yml  | 4 ++--
>   5 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
> index 9e7cc1f1..ff4b0ac2 100644
> --- a/.github/workflows/lint.yml
> +++ b/.github/workflows/lint.yml
> @@ -45,4 +45,4 @@ jobs:
>         - name: configure
>           run: cmake .
>         - name: test
> -        run: make -j LuaJIT-luacheck
> +        run: cmake --build . -j -t LuaJIT-luacheck
> diff --git a/.github/workflows/linux-aarch64.yml b/.github/workflows/linux-aarch64.yml
> index 09b85b56..2f62394f 100644
> --- a/.github/workflows/linux-aarch64.yml
> +++ b/.github/workflows/linux-aarch64.yml
> @@ -54,6 +54,6 @@ jobs:
>         - name: configure
>           run: cmake . ${{ matrix.CMAKEFLAGS }} -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
>         - name: build
> -        run: make -j
> +        run: cmake --build . -j
>         - name: test
> -        run: make -j test
> +        run: cmake --build . -j -t test
> diff --git a/.github/workflows/linux-x86_64.yml b/.github/workflows/linux-x86_64.yml
> index a1e3a618..4e6e3d9b 100644
> --- a/.github/workflows/linux-x86_64.yml
> +++ b/.github/workflows/linux-x86_64.yml
> @@ -54,6 +54,6 @@ jobs:
>         - name: configure
>           run: cmake . ${{ matrix.CMAKEFLAGS }} -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
>         - name: build
> -        run: make -j
> +        run: cmake --build . -j
>         - name: test
> -        run: make -j test
> +        run: cmake --build . -j -t test
> diff --git a/.github/workflows/macos-m1.yml b/.github/workflows/macos-m1.yml
> index eb822142..a272e39b 100644
> --- a/.github/workflows/macos-m1.yml
> +++ b/.github/workflows/macos-m1.yml
> @@ -68,6 +68,6 @@ jobs:
>         - name: configure
>           run: ${ARCH} cmake . ${{ matrix.CMAKEFLAGS }} -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
>         - name: build
> -        run: ${ARCH} make -j
> +        run: ${ARCH} cmake --build . -j
>         - name: test
> -        run: ${ARCH} make -j test
> +        run: ${ARCH} cmake --build . -j -t test
> diff --git a/.github/workflows/macos-x86_64.yml b/.github/workflows/macos-x86_64.yml
> index 09f391db..d639e872 100644
> --- a/.github/workflows/macos-x86_64.yml
> +++ b/.github/workflows/macos-x86_64.yml
> @@ -63,6 +63,6 @@ jobs:
>         - name: configure
>           run: cmake . ${{ matrix.CMAKEFLAGS }} -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
>         - name: build
> -        run: make -j
> +        run: cmake --build . -j
>         - name: test
> -        run: make -j test
> +        run: cmake --build . -j -t test

Re: [Tarantool-patches] [PATCH luajit] ci: make GitHub workflows more CMake-ish

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for your review!

On 02.06.22, Sergey Bronnikov wrote:
> Igor, thanks for the patch!
> 
> See my comments below.
> 
> 1. I propose to use long-options for clarity. In particular I mean 
> long-option for "-j", it is better to replace it with "--parallel", see [1].

Agree, fixed. Diff is below:

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

diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
index ff4b0ac2..c629e5f4 100644
--- a/.github/workflows/lint.yml
+++ b/.github/workflows/lint.yml
@@ -45,4 +45,4 @@ jobs:
       - name: configure
         run: cmake .
       - name: test
-        run: cmake --build . -j -t LuaJIT-luacheck
+        run: cmake --build . --parallel --target LuaJIT-luacheck
diff --git a/.github/workflows/linux-aarch64.yml b/.github/workflows/linux-aarch64.yml
index 2f62394f..19a6e14a 100644
--- a/.github/workflows/linux-aarch64.yml
+++ b/.github/workflows/linux-aarch64.yml
@@ -54,6 +54,6 @@ jobs:
       - name: configure
         run: cmake . ${{ matrix.CMAKEFLAGS }} -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
       - name: build
-        run: cmake --build . -j
+        run: cmake --build . --parallel
       - name: test
-        run: cmake --build . -j -t test
+        run: cmake --build . --parallel --target test
diff --git a/.github/workflows/linux-x86_64.yml b/.github/workflows/linux-x86_64.yml
index 4e6e3d9b..6b17d1a3 100644
--- a/.github/workflows/linux-x86_64.yml
+++ b/.github/workflows/linux-x86_64.yml
@@ -54,6 +54,6 @@ jobs:
       - name: configure
         run: cmake . ${{ matrix.CMAKEFLAGS }} -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
       - name: build
-        run: cmake --build . -j
+        run: cmake --build . --parallel
       - name: test
-        run: cmake --build . -j -t test
+        run: cmake --build . --parallel --target test
diff --git a/.github/workflows/macos-m1.yml b/.github/workflows/macos-m1.yml
index a272e39b..a931495b 100644
--- a/.github/workflows/macos-m1.yml
+++ b/.github/workflows/macos-m1.yml
@@ -68,6 +68,6 @@ jobs:
       - name: configure
         run: ${ARCH} cmake . ${{ matrix.CMAKEFLAGS }} -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
       - name: build
-        run: ${ARCH} cmake --build . -j
+        run: ${ARCH} cmake --build . --parallel
       - name: test
-        run: ${ARCH} cmake --build . -j -t test
+        run: ${ARCH} cmake --build . --parallel --target test
diff --git a/.github/workflows/macos-x86_64.yml b/.github/workflows/macos-x86_64.yml
index d639e872..99bd9fe9 100644
--- a/.github/workflows/macos-x86_64.yml
+++ b/.github/workflows/macos-x86_64.yml
@@ -63,6 +63,6 @@ jobs:
       - name: configure
         run: cmake . ${{ matrix.CMAKEFLAGS }} -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
       - name: build
-        run: cmake --build . -j
+        run: cmake --build . --parallel
       - name: test
-        run: cmake --build . -j -t test
+        run: cmake --build . --parallel --target test

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

> 
> 2. I propose to define a number of threads explicitly, because Ninja and 
> Make have different meaning for -j without a number of threads:
> 
> Ninja:
> 
>        -j N   run N jobs in parallel (0 means infinity) [default=#CPUs]
> 
> Make:
> 
>         -j [jobs], --jobs[=jobs]
>              Specifies the number of jobs (commands) to run 
> simultaneously.  If
>              there is more than one -j option, the last one is 
> effective.  If
>              the -j option is given without an argument, make will not limit
>              the number of jobs that can run simultaneously. When make 
> invokes
>              a sub-make, all instances of make will coordinate to run the
>              specified number of jobs at a time; see the section 
> PARALLEL MAKE
>              AND THE JOBSERVER for details.
> 
> It is recommended to use a number (CPU cores + 1), so we can explicitly 
> set "$(nproc) + 1".

Also fixed. Diff is below:

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

diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
index c629e5f4..3091c10b 100644
--- a/.github/workflows/lint.yml
+++ b/.github/workflows/lint.yml
@@ -45,4 +45,4 @@ jobs:
       - name: configure
         run: cmake .
       - name: test
-        run: cmake --build . --parallel --target LuaJIT-luacheck
+        run: cmake --build . --parallel $(($(nproc) + 1) --target LuaJIT-luacheck
diff --git a/.github/workflows/linux-aarch64.yml b/.github/workflows/linux-aarch64.yml
index 19a6e14a..19412b3f 100644
--- a/.github/workflows/linux-aarch64.yml
+++ b/.github/workflows/linux-aarch64.yml
@@ -54,6 +54,6 @@ jobs:
       - name: configure
         run: cmake . ${{ matrix.CMAKEFLAGS }} -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
       - name: build
-        run: cmake --build . --parallel
+        run: cmake --build . --parallel $(($(nproc) + 1))
       - name: test
-        run: cmake --build . --parallel --target test
+        run: cmake --build . --parallel $(($(nproc) + 1)) --target test
diff --git a/.github/workflows/linux-x86_64.yml b/.github/workflows/linux-x86_64.yml
index 6b17d1a3..d6434afe 100644
--- a/.github/workflows/linux-x86_64.yml
+++ b/.github/workflows/linux-x86_64.yml
@@ -54,6 +54,6 @@ jobs:
       - name: configure
         run: cmake . ${{ matrix.CMAKEFLAGS }} -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
       - name: build
-        run: cmake --build . --parallel
+        run: cmake --build . --parallel $(($(nproc) + 1))
       - name: test
-        run: cmake --build . --parallel --target test
+        run: cmake --build . --parallel $(($(nproc) + 1)) --target test
diff --git a/.github/workflows/macos-m1.yml b/.github/workflows/macos-m1.yml
index a931495b..8387bbcf 100644
--- a/.github/workflows/macos-m1.yml
+++ b/.github/workflows/macos-m1.yml
@@ -68,6 +68,6 @@ jobs:
       - name: configure
         run: ${ARCH} cmake . ${{ matrix.CMAKEFLAGS }} -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
       - name: build
-        run: ${ARCH} cmake --build . --parallel
+        run: ${ARCH} cmake --build . --parallel $(($(nproc) + 1))
       - name: test
-        run: ${ARCH} cmake --build . --parallel --target test
+        run: ${ARCH} cmake --build . --parallel $(($(nproc) + 1)) --target test
diff --git a/.github/workflows/macos-x86_64.yml b/.github/workflows/macos-x86_64.yml
index 99bd9fe9..2ab2c8d0 100644
--- a/.github/workflows/macos-x86_64.yml
+++ b/.github/workflows/macos-x86_64.yml
@@ -63,6 +63,6 @@ jobs:
       - name: configure
         run: cmake . ${{ matrix.CMAKEFLAGS }} -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
       - name: build
-        run: cmake --build . --parallel
+        run: cmake --build . --parallel $(($(nproc) + 1))
       - name: test
-        run: cmake --build . --parallel --target test
+        run: cmake --build . --parallel $(($(nproc) + 1)) --target test

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

Side note: this $(($(..)..)) mess looks a bit ugly, but I can't imagine
another way to implement the approach you're proposing. Furthermore,
there is nothing similar provided by Github Actions, so I believe your
way is the only one.

> 
> There is a CMake option "" [2], but we couldn't use it as it is 
> available since CMake 3.12,
> 
> when LuaJIT build system has a minimal version only 3.1.

Unfortunately, we have to wait until Debian Stretch, Ubuntu Xenial and
Ubuntu Bionic reach their EOL to bump CMake minimal version up to 3.12.
Looking forward to these days...

> 
> 
> 1. https://cmake.org/cmake/help/v3.12/manual/cmake.1.html#build-tool-mode
> 
> 2. 
> https://cmake.org/cmake/help/latest/envvar/CMAKE_BUILD_PARALLEL_LEVEL.html
> 
> Sergey
> 
> On 01.06.2022 18:54, Igor Munkin wrote:

<snipped>

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit] ci: make GitHub workflows more CMake-ish

[Sergey Bronnikov via Tarantool-patches]

Thanks for changes!

LGTM now with one note - seems passing "--parallel" for target 
"LuaJIT-luacheck" is useless because:

1. luacheck requires LuaLanes for enabling multithreading and it is not 
installed in LuaJIT CI

2. option "--jobs" is not passed for luacheck in test/CMakeLists.txt

Feel free to keep patch as is, we can enable option "--jobs" and install 
required LuaLanes module later

in a separate patch.

Sergey

On 02.06.2022 19:00, Igor Munkin wrote:
> <snipped>
> ================================================================================
>
> diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
> index c629e5f4..3091c10b 100644
> --- a/.github/workflows/lint.yml
> +++ b/.github/workflows/lint.yml
> @@ -45,4 +45,4 @@ jobs:
>         - name: configure
>           run: cmake .
>         - name: test
> -        run: cmake --build . --parallel --target LuaJIT-luacheck
> +        run: cmake --build . --parallel $(($(nproc) + 1) --target LuaJIT-luacheck
>
<snipped>

Re: [Tarantool-patches] [PATCH luajit] ci: make GitHub workflows more CMake-ish

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for your review!

On 03.06.22, Sergey Bronnikov wrote:
> Thanks for changes!
> 
> LGTM now with one note - seems passing "--parallel" for target 

Added your tag:
| Reviewed-by: Sergey Bronnikov <sergeyb@tarantool.org>

> "LuaJIT-luacheck" is useless because:
> 
> 1. luacheck requires LuaLanes for enabling multithreading and it is not 
> installed in LuaJIT CI
> 
> 2. option "--jobs" is not passed for luacheck in test/CMakeLists.txt
> 
> Feel free to keep patch as is, we can enable option "--jobs" and install 
> required LuaLanes module later
> 
> in a separate patch.

I've removed "-j" for LuaJIT-luacheck. Diff is below:

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

diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
index 3091c10b..e2d353e9 100644
--- a/.github/workflows/lint.yml
+++ b/.github/workflows/lint.yml
@@ -45,4 +45,4 @@ jobs:
       - name: configure
         run: cmake .
       - name: test
-        run: cmake --build . --parallel $(($(nproc) + 1) --target LuaJIT-luacheck
+        run: cmake --build . --target LuaJIT-luacheck

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

> 
> Sergey
> 

<snipped>

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit] ci: make GitHub workflows more CMake-ish

[Igor Munkin via Tarantool-patches]

I've checked the patch into all long-term branches in tarantool/luajit
and bumped a new version in master, 2.10 and 1.10.

On 01.06.22, Igor Munkin wrote:
> Using explicit <make> commands makes changing the build system generator
> to another one (e.g. Ninja) too verbose. Fortunately, we can use
> generator-agnostic command provided by CMake, so the only spot where
> generator name occurs is the configuration step of GitHub workflow
> (UNIX Makefiles are generated for UNIX-like platforms by default).
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---

<snipped>

> -- 
> 2.34.0
> 

-- 
Best regards,
IM