[Tarantool-patches] [PATCH luajit] ci: make GitHub workflows more CMake-ish
Igor Munkin
imun at tarantool.org
Thu Jun 2 19:00:10 MSK 2022
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
More information about the Tarantool-patches
mailing list