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

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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

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
>         </compose?To=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: 13372 bytes --]