Hi, Max

thanks for review! see my comments

Hi, Sergey!

Please consider my comments below.

 

Среда, 11 октября 2023, 19:54 +03:00 от Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>:
 

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

Typo: s/a whitelist/the whitelist/

Fixed.

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
index eebf3d6f..7ef10f2f 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -32,6 +32,7 @@ set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
 include(LuaJITUtils)
 include(SetBuildParallelLevel)
 include(SetVersion)
+include(CodeSpell)
 
 # --- Variables to be exported to child scopes ---------------------------------
 
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.

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

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