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

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

Hi, Sergey!

On 8/9/23 17:04, Sergey Kaplun wrote:
> 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/

Fixed.


>
> <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.
No, this var is required for gcovr too and will be used by it in patches 
with coverage support.
>
>>    include(LuaJITUtils)
>>    include(SetBuildParallelLevel)
>>    include(SetVersion)
>> +include(CheckPatch)
> And be included from test/CMakeLists.txt, not from root CMakeLists.txt.
Why? How checkpatch is related to testing?
>
>>    # --- 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/

Fixed.


>> +        # is annoying.
> But we always have at least two lines, don't we?
Sometimes no. See commit "codehealth: fix typos" in patch series.
>
>> +        --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


Added.

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

Agree, but this requires patching checkpatch itself, right now it is not 
possible

to suppress warnings for a certain files in patch. I'll try to implement 
such patch.


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

Updated.


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