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

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

Hi, Sergey!
Thanks for the patch!

It's really amazing changes: I've already found some typos in my
commit messages and comments!:)

Please, consider my comments below.

On 04.08.23, Sergey Bronnikov via Tarantool-patches wrote:
> Hi, Max
> 
> On 8/3/23 22:38, Maxim Kokryashkin via Tarantool-patches wrote:
> > Hi, Sergey!
> > Please consider my comments below.

<snipped>

> >
> >         Patch introduces two new CMake targets: "LuaJIT-checkpatch",

Typo: s/Patch/The patch/

<snipped>

> >
> >     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.

<snipped>

> 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")
> +

I suppose this should be moved to <cmake/CheckPatch.cmake> too.

>   include(LuaJITUtils)
>   include(SetBuildParallelLevel)
>   include(SetVersion)
> +include(CheckPatch)

And be included from test/CMakeLists.txt, not from root CMakeLists.txt.

> 
>   # --- 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

Typo: s/in commit message/in the commit message/
Typo: s/ and/, and/

> +        # is annoying.

But we always have at least two lines, don't we?

> +        --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

I suppose we should add `--ignore PREFER_FALLTHROUGH`, too.
| ERROR:PREFER_FALLTHROUGH: Prefer 'FALLTHROUGH;' over fallthrough comment

Since it's already used in the codebase:
| $ grep fallthrough -R . | wc -l
| 47

> +    WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}

Also, we can't just check all patches with this since it will have
TONS of errors. We must at least exclude all <src/*>, <dynasm/*> and
include back files that are maintained by us.

Without this, it becomes pointless, since we either
* will have red always checkpatch job and don't be aware, so can miss
  some errors, either
* will set ignore all (or something like it) to ignore all checkpatch
  warnings, including meaningful one.

> +  )
> +else()
> +  set(MSG "`checkpatch.pl' is not found, so ${PROJECT_NAME}-checkpatch 
> target is dummy")

Please, split this line into several.

Minor: I suggest to rename it to the WARN_MSG.
Feel free to ignore.

> +  add_custom_command(TARGET ${PROJECT_NAME}-checkpatch
> +    COMMAND ${CMAKE_COMMAND} -E cmake_echo_color --red ${MSG}
> +    COMMENT ${MSG}
> +  )
> +endif()

<snipped>

> >

-- 
Best regards,
Sergey Kaplun