Re: [Tarantool-patches] [PATCH 3/5][v3] cmake: introduce new targets

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

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

Hi, Max

On 8/3/23 22:38, 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 [1] with
>         additional check
>         types. It's logical to use it in LuaJIT development. However,
>         we don't
>         need to enable all checks [2] implemented in checkpatch,
>         therefore a
>         number of checks are disabled.
>
>         Patch introduces two new CMake targets: "LuaJIT-checkpatch",
>         that checks
>         patches on top of the master branch using script
>         checkpatch.pl, and
>
>     Typo: s/using/using the/
>
Fixed.

>         target "check", that combines LuaJIT-luacheck and
>         LuaJIT-checkpatch. By
>         default CMake looking for checkpatch.pl in a directory
>         "checkpatch" in
>
>     Typo: s/looking/looks/
>     Typo: s/in a directory/in the directory/
>
Fixed.
>
>         LuaJIT repository root directory and in a directories
>         specified in PATH.
>
>     Typo: s/LuaJIT/the LuaJIT/
>     Typo: s/a directories/directories/
>
Fixed.


>
>         1. https://github.com/tarantool/checkpatch
>         2.
>         https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst
>         ---
>          test/CMakeLists.txt | 51
>         +++++++++++++++++++++++++++++++++++++++++++++
>          1 file changed, 51 insertions(+)
>
>         diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
>         index 47296a22..5ec0bed6 100644
>         --- a/test/CMakeLists.txt
>         +++ b/test/CMakeLists.txt
>         @@ -42,6 +42,56 @@ else()
>            )
>          endif()
>
>         +find_program(CHECKPATCH checkpatch.pl
>         + HINTS ${PROJECT_SOURCE_DIR}/checkpatch)
>         +add_custom_target(${PROJECT_NAME}-checkpatch)
>         +set(MASTER_BRANCH "tarantool/master")
>         +if(CHECKPATCH)
>         + add_custom_command(TARGET ${PROJECT_NAME}-checkpatch
>         + COMMENT "Running checkpatch"
>         + COMMAND
>         + ${CHECKPATCH}
>         + # Description of supported checks in
>         + #
>         https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst
>         + --codespell
>         + --color=always
>         + --git ${MASTER_BRANCH}..HEAD
>         + --show-types
>         + --ignore BAD_SIGN_OFF
>         + --ignore BLOCK_COMMENT_STYLE
>         + --ignore CODE_INDENT
>         + --ignore COMMIT_LOG_LONG_LINE
>         + # Requires at least two lines in commit message and this
>         + # is annoying.
>         + --ignore COMMIT_MESSAGE
>         + --ignore CONSTANT_COMPARISON
>         + --ignore FUNCTION_NAME_NO_NEWLINE
>         + --ignore GIT_COMMIT_ID
>         + --ignore INCLUDE_GUARD
>         + --ignore LOGICAL_CONTINUATIONS
>         + --ignore LONG_LINE
>         + --ignore NO_CHANGELOG
>         + --ignore NO_DOC
>         + --ignore NO_TEST
>         + --ignore PREFER_DEFINED_ATTRIBUTE_MACRO
>         + --ignore SPACING
>         + --ignore SUSPECT_CODE_INDENT
>         + --ignore TABSTOP
>         + --ignore TRAILING_STATEMENTS
>         + --ignore UNCOMMENTED_DEFINITION
>         + --ignore UNSAFE_FUNCTION
>         + WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
>         + )
>         +else()
>
>     I suggest doing the same as with coverage — move that logic into a
>     separate module,
>     so the test/CMakeLists.txt remains readable and clean.
>

Done, force-pushed.


commit 609f893ecc5ee9895916c637c145489fe59d9bb0
Author: Sergey Bronnikov <estetus@gmail.com>
Date:   Fri Aug 4 13:51:23 2023 +0300

     cmake: checkpatch module

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 0204e852..b1a442b7 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -29,9 +29,12 @@ endif()

  set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")

+set(GIT_MASTER_BRANCH "tarantool/master")
+
  include(LuaJITUtils)
  include(SetBuildParallelLevel)
  include(SetVersion)
+include(CheckPatch)

  # --- Variables to be exported to child scopes 
---------------------------------

diff --git a/cmake/CheckPatch.cmake b/cmake/CheckPatch.cmake
new file mode 100644
index 00000000..243ee426
--- /dev/null
+++ b/cmake/CheckPatch.cmake
@@ -0,0 +1,46 @@
+find_program(CHECKPATCH checkpatch.pl
+             HINTS ${PROJECT_SOURCE_DIR}/checkpatch)
+add_custom_target(${PROJECT_NAME}-checkpatch)
+if(CHECKPATCH)
+  add_custom_command(TARGET ${PROJECT_NAME}-checkpatch
+    COMMENT "Running checkpatch"
+    COMMAND
+      ${CHECKPATCH}
+        # Description of supported checks in
+        # 
https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst
+        --codespell
+        --color=always
+        --git ${GIT_MASTER_BRANCH}..HEAD
+        --show-types
+        --ignore BAD_SIGN_OFF
+        --ignore BLOCK_COMMENT_STYLE
+        --ignore CODE_INDENT
+        --ignore COMMIT_LOG_LONG_LINE
+        # Requires at least two lines in commit message and this
+        # is annoying.
+        --ignore COMMIT_MESSAGE
+        --ignore CONSTANT_COMPARISON
+        --ignore FUNCTION_NAME_NO_NEWLINE
+        --ignore GIT_COMMIT_ID
+        --ignore INCLUDE_GUARD
+        --ignore LOGICAL_CONTINUATIONS
+        --ignore LONG_LINE
+        --ignore NO_CHANGELOG
+        --ignore NO_DOC
+        --ignore NO_TEST
+        --ignore PREFER_DEFINED_ATTRIBUTE_MACRO
+        --ignore SPACING
+        --ignore SUSPECT_CODE_INDENT
+        --ignore TABSTOP
+        --ignore TRAILING_STATEMENTS
+        --ignore UNCOMMENTED_DEFINITION
+        --ignore UNSAFE_FUNCTION
+    WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
+  )
+else()
+  set(MSG "`checkpatch.pl' is not found, so ${PROJECT_NAME}-checkpatch 
target is dummy")
+  add_custom_command(TARGET ${PROJECT_NAME}-checkpatch
+    COMMAND ${CMAKE_COMMAND} -E cmake_echo_color --red ${MSG}
+    COMMENT ${MSG}
+  )
+endif()
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 5ec0bed6..66f6ee77 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -42,52 +42,6 @@ else()
    )
  endif()

-find_program(CHECKPATCH checkpatch.pl
-             HINTS ${PROJECT_SOURCE_DIR}/checkpatch)
-add_custom_target(${PROJECT_NAME}-checkpatch)
-set(MASTER_BRANCH "tarantool/master")
-if(CHECKPATCH)
-  add_custom_command(TARGET ${PROJECT_NAME}-checkpatch
-    COMMENT "Running checkpatch"
-    COMMAND
-      ${CHECKPATCH}
-        # Description of supported checks in
-        # 
https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst
-        --codespell
-        --color=always
-        --git ${MASTER_BRANCH}..HEAD
-        --show-types
-        --ignore BAD_SIGN_OFF
-        --ignore BLOCK_COMMENT_STYLE
-        --ignore CODE_INDENT
-        --ignore COMMIT_LOG_LONG_LINE
-        # Requires at least two lines in commit message and this
-        # is annoying.
-        --ignore COMMIT_MESSAGE
-        --ignore CONSTANT_COMPARISON
-        --ignore FUNCTION_NAME_NO_NEWLINE
-        --ignore GIT_COMMIT_ID
-        --ignore INCLUDE_GUARD
-        --ignore LOGICAL_CONTINUATIONS
-        --ignore LONG_LINE
-        --ignore NO_CHANGELOG
-        --ignore NO_DOC
-        --ignore NO_TEST
-        --ignore PREFER_DEFINED_ATTRIBUTE_MACRO
-        --ignore SPACING
-        --ignore SUSPECT_CODE_INDENT
-        --ignore TABSTOP
-        --ignore TRAILING_STATEMENTS
-        --ignore UNCOMMENTED_DEFINITION
-        --ignore UNSAFE_FUNCTION
-    WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
-  )
-else()
-  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
  )


>         + 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
>         +)
>         +
>          set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e
>         dofile[[${LUAJIT_TEST_INIT}]]")
>          separate_arguments(LUAJIT_TEST_COMMAND)
>
>         @@ -75,5 +125,6 @@ if(LUAJIT_USE_TEST)
>            add_custom_target(test DEPENDS
>              ${PROJECT_NAME}-test
>              ${PROJECT_NAME}-luacheck
>         + ${PROJECT_NAME}-checkpatch
>            )
>          endif()
>         --
>         2.34.1
>
>     --
>     Best regards,
>     Maxim Kokryashkin
>

[-- Attachment #2: Type: text/html, Size: 16388 bytes --]