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

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

Maxim,


On 10/12/23 21:46, Maxim Kokryashkin wrote:
> 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.

I don't get why it is a problem. However, I have updated a patch to 
reduce a number of `list` operators.

--- a/cmake/CodeSpell.cmake
+++ b/cmake/CodeSpell.cmake
@@ -1,15 +1,17 @@
  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)
+string(JOIN "," CODESPELL_WHITELIST
+  ${PROJECT_SOURCE_DIR}/src/lj_mapi.c
+  ${PROJECT_SOURCE_DIR}/src/lj_sysprof.c
+  ${PROJECT_SOURCE_DIR}/src/lj_utils_leb128.c
+  ${PROJECT_SOURCE_DIR}/src/lj_wbuf.c
+  ${PROJECT_SOURCE_DIR}/src/luajit-gdb.py
+  ${PROJECT_SOURCE_DIR}/src/luajit_lldb.py
+  ${PROJECT_SOURCE_DIR}/test/CMakeLists.txt
+  ${PROJECT_SOURCE_DIR}/test/tarantool-c-tests
+  ${PROJECT_SOURCE_DIR}/test/tarantool-tests
+  ${PROJECT_SOURCE_DIR}/tools
+)

  set(IGNORE_WORDS ${PROJECT_SOURCE_DIR}/tools/codespell-ignore-words.txt)


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

Actually no. When codespell is not found CMake just prints a message 
about successful codespell running:

$ cmake -S . -B build
-- The C compiler identification is GNU 11.4.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- [SetVersion] Reading version from VCS: v2.1.0-beta3-405-g52d646ed
-- [SetBuildParallelLevel] CMAKE_BUILD_PARALLEL_LEVEL is 8
-- The ASM compiler identification is GNU
-- Found assembler: /bin/cc
-- Configuring done
-- Generating done
-- Build files have been written to: 
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/build
$ cmake --build build --parallel -t LuaJIT-codespell
Built target LuaJIT-codespell

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

Ok, then we should implement the same approach for targets with prove.

This is out of scope patches with codespell support.

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