Re: [Tarantool-patches] [PATCH luajit 3/4][v2] cmake: introduce target with codespell

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

Sergey, 
On Thu, Oct 12, 2023 at 04:28:36PM +0300, Sergey Bronnikov wrote:
> Hi, Max
> 
> 
> thanks for review! see my comments
> 
<snipped>
 > 
> >     diff --git a/cmake/CodeSpell.cmake b/cmake/CodeSpell.cmake
> >     new file mode 100644
> >     index 00000000..c4d3555d
> >     --- /dev/null
> >     +++ b/cmake/CodeSpell.cmake
> >     @@ -0,0 +1,36 @@
> >     +find_program(CODESPELL codespell)
> >     +
> >     +list(APPEND CODESPELL_WHITELIST ${PROJECT_SOURCE_DIR}/src/lj_mapi.c)
> >     +list(APPEND CODESPELL_WHITELIST
> >     ${PROJECT_SOURCE_DIR}/src/lj_sysprof.c)
> >     +list(APPEND CODESPELL_WHITELIST
> >     ${PROJECT_SOURCE_DIR}/src/lj_utils_leb128.c)
> >     +list(APPEND CODESPELL_WHITELIST ${PROJECT_SOURCE_DIR}/src/lj_wbuf.c)
> >     +list(APPEND CODESPELL_WHITELIST
> >     ${PROJECT_SOURCE_DIR}/src/luajit-gdb.py)
> >     +list(APPEND CODESPELL_WHITELIST
> >     ${PROJECT_SOURCE_DIR}/src/luajit_lldb.py)
> >     +list(APPEND CODESPELL_WHITELIST
> >     ${PROJECT_SOURCE_DIR}/test/CMakeLists.txt)
> >     +list(APPEND CODESPELL_WHITELIST
> >     ${PROJECT_SOURCE_DIR}/test/tarantool-c-tests)
> >     +list(APPEND CODESPELL_WHITELIST
> >     ${PROJECT_SOURCE_DIR}/test/tarantool-tests)
> >     +list(APPEND CODESPELL_WHITELIST ${PROJECT_SOURCE_DIR}/tools)
> > 
> > CMake’s list is variadic, you can add all entries in one go.
> 
> Sure, I know. This way it looks better and changes in patches will be more
> readable.

I don't get how it is more readable. For me it's quite the opposite -- there is
too much repetitions of the same pattern `list(APPEND CODESPELL_WHITELIST ...),
which makes the latter part with paths less readable.
> 
> 
> >     +
> >     +set(IGNORE_WORDS
> >     ${PROJECT_SOURCE_DIR}/tools/codespell-ignore-words.txt)
> >     +
> >     +add_custom_target(${PROJECT_NAME}-codespell)
> >     +if (CODESPELL)
> >     + add_custom_command(TARGET ${PROJECT_NAME}-codespell
> >     + COMMENT "Running codespell"
> >     + COMMAND
> >     + ${CODESPELL}
> >     + --ignore-words ${IGNORE_WORDS}
> >     + --skip ${IGNORE_WORDS}
> >     + --ignore-words-list fpr
> >     + --check-filenames
> >     + ${CODESPELL_WHITELIST}
> >     + WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
> >     + )
> >     +else ()
> >     + set(WARN_MSG "`codespell' is not found, "
> >     + "so ${PROJECT_NAME}-codespell target is dummy")
> >     + add_custom_command(TARGET ${PROJECT_NAME}-codespell
> >     + COMMAND ${CMAKE_COMMAND} -E cmake_echo_color --red ${MSG}
> >     + COMMENT ${MSG}
> > 
> > Why a `message` call with level set to WARNING is not enough?
> 
> it is a CMake target, you cannot use `message` command there.
You can just leave the target empty then. `message` can be called from
anywhere.
> 
> 
> > Is there a concern
> > about coloring?
> 
> for me warning highlighted by red color will be more notable than without
> it.
> 
> 
> > If so, I believe it’s inconsistent to use approach with cmake_echo_color.
> > In case of `prove` absence on the machine, the more important message
> > about
> > skipped regression tests will be less noticeable in comparison with
> > codespell, which is
> > by far less important.
> 
> >     + )
> >     +endif (CODESPELL)
> >     diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> >     index 58cba5ba..8afc42df 100644
> >     --- a/test/CMakeLists.txt
> >     +++ b/test/CMakeLists.txt
> >     @@ -68,6 +68,7 @@ endif()
> >      add_custom_target(${PROJECT_NAME}-lint DEPENDS
> >        ${PROJECT_NAME}-luacheck
> >        ${PROJECT_NAME}-flake8
> >     + ${PROJECT_NAME}-codespell
> >      )
> > 
> >      set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e
> >     dofile[[${LUAJIT_TEST_INIT}]]")
> >     diff --git a/tools/codespell-ignore-words.txt
> >     b/tools/codespell-ignore-words.txt
> >     new file mode 100644
> >     index 00000000..ceeed47c
> >     --- /dev/null
> >     +++ b/tools/codespell-ignore-words.txt
> >     @@ -0,0 +1,3 @@
> >     +mmaped
> >     +isnt
> >     +FPR
> >     --
> >     2.34.1
> > 
> > --
> > Best regards,
> > Maxim Kokryashkin