Re: [Tarantool-patches] [PATCH 3/4][v2] cmake: introduce 'check' and 'LuaJIT-checkpatch' targets

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 3590 bytes --]


Hi, Sergey!
Please consider my comments below.
 
> 
>>From: Sergey Bronnikov < sergeyb@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/
>>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/
>>
>>See documentation for used checkpatch in [4].
>Typo: s/documentation/the documentation/
>Typo: s/for used/for the/
>>
>>Patch introduce new CMake targets: LuaJIT-checkpatch, that checks
>Typo: s/introduce/introduces/
>>patches on top of master branch using script checkpatch.pl [1], and
>Typo: s/on top of/on top of the/
>>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.
>>
>>--- 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.
>>+ 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.
>>+ 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.
>> 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

[-- Attachment #2: Type: text/html, Size: 6327 bytes --]