Re: [Tarantool-patches] [v2][PATCH 5/5] ci: add job with build using Ninja on linux-x86_64

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey

thanks for review!

On 15.06.2022 12:27, Sergey Kaplun wrote:
> Hi, Sergey!
>
> Thanks for the patch!
>
> LGTM, after fixes mentioned by Igor below and my 2 cents below.
>
> On 15.06.22, Igor Munkin via Tarantool-patches wrote:
>> Sergey,
>>
>> Thanks for the patch! LGTM as trivial, but please consider nits below.
>>
>> On 06.06.22, Sergey Bronnikov wrote:
>>> ---
>>>   .github/workflows/linux-x86_64-ninja.yml | 51 ++++++++++++++++++++++++
>>>   1 file changed, 51 insertions(+)
>>>   create mode 100644 .github/workflows/linux-x86_64-ninja.yml
>>>
>>> diff --git a/.github/workflows/linux-x86_64-ninja.yml b/.github/workflows/linux-x86_64-ninja.yml
>>> new file mode 100644
>>> index 00000000..833d36d9
>>> --- /dev/null
>>> +++ b/.github/workflows/linux-x86_64-ninja.yml
>>> @@ -0,0 +1,51 @@
>>> +name: "LuaJIT test workflow with Ninja (Linux/x86_64)"
>>> +
>>> +on:
>>> +  push:
>>> +    branches-ignore:
>>> +      - '**-notest'
>>> +      - 'upstream-**'
>>> +    tags-ignore:
>>> +      - '**'
>>> +
>>> +concurrency:
>>> +  # An update of a developer branch cancels the previously
>>> +  # scheduled workflow run for this branch. However, the default
>>> +  # branch, and long-term branch (tarantool-1.10, tarantool-2.8,
>>> +  # etc.) workflow runs are never canceled.
>>> +  #
>>> +  # We use a trick here: define the concurrency group as 'workflow
>>> +  # run ID' + # 'workflow run attempt' because it is a unique
>>> +  # combination for any run. So it effectively discards grouping.
>>> +  #
>>> +  # XXX: we cannot use `github.sha` as a unique identifier because
>>> +  # pushing a tag may cancel a run that works on a branch push
>>> +  # event.
>>> +  group: ${{ (
>>> +    github.ref == 'refs/heads/tarantool' ||
>>> +    startsWith(github.ref, 'refs/heads/tarantool-')) &&
>>> +    format('{0}-{1}', github.run_id, github.run_attempt) ||
>>> +    format('{0}-{1}', github.workflow, github.ref) }}
>>> +  cancel-in-progress: true
>>> +
>>> +jobs:
>>> +  test-linux-x86_64:
> Nit: May be `test-linux-ninja-x86_64` is better?
> To be different from the same name for make target building?

Agree, fixed:

--- a/.github/workflows/linux-x86_64-ninja.yml
+++ b/.github/workflows/linux-x86_64-ninja.yml
@@ -29,10 +29,8 @@ concurrency:
    cancel-in-progress: true

  jobs:
-  test-linux-x86_64:
+  test-linux-ninja-x86_64:
      runs-on: ubuntu-20.04-self-hosted

>
>>> +    runs-on: ubuntu-20.04-self-hosted
>>> +    strategy:
>>> +      fail-fast: false
> Nit: As far as there is no testing matrix we can omit strategy for it.

Seems so [1].

Fixed with patch below:

--- a/.github/workflows/linux-x86_64-ninja.yml
+++ b/.github/workflows/linux-x86_64-ninja.yml
@@ -31,8 +31,6 @@ concurrency:
  jobs:
    test-linux-x86_64:
      runs-on: ubuntu-20.04-self-hosted
-    strategy:
-      fail-fast: false
      name: Linux/x86_64 ${{ matrix.BUILDTYPE }}
      steps:
        - uses: actions/checkout@v2.3.4

1. 
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstrategyfail-fast

>
>>> +    name: Linux/x86_64 ${{ matrix.BUILDTYPE }}
>> There is no testing matrix, so you can just name the workflow kinda
>> "Linux/x86_64 Ninja".
Fixed too.
>>
>>> +    steps:
>>> +      - uses: actions/checkout@v2.3.4
>>> +        with:
>>> +          fetch-depth: 0
>>> +          submodules: recursive
>>> +      - name: setup
>>> +        run: |
>>> +          sudo apt -y update
>>> +          sudo apt -y install cmake ninja-build gcc perl
>>> +      - name: configure
>>> +        run: cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -G Ninja
>>> +      - name: build
>>> +        run: cmake --build . -j $(nproc)
>>> +      - name: test
>>> +        run: cmake --build . -j $(nproc) -t test
>> Please, use long options, as we discussed here[1].

Fixed:

--- a/.github/workflows/linux-x86_64-ninja.yml
+++ b/.github/workflows/linux-x86_64-ninja.yml
@@ -44,6 +44,6 @@ jobs:
        - name: configure
          run: cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -G Ninja
        - name: build
-        run: cmake --build . -j $(nproc)
+        run: cmake --build . --parallel $(($(nproc) + 1))
        - name: test
-        run: cmake --build . -j $(nproc) -t test
+        run: cmake --build . --parallel $(($(nproc) + 1)) --target test

>>
>>> -- 
>>> 2.25.1
>>>
>> [1]: https://lists.tarantool.org/tarantool-patches/YpjeijSClWO0v82Y@tarantool.org/
>>
>> -- 
>> Best regards,
>> IM