Re: [Tarantool-patches] [PATCH 2/2] ci: enable checkpatch

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

Hi, Sergey!
Thanks for the patch!
Please, consider my comments below.

On 11.07.23, Sergey Bronnikov wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> Patch enables checkpatch [1] for checking patch on a pre-commit stage.

Minor: Strictly saying it's not a precommit stage (since it's not a
pre-commit hook). But this is a prerequisite for other testing workflow.

> In Tarantool we use our own fork of checkpatch [2] with additional check
> types. It's logical to use it in a LuaJIT development. We don't need

Also, it is good to have some checks for our C code like:
* <src/lib_misc.c>
* <src/lj_mapi.c>
* <src/lj_memprof.[ch]>
* <src/lj_symtab.[ch]>
* <src/lj_sysprof.[ch]>
* <src/lj_utils.h>
* <src/lj_utils_leb128.c>
* <src/lj_wbuf.[ch]>
* <src/lmisclib.h>
But they are contradicting with Tarantool's guidelines as far as they
are written in LuaJIT's style. Have we some way to check them too?

> check tags in commit messages like NO_DOC, NO_CHANGELOG, NO_TEST and

Side note: Not so sure, that NO_TEST check is a good thing, but I'm OK
with it, since it may be useful for backporting refactoring changes.

> others, so to be able to customize command-line options Github Action, provided
> by checkpatch repository [3], was added to the repository.
> 
> See documentation for used checkpatch in [4].
> 
> 1. https://docs.kernel.org/dev-tools/checkpatch.html
> 2. https://github.com/tarantool/checkpatch
> 3. https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml
> 4. https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst

Also, is it possible to create the make target similar to
LuaJIT-luacheck? It should:
0) Be dummy if there is no chekcpatch or codespell installed.
1) Be included in the make test check.
It is useful for local spellcheck without any push to remote CI.

> ---
>  .github/actions/checkpatch/action.yml | 17 +++++++++++++++++
>  .github/workflows/lint.yml            | 14 ++++++++++++++
>  2 files changed, 31 insertions(+)
>  create mode 100644 .github/actions/checkpatch/action.yml
> 
> diff --git a/.github/actions/checkpatch/action.yml b/.github/actions/checkpatch/action.yml
> new file mode 100644
> index 00000000..df2e2a2b
> --- /dev/null
> +++ b/.github/actions/checkpatch/action.yml
> @@ -0,0 +1,17 @@
> +name: Checkpatch
> +description: Check patches against LuaJIT development guidelines
> +inputs:
> +  revision-range:
> +    description: Git revision range to check
> +    required: true
> +runs:
> +  using: composite
> +  steps:
> +    - uses: actions/checkout@v3
> +      with:
> +        repository: tarantool/checkpatch
> +        path: 'checkpatch'
> +    - run: apt install -y codespell
> +      shell: bash
> +    - run: checkpatch/checkpatch.pl --codespell --color=always --show-types --git ${{ inputs.revision-range }} --codespellfile /usr/lib/python3/dist-packages/codespell_lib/data/dictionary.txt --ignore NO_CHANGELOG,NO_DOC,NO_TEST,COMMIT_LOG_LONG_LINE

Please, split this line into several.

> +      shell: bash
> diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
> index 71ceee9a..d28aa15b 100644
> --- a/.github/workflows/lint.yml
> +++ b/.github/workflows/lint.yml
> @@ -53,3 +53,17 @@ jobs:
>        - name: test
>          run: cmake --build . --target LuaJIT-luacheck
>          working-directory: ${{ env.BUILDDIR }}
> +
> +  checkpatch:
> +    runs-on: [self-hosted, lightweight, Linux, x86_64]
> +
> +    steps:
> +      - uses: actions/checkout@v3
> +        with:
> +          fetch-depth: 0
> +          # ref: ${{ github.event.pull_request.head.sha }}

Minor: Why do we need this comment?

> +          submodules: recursive
> +      - name: checkpatch
> +        uses: ./.github/actions/checkpatch
> +        with:
> +          revision-range: HEAD~${{ github.event.pull_request.commits }}..HEAD

There is something wrong with this definition; in CI [1] it changes to the
following:

| checkpatch/checkpatch.pl --codespell --color=always --show-types --git HEAD~..HEAD --ignore NO_CHANGELOG,NO_DOC,NO_TEST,COMMIT_LOG_LONG_LINE

So, only the top commit is verified (instead of 2 -- this is why there
is a typo in the previous commit message).

Also, is it possible to do this activity on push instead, depending on
difference with tarantool/master HEAD?

Also, how can we avoid some errors here, if this will show incorrect
spelling in the backported patches?

> -- 
> 2.34.1
> 

[1]: https://github.com/tarantool/luajit/actions/runs/5519618350/jobs/10065116711#step:3:143

-- 
Best regards,
Sergey Kaplun