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

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

Hi, Sergey!
Thanks for the fixes!
LGTM, just minor nits below.

On 23.10.23, Sergey Bronnikov wrote:
> Hi, Sergey
> 
> On 10/23/23 12:20, Sergey Kaplun wrote:
> > Hi, Sergey!
> > Thanks for the patch!
> > Please, consider my comments below.
> >
> > On 11.10.23, Sergey Bronnikov wrote:
> >> From: Sergey Bronnikov <sergeyb@tarantool.org>
> >>
> >> The patch introduces a new CMake target: "LuaJIT-codespell", that
> >> spellchecks files specified in a whitelist by codespell [1].
> >>
> >> 1. https://github.com/codespell-project/codespell
> >> ---
> >>   CMakeLists.txt                   |  1 +
> >>   cmake/CodeSpell.cmake            | 36 ++++++++++++++++++++++++++++++++
> >>   test/CMakeLists.txt              |  1 +
> >>   tools/codespell-ignore-words.txt |  3 +++
> >>   4 files changed, 41 insertions(+)
> >>   create mode 100644 cmake/CodeSpell.cmake
> >>   create mode 100644 tools/codespell-ignore-words.txt
> >>
> >> diff --git a/CMakeLists.txt b/CMakeLists.txt
> > <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)
> > Also, the following files should be checked:
> >
> > * src/lib_misc.c
> > * src/lj_memprof.c
> > * src/lj_memprof.h
> > * src/lj_sysprof.h
> > * src/lj_utils.h
> > * src/lj_wbuf.h
> > * src/lmisclib.h
> > * test/LuaJIT-tests/CMakeLists.txt
> > * test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
> > * test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
> > * test/PUC-Rio-Lua-5.1-tests/libs/CMakeLists.txt
> > * test/lua-Harness-tests/CMakeLists.txt
> 
> 
> Added:
> 
> --- a/cmake/CodeSpell.cmake
> +++ b/cmake/CodeSpell.cmake
> @@ -1,13 +1,25 @@
>   find_program(CODESPELL codespell)
> 
>   string(JOIN "," CODESPELL_WHITELIST
> +  ${PROJECT_SOURCE_DIR}/src/lib_misc.c
>     ${PROJECT_SOURCE_DIR}/src/lj_mapi.c
> +  ${PROJECT_SOURCE_DIR}/src/lj_memprof.c
> +  ${PROJECT_SOURCE_DIR}/src/lj_memprof.h
>     ${PROJECT_SOURCE_DIR}/src/lj_sysprof.c
> +  ${PROJECT_SOURCE_DIR}/src/lj_sysprof.h
> +  ${PROJECT_SOURCE_DIR}/src/lj_utils.h
>     ${PROJECT_SOURCE_DIR}/src/lj_utils_leb128.c
>     ${PROJECT_SOURCE_DIR}/src/lj_wbuf.c
> +  ${PROJECT_SOURCE_DIR}/src/lj_wbuf.h
> +  ${PROJECT_SOURCE_DIR}/src/lmisclib.h
>     ${PROJECT_SOURCE_DIR}/src/luajit-gdb.py
>     ${PROJECT_SOURCE_DIR}/src/luajit_lldb.py
>     ${PROJECT_SOURCE_DIR}/test/CMakeLists.txt
> +  ${PROJECT_SOURCE_DIR}/test/lua-Harness-tests/CMakeLists.txt
> +  ${PROJECT_SOURCE_DIR}/test/LuaJIT-tests/CMakeLists.txt
> +  ${PROJECT_SOURCE_DIR}/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
> +  ${PROJECT_SOURCE_DIR}/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt
> + ${PROJECT_SOURCE_DIR}/test/PUC-Rio-Lua-5.1-tests/libs/CMakeLists.txt
>     ${PROJECT_SOURCE_DIR}/test/tarantool-c-tests
>     ${PROJECT_SOURCE_DIR}/test/tarantool-tests
>     ${PROJECT_SOURCE_DIR}/tools

Side note: within vim's (sort 'l' is greater then 'L', see `man ascii`),
but I'm OK with this order too.

> 
> >
> >> +
> >> +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")
> > The output is splitted (thanks CMake). I found the only way to join
> > lines via `string(CONCAT WARN_MSG <str1> <str2>)`.
> 
> Fixed:
> 
> [1] ~/sources/MRG/tarantool/third_party/luajit$ cmake --build build 
> --parallel -t LuaJIT-codespell
> codespell is not found, so LuaJIT-codespell target is dummy
> Built target LuaJIT-codespell
> 
> 
> @@ -28,8 +40,9 @@ if (CODESPELL)
>       WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
>     )
>   else ()
> -  set(WARN_MSG "codespell is not found, "
> -               "so ${PROJECT_NAME}-codespell target is dummy")
> +  set(STR1 "codespell is not found,")
> +  set(STR2 "so ${PROJECT_NAME}-codespell target is dummy")
> +  string(CONCAT WARN_MSG "${STR1} ${STR2}")

Oh, may be just the following?
| string(CONCAT 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 ${WARN_MSG}
>       COMMENT ${MSG}
> 
> >

<snipped>

> >> +++ b/tools/codespell-ignore-words.txt
> >> @@ -0,0 +1,3 @@
> >> +mmaped
> > Should it be mmapped instead (like map -> mapped)?
> removed this work from exclusion list, I don't know why but codespell is 
> not warns about it.

I suppose, that some files aren't covered...

| codespell --ignore-words tools/codespell-ignore-words.txt
| ...
| ./test/tarantool-c-tests/lj-49-bad-lightuserdata.test.c:42: mmaped ==> mapped
| ./test/tarantool-c-tests/lj-49-bad-lightuserdata.test.c:44: mmaped ==> mapped
| ./test/tarantool-c-tests/lj-49-bad-lightuserdata.test.c:45: mmaped ==> mapped
| ./test/tarantool-c-tests/lj-49-bad-lightuserdata.test.c:46: mmaped ==> mapped
| ./test/tarantool-c-tests/lj-49-bad-lightuserdata.test.c:47: mmaped ==> mapped

So, maybe we should use mmapped and also add it to the ignore-words?

> >
> >> +isnt
> >> +FPR
> >> -- 
> >> 2.34.1
> >>

-- 
Best regards,
Sergey Kaplun