[Tarantool-patches] [PATCH 2/2] ci: enable checkpatch
Sergey Kaplun
skaplun at tarantool.org
Fri Jul 14 15:45:40 MSK 2023
Hi, Sergey!
Thanks for the patch!
Please, consider my comments below.
On 11.07.23, Sergey Bronnikov wrote:
> From: Sergey Bronnikov <sergeyb at 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 at 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 at 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
More information about the Tarantool-patches
mailing list