<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi, Max!</p>
<p>Thanks for your comments!</p>
<p>See my answers below. Updated patch was force-pushed.</p>
<p><br>
</p>
<div class="moz-cite-prefix">On 7/17/23 19:10, Maxim Kokryashkin via
Tarantool-patches wrote:<br>
</div>
<blockquote type="cite"
cite="mid:1689610201.27637747@f738.i.mail.ru">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div>Hi, Sergey!</div>
<div>Please consider my comments below.</div>
<div> </div>
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div>
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div id="">
<div class="js-helper js-readmsg-msg">
<div>
<div id="style_16896011180277443001_BODY">From: Sergey
Bronnikov <<a
href="/compose?To=sergeyb@tarantool.org"
moz-do-not-send="true">sergeyb@tarantool.org</a>><br>
<br>
In Tarantool we use our own fork of checkpatch [2]
with additional check<br>
types. It's logical to use it in a LuaJIT
development. We don't need</div>
</div>
</div>
</div>
</blockquote>
<div>Typo: s/in a/in/</div>
</div>
</blockquote>
</blockquote>
Fixed.<br>
<blockquote type="cite"
cite="mid:1689610201.27637747@f738.i.mail.ru">
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div>
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div>
<div class="js-helper js-readmsg-msg">
<div>
<div>check tags in commit messages like NO_DOC,
NO_CHANGELOG, NO_TEST and<br>
others, so to be able to customize command-line
options Github Action, provided<br>
by checkpatch repository [3], was added to the
repository.</div>
</div>
</div>
</div>
</blockquote>
<div>Typo: s/by checkpatch/by the checkpatch/</div>
</div>
</blockquote>
</blockquote>
Fixed.<br>
<blockquote type="cite"
cite="mid:1689610201.27637747@f738.i.mail.ru">
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div>
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div>
<div class="js-helper js-readmsg-msg">
<div>
<div><br>
See documentation for used checkpatch in [4].</div>
</div>
</div>
</div>
</blockquote>
<div>Typo: s/documentation/the documentation/</div>
<div>Typo: s/for used/for the/</div>
</div>
</blockquote>
</blockquote>
Fixed. Fixed.<br>
<blockquote type="cite"
cite="mid:1689610201.27637747@f738.i.mail.ru">
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div>
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div>
<div class="js-helper js-readmsg-msg">
<div>
<div><br>
Patch introduce new CMake targets:
LuaJIT-checkpatch, that checks</div>
</div>
</div>
</div>
</blockquote>
<div>Typo: s/introduce/introduces/</div>
</div>
</blockquote>
</blockquote>
Fixed.<br>
<blockquote type="cite"
cite="mid:1689610201.27637747@f738.i.mail.ru">
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div>
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div>
<div class="js-helper js-readmsg-msg">
<div>
<div>patches on top of master branch using script
checkpatch.pl [1], and</div>
</div>
</div>
</div>
</blockquote>
<div>Typo: s/on top of/on top of the/</div>
</div>
</blockquote>
</blockquote>
Fixed.<br>
<blockquote type="cite"
cite="mid:1689610201.27637747@f738.i.mail.ru">
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div>
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div>
<div class="js-helper js-readmsg-msg">
<div>
<div>target check, that combines LuaJIT-luacheck and
LuaJIT-checkpatch.<br>
<br>
1. <a
href="https://docs.kernel.org/dev-tools/checkpatch.html"
target="_blank" moz-do-not-send="true"
class="moz-txt-link-freetext">https://docs.kernel.org/dev-tools/checkpatch.html</a><br>
2. <a
href="https://github.com/tarantool/checkpatch"
target="_blank" moz-do-not-send="true"
class="moz-txt-link-freetext">https://github.com/tarantool/checkpatch</a><br>
3. <a
href="https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml"
target="_blank" moz-do-not-send="true"
class="moz-txt-link-freetext">https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml</a><br>
4. <a
href="https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst"
target="_blank" moz-do-not-send="true"
class="moz-txt-link-freetext">https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst</a></div>
</div>
</div>
</div>
</blockquote>
<div>Nit: It is kinda strange to see link [1] going after the
link [4] in the commit message.</div>
<div>I think, it generally looks clearer, when they are
ordered, but that’s a matter of taste.</div>
<div>Feel free to ignore.</div>
</div>
</blockquote>
</blockquote>
Rewrote description and fixed order of references.<br>
<blockquote type="cite"
cite="mid:1689610201.27637747@f738.i.mail.ru">
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div>
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div>
<div class="js-helper js-readmsg-msg">
<div>
<div><br>
--- test/CMakeLists.txt | 33
+++++++++++++++++++++++++++++++++<br>
1 file changed, 33 insertions(+)<br>
<br>
diff --git a/test/CMakeLists.txt
b/test/CMakeLists.txt<br>
index 47296a22..ccbad035 100644<br>
--- a/test/CMakeLists.txt<br>
+++ b/test/CMakeLists.txt<br>
@@ -42,6 +42,39 @@ else()<br>
)<br>
endif()<br>
<br>
+find_program(CHECKPATCH checkpatch.pl<br>
+ HINTS ${PROJECT_SOURCE_DIR}/checkpatch)<br>
+if(CHECKPATCH)</div>
</div>
</div>
</div>
</blockquote>
<div>I don’t really like that `MASTER_BRANCH` name is
hardcoded. I think</div>
<div>it’s possible to implement it similarly to how it’s done
in the `tarantool/checkpatch`</div>
<div>github action[1] with revision range. Or, at least, it is
for sure possible to obtain</div>
<div>the master branch name dynamically.</div>
</div>
</blockquote>
</blockquote>
<p>In LuaJIT we have a single branch for merging new patches -
tarantool/master.</p>
<p>Why do you need to redefine master branch?<br>
</p>
<blockquote type="cite"
cite="mid:1689610201.27637747@f738.i.mail.ru">
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div>
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div>
<div class="js-helper js-readmsg-msg">
<div>
<div>+ set(MASTER_BRANCH "tarantool/master")<br>
+ add_custom_target(${PROJECT_NAME}-checkpatch)<br>
+ add_custom_command(TARGET
${PROJECT_NAME}-checkpatch<br>
+ COMMENT "Running checkpatch"<br>
+ COMMAND<br>
+ ${CHECKPATCH}<br>
+ --codespell<br>
+ --color=always<br>
+ --git ${MASTER_BRANCH}..HEAD<br>
+ --ignore COMMIT_LOG_LONG_LINE<br>
+ # Requires at least two lines in commit message
and this<br>
+ # is annoying.<br>
+ --ignore COMMIT_MESSAGE<br>
+ --ignore NO_CHANGELOG<br>
+ --ignore NO_DOC<br>
+ --ignore NO_TEST<br>
+ --show-types<br>
+ WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}<br>
+ )<br>
+else()<br>
+ add_custom_target(${PROJECT_NAME}-checkpatch)</div>
</div>
</div>
</div>
</blockquote>
<div>It seems like the target definition can be moved out of
the `if` statement</div>
<div>just before it.</div>
</div>
</blockquote>
</blockquote>
Done. As well as definition of MASTER_BRANCH variable.
<blockquote type="cite"
cite="mid:1689610201.27637747@f738.i.mail.ru">
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div>
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div>
<div class="js-helper js-readmsg-msg">
<div>
<div>+ add_custom_command(TARGET
${PROJECT_NAME}-checkpatch<br>
+ COMMENT "`checkpatch.pl' is not found, so
${PROJECT_NAME}-checkpatch target is dummy"<br>
+ )<br>
+endif()<br>
+<br>
+add_custom_target(check<br>
+ DEPENDS ${PROJECT_NAME}-checkpatch
${PROJECT_NAME}-luacheck<br>
+)<br>
+</div>
</div>
</div>
</div>
</blockquote>
<div>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.</div>
</div>
</blockquote>
</blockquote>
Fixed.<br>
<blockquote type="cite"
cite="mid:1689610201.27637747@f738.i.mail.ru">
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div>
<blockquote style="border-left:1px solid #0857A6; margin:10px;
padding:0 0 0 10px;">
<div>
<div class="js-helper js-readmsg-msg">
<div>
<div> set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY}
-e dofile[[${LUAJIT_TEST_INIT}]]")<br>
separate_arguments(LUAJIT_TEST_COMMAND)<br>
<br>
--<br>
2.34.1</div>
</div>
</div>
</div>
</blockquote>
<div>[1]: <a
href="https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml"
moz-do-not-send="true" class="moz-txt-link-freetext">https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml</a></div>
<div>
<div>--<br>
Best regards,</div>
<div>Maxim Kokryashkin</div>
</div>
</div>
</blockquote>
</blockquote>
</body>
</html>