[Tarantool-patches] [PATCH luajit 3/4][v2] cmake: introduce target with codespell
Maxim Kokryashkin
m.kokryashkin at tarantool.org
Thu Oct 12 21:46:06 MSK 2023
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
More information about the Tarantool-patches
mailing list