Tarantool development patches archive
 help / color / mirror / Atom feed
From: Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: "Sergey Bronnikov" <sergeyb@tarantool.org>
Cc: max.kokryashkin@gmail.com, tarantool-patches@dev.tarantool.org,
	"Sergey Bronnikov" <estetus@gmail.com>
Subject: Re: [Tarantool-patches]  [PATCH 3/4][v2] cmake: introduce 'check' and 'LuaJIT-checkpatch' targets
Date: Fri, 21 Jul 2023 00:13:11 +0300	[thread overview]
Message-ID: <1689887591.455772041@f509.i.mail.ru> (raw)
In-Reply-To: <f801b47f-1936-04ef-66b8-a0e10ac0cf44@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 --]

  reply	other threads:[~2023-07-20 21:13 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-07-17 13:34 [Tarantool-patches] [PATCH 0/4][v2] Fix typos and enable checkpatch Sergey Bronnikov via Tarantool-patches
2023-07-17 13:34 ` [Tarantool-patches] [PATCH 1/4][v2] ci: fix a step name Sergey Bronnikov via Tarantool-patches
2023-07-17 15:43   ` Maxim Kokryashkin via Tarantool-patches
2023-07-17 13:34 ` [Tarantool-patches] [PATCH 2/4][v2] codehealth: fix typos Sergey Bronnikov via Tarantool-patches
2023-07-17 15:48   ` Maxim Kokryashkin via Tarantool-patches
2023-07-17 13:34 ` [Tarantool-patches] [PATCH 3/4][v2] cmake: introduce 'check' and 'LuaJIT-checkpatch' targets Sergey Bronnikov via Tarantool-patches
2023-07-17 16:10   ` Maxim Kokryashkin via Tarantool-patches
2023-07-20 18:07     ` Sergey Bronnikov via Tarantool-patches
2023-07-20 21:13       ` Maxim Kokryashkin via Tarantool-patches [this message]
2023-07-17 13:34 ` [Tarantool-patches] [PATCH 4/4][v2] ci: enable checkpatch Sergey Bronnikov via Tarantool-patches
2023-07-17 18:48   ` Maxim Kokryashkin via Tarantool-patches
2023-07-20 18:14     ` Sergey Bronnikov via Tarantool-patches
2023-07-20 21:14       ` Maxim Kokryashkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1689887591.455772041@f509.i.mail.ru \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=estetus@gmail.com \
    --cc=m.kokryashkin@tarantool.org \
    --cc=max.kokryashkin@gmail.com \
    --cc=sergeyb@tarantool.org \
    --subject='Re: [Tarantool-patches]  [PATCH 3/4][v2] cmake: introduce '\''check'\'' and '\''LuaJIT-checkpatch'\'' targets' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox