<HTML><BODY><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">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><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><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><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><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><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">https://docs.kernel.org/dev-tools/checkpatch.html</a><br>2. <a href="https://github.com/tarantool/checkpatch" target="_blank">https://github.com/tarantool/checkpatch</a><br>3. <a href="https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml" target="_blank">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">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><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><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><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><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">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></BODY></HTML>