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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
LGTM, but I am really concerned about that `src/*` supression problem. 
Looking forward to see the patch with supression in this series.

On Wed, Aug 09, 2023 at 05:55:02PM +0300, Sergey Bronnikov wrote:
> 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?
Agree with Sergey Bronnikov here.
> > 
> > >    # --- 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>
Best regards, Maxim Kokryashkin
> >