[Tarantool-patches] [PATCH luajit 3/4][v2] cmake: introduce target with codespell
Sergey Bronnikov
sergeyb at tarantool.org
Tue Oct 17 17:50:16 MSK 2023
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
More information about the Tarantool-patches
mailing list