From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 3A70953D9F5; Thu, 20 Jul 2023 21:08:00 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3A70953D9F5 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1689876480; bh=LZ0M7Fm7I3CsZCdBzuDTFwUFFq+htTZdYrlzSFw4pJE=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=Fc6GgoEwq/e+CgT3mxltxeVXn8UoeuYCOTFx1Jb5UM3zYGmcmYAXVmdMyF14Sf+Vp GJ5TI4oEDifwHpg1xrlip2VQVEN/1LctgkU+OvFD3q+CrqdC03K2mz9CPTKCzmfypw 0KaVP2UGuNB999L/AA4ORSznJoRl5l+WV/L35ae4= Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [95.163.41.87]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id B40DB53D9EF for ; Thu, 20 Jul 2023 21:07:59 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B40DB53D9EF Received: by smtp51.i.mail.ru with esmtpa (envelope-from ) id 1qMY4E-00DBBd-Ru; Thu, 20 Jul 2023 21:07:59 +0300 Content-Type: multipart/alternative; boundary="------------JwXY0ILE8OOJF1jBGf295Huv" Message-ID: Date: Thu, 20 Jul 2023 21:07:57 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Content-Language: en-US To: Maxim Kokryashkin , Sergey Bronnikov Cc: max.kokryashkin@gmail.com, tarantool-patches@dev.tarantool.org References: <1c8a5c13c50a2561ebe5d3f82749df644fe2a352.1689600525.git.sergeyb@tarantool.org> <1689610201.27637747@f738.i.mail.ru> In-Reply-To: <1689610201.27637747@f738.i.mail.ru> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9227D43F3CB451B4174B8553CB8B83CEC947A95EC0C091FE4182A05F538085040F202123BD18E0B80EC684BC497990A8D8AB4F374827B6D81637D4B722BA2E63F X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7624C4D757C4F5837EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063761A26A2A89ED60DE8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D86E35EAAFE44D11689DC7A769F148CAE6117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8B59F0E22272DBA2DBA471835C12D1D977C4224003CC836476EB9C4185024447017B076A6E789B0E975F5C1EE8F4F765FC6A41CCFFBAEDDFE13AA81AA40904B5D9CF19DD082D7633A0C84D3B47A649675F3AA81AA40904B5D98AA50765F79006373018F9FAEF57917FD81D268191BDAD3D3666184CF4C3C14F3FC91FA280E0CE3D1A620F70A64A45A98AA50765F79006375CA0AC343891687B6D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637F765F39FA4E70FFE43847C11F186F3C59DAA53EE0834AAEE X-87b9d050: 1 X-C1DE0DAB: 0D63561A33F958A5F2819BF37E0DED2D1651EB0DB80BD011E0EFB506C396A058F87CCE6106E1FC07E67D4AC08A07B9B0F254576263B31EA9CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFB1276B873D970BF16656852741D6AAE01E87AA21839B4666FC2A45F4D52690691F5011C79ABBA8E77A512D943FCE53BFAEFA5CAB41C6B6FAEAB36561E43B1675A74DFFEFA5DC0E7F02C26D483E81D6BE0DBAE6F56676BC7117BB6831D7356A2DEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojGEBeBQ0LqGbmAq9NyFsR4Q== X-Mailru-Sender: C4F68CFF4024C8867DFDF7C7F2588458E2176237F95AC750368BFBFA47622BCAD450509543FB043B282EC151BADDC1D3523A6D01B4765B2DFB59E2DDD9FE06B14FA522850F29BC30B0DAF586E7D11B3E67EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 3/4][v2] cmake: introduce 'check' and 'LuaJIT-checkpatch' targets X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------JwXY0ILE8OOJF1jBGf295Huv Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 > > > 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 > --------------JwXY0ILE8OOJF1jBGf295Huv Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

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.
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
--
Best regards,
Maxim Kokryashkin
--------------JwXY0ILE8OOJF1jBGf295Huv--