From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Bronnikov <sergeyb@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit] ci: make GitHub workflows more CMake-ish
Date: Thu, 2 Jun 2022 19:00:10 +0300 [thread overview]
Message-ID: <YpjeijSClWO0v82Y@tarantool.org> (raw)
In-Reply-To: <58ae0b0e-387a-2171-1d3b-dbdfa746c421@tarantool.org>
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
next prev parent reply other threads:[~2022-06-02 16:07 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-01 15:54 Igor Munkin via Tarantool-patches
2022-06-02 7:45 ` Sergey Kaplun via Tarantool-patches
2022-06-06 10:19 ` Igor Munkin via Tarantool-patches
2022-06-02 12:25 ` Sergey Bronnikov via Tarantool-patches
2022-06-02 16:00 ` Igor Munkin via Tarantool-patches [this message]
2022-06-03 10:15 ` Sergey Bronnikov via Tarantool-patches
2022-06-06 10:18 ` Igor Munkin via Tarantool-patches
2022-06-20 12:47 ` Igor Munkin via Tarantool-patches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YpjeijSClWO0v82Y@tarantool.org \
--to=tarantool-patches@dev.tarantool.org \
--cc=imun@tarantool.org \
--cc=sergeyb@tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH luajit] ci: make GitHub workflows more CMake-ish' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox