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: 4547 bytes --]


Hi!
Thanks for the fixes!
LGTM now. As for the master branch name,
your comment is fair enough, I just wanted to ensure
that our solution is robust enough.
 
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>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@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
> 

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