[Tarantool-patches] [PATCH 3/4][v2] cmake: introduce 'check' and 'LuaJIT-checkpatch' targets
Sergey Bronnikov
sergeyb at tarantool.org
Thu Jul 20 21:07:57 MSK 2023
Hi, Max!
Thanks for your comments!
See my answers below. Updated patch was force-pushed.
On 7/17/23 19:10, Maxim Kokryashkin via Tarantool-patches wrote:
> Hi, Sergey!
> Please consider my comments below.
>
> From: Sergey Bronnikov <sergeyb at tarantool.org
> </compose?To=sergeyb at tarantool.org>>
>
> 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
>
> Typo: s/in a/in/
>
Fixed.
>
> check tags in commit messages like NO_DOC, NO_CHANGELOG,
> NO_TEST and
> others, so to be able to customize command-line options Github
> Action, provided
> by checkpatch repository [3], was added to the repository.
>
> Typo: s/by checkpatch/by the checkpatch/
>
Fixed.
>
>
> See documentation for used checkpatch in [4].
>
> Typo: s/documentation/the documentation/
> Typo: s/for used/for the/
>
Fixed. Fixed.
>
>
> Patch introduce new CMake targets: LuaJIT-checkpatch, that checks
>
> Typo: s/introduce/introduces/
>
Fixed.
>
> patches on top of master branch using script checkpatch.pl
> [1], and
>
> Typo: s/on top of/on top of the/
>
Fixed.
>
> target check, that combines LuaJIT-luacheck and LuaJIT-checkpatch.
>
> 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
>
> Nit: It is kinda strange to see link [1] going after the link [4]
> in the commit message.
> I think, it generally looks clearer, when they are ordered, but
> that’s a matter of taste.
> Feel free to ignore.
>
Rewrote description and fixed order of references.
>
>
> --- test/CMakeLists.txt | 33 +++++++++++++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
>
> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> index 47296a22..ccbad035 100644
> --- a/test/CMakeLists.txt
> +++ b/test/CMakeLists.txt
> @@ -42,6 +42,39 @@ else()
> )
> endif()
>
> +find_program(CHECKPATCH checkpatch.pl
> + HINTS ${PROJECT_SOURCE_DIR}/checkpatch)
> +if(CHECKPATCH)
>
> I don’t really like that `MASTER_BRANCH` name is hardcoded. I think
> it’s possible to implement it similarly to how it’s done in the
> `tarantool/checkpatch`
> github action[1] with revision range. Or, at least, it is for sure
> possible to obtain
> the master branch name dynamically.
>
In LuaJIT we have a single branch for merging new patches -
tarantool/master.
Why do you need to redefine master branch?
> + set(MASTER_BRANCH "tarantool/master")
> + add_custom_target(${PROJECT_NAME}-checkpatch)
> + add_custom_command(TARGET ${PROJECT_NAME}-checkpatch
> + COMMENT "Running checkpatch"
> + COMMAND
> + ${CHECKPATCH}
> + --codespell
> + --color=always
> + --git ${MASTER_BRANCH}..HEAD
> + --ignore COMMIT_LOG_LONG_LINE
> + # Requires at least two lines in commit message and this
> + # is annoying.
> + --ignore COMMIT_MESSAGE
> + --ignore NO_CHANGELOG
> + --ignore NO_DOC
> + --ignore NO_TEST
> + --show-types
> + WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
> + )
> +else()
> + add_custom_target(${PROJECT_NAME}-checkpatch)
>
> It seems like the target definition can be moved out of the `if`
> statement
> just before it.
>
Done. As well as definition of MASTER_BRANCH variable.
>
> + add_custom_command(TARGET ${PROJECT_NAME}-checkpatch
> + COMMENT "`checkpatch.pl' is not found, so
> ${PROJECT_NAME}-checkpatch target is dummy"
> + )
> +endif()
> +
> +add_custom_target(check
> + DEPENDS ${PROJECT_NAME}-checkpatch ${PROJECT_NAME}-luacheck
> +)
> +
>
> As I have already said offline, I think we should include the
> `check` target as a dependency to the `test` target, just like it
> is currently done for the luacheck. It is much more convenient for
> local testing that way.
>
Fixed.
>
> set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e
> dofile[[${LUAJIT_TEST_INIT}]]")
> separate_arguments(LUAJIT_TEST_COMMAND)
>
> --
> 2.34.1
>
> [1]:
> https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml
> --
> Best regards,
> Maxim Kokryashkin
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20230720/32c8d312/attachment.htm>
More information about the Tarantool-patches
mailing list