Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Maxim Kokryashkin <m.kokryashkin@tarantool.org>
Cc: Sergey Bronnikov <estetus@gmail.com>,
	max.kokryashkin@gmail.com, tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit 3/4][v2] cmake: introduce target with codespell
Date: Tue, 17 Oct 2023 17:50:16 +0300	[thread overview]
Message-ID: <2f093b55-cec5-4cfa-a452-bc79598278a7@tarantool.org> (raw)
In-Reply-To: <jbssdubnkimuv6hjslfiuath4wfhw5aa4a3v72jp4dmpmmbp3n@hpnxpttzlokj>

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

  reply	other threads:[~2023-10-17 14:50 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-11 16:52 [Tarantool-patches] [PATCH luajit 0/4][v2] Fix typos and enable codespell Sergey Bronnikov via Tarantool-patches
2023-10-11 16:52 ` [Tarantool-patches] [PATCH luajit 1/4][v2] codehealth: fix typos Sergey Bronnikov via Tarantool-patches
2023-10-12 10:33   ` Maxim Kokryashkin via Tarantool-patches
2023-10-23  8:50   ` Sergey Kaplun via Tarantool-patches
2023-10-11 16:52 ` [Tarantool-patches] [PATCH luajit 2/4][v2] test: fix codestyle Sergey Bronnikov via Tarantool-patches
2023-10-12 10:34   ` Maxim Kokryashkin via Tarantool-patches
2023-10-23  8:52   ` Sergey Kaplun via Tarantool-patches
2023-10-23 14:13     ` Sergey Bronnikov via Tarantool-patches
2023-10-23 14:27       ` Sergey Kaplun via Tarantool-patches
2023-10-11 16:52 ` [Tarantool-patches] [PATCH luajit 3/4][v2] cmake: introduce target with codespell Sergey Bronnikov via Tarantool-patches
2023-10-11 19:33   ` Sergey Bronnikov via Tarantool-patches
2023-10-12 10:43   ` Maxim Kokryashkin via Tarantool-patches
2023-10-12 13:28     ` Sergey Bronnikov via Tarantool-patches
2023-10-12 18:46       ` Maxim Kokryashkin via Tarantool-patches
2023-10-17 14:50         ` Sergey Bronnikov via Tarantool-patches [this message]
2023-10-23  9:20   ` Sergey Kaplun via Tarantool-patches
2023-10-23 12:29     ` Sergey Bronnikov via Tarantool-patches
2023-10-23 14:38       ` Sergey Kaplun via Tarantool-patches
2023-10-31  6:42         ` Sergey Kaplun via Tarantool-patches
2023-10-31 10:50           ` Sergey Bronnikov via Tarantool-patches
2023-10-31 11:31             ` Sergey Kaplun via Tarantool-patches
2023-10-31 11:53             ` Maxim Kokryashkin via Tarantool-patches
2023-10-11 16:52 ` [Tarantool-patches] [PATCH luajit 4/4][v2] ci: enable codespell Sergey Bronnikov via Tarantool-patches
2023-10-12 10:45   ` Maxim Kokryashkin via Tarantool-patches
2023-10-23  8:53   ` Sergey Kaplun via Tarantool-patches
2023-11-16 16:19 ` [Tarantool-patches] [PATCH luajit 0/4][v2] Fix typos and " Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2f093b55-cec5-4cfa-a452-bc79598278a7@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=estetus@gmail.com \
    --cc=m.kokryashkin@tarantool.org \
    --cc=max.kokryashkin@gmail.com \
    --cc=sergeyb@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit 3/4][v2] cmake: introduce target with codespell' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox