[Tarantool-patches] [PATCH 2/2] ci: enable checkpatch
Sergey Bronnikov
sergeyb at tarantool.org
Mon Jul 17 16:22:54 MSK 2023
Hi, Sergey!
thanks for review!
Fixes applied and force-pushed.
On 7/14/23 15:45, Sergey Kaplun wrote:
> 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?
New findings:
diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
index 2e9ed9b3..745eb1a3 100644
--- a/src/lj_sysprof.c
+++ b/src/lj_sysprof.c
@@ -485,7 +485,7 @@ int lj_sysprof_stop(lua_State *L)
if (SPS_HALT == sp->state) {
errno = sp->saved_errno;
sp->state = SPS_IDLE;
- /* wbuf was terminated when error occured. */
+ /* wbuf was terminated when error occurred. */
return PROFILE_ERRIO;
}
diff --git a/src/lj_wbuf.h b/src/lj_wbuf.h
index 9eaa5e51..33ec8463 100644
--- a/src/lj_wbuf.h
+++ b/src/lj_wbuf.h
@@ -75,7 +75,7 @@ void lj_wbuf_addn(struct lj_wbuf *buf, const void
*src, size_t n);
/* Write string to the buffer. */
void LJ_FASTCALL lj_wbuf_addstring(struct lj_wbuf *buf, const char *s);
-/* Immediatly flush the buffer. */
+/* Immediately flush the buffer. */
void LJ_FASTCALL lj_wbuf_flush(struct lj_wbuf *buf);
/* Check flags. */
new fixes were added (amended) to a commit with typos fixes.
>
>> 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.
I don't remember a case when someone missed a test for the patch with fix,
so I would leave this options disabled.
(Let's not cure what does not hurt.)
>
>> 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.
Added.
> 1) Be included in the make test check.
Didn't get it. What make target did you mean? "test" or "check"?
I've added a new target "check" and added LuaJIT-luacheck and
LuaJIT-checkpatch there.
> 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.
Done.
--- a/.github/actions/checkpatch/action.yml
+++ b/.github/actions/checkpatch/action.yml
@@ -13,5 +13,12 @@ runs:
path: 'checkpatch'
- run: apt install -y codespell
shell: bash
- - run: checkpatch/checkpatch.pl --codespell --color=always
--show-types --git ${{ inputs.revision-range }} --ignore NO_CHANGELOG,NO_DOC
,NO_TEST,COMMIT_LOG_LONG_LINE
+ - run: checkpatch/checkpatch.pl --codespell \
+ --color=always \
+ --show-types \
+ --git ${{ inputs.revision-range }} \
+ --ignore NO_CHANGELOG \
+ --ignore NO_DOC \
+ --ignore NO_TEST \
+ --ignore COMMIT_LOG_LONG_LINE
shell: bash
>
>> + 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?
Forgot to remove, removed.
>
>> + 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).
Fixed. Now checkpatch always checks a range tarantool/master..HEAD.
>
> Also, is it possible to do this activity on push instead, depending on
> difference with tarantool/master HEAD?
Yes, fixed.
>
> Also, how can we avoid some errors here, if this will show incorrect
> spelling in the backported patches?
Should we avoid these errors?
If yes, then, I suppose, we can ignore them.
>> --
>> 2.34.1
>>
> [1]: https://github.com/tarantool/luajit/actions/runs/5519618350/jobs/10065116711#step:3:143
More information about the Tarantool-patches
mailing list