Hi, Igor


thanks for the patch! see my comments


Sergey

On 8/3/23 10:30, Igor Munkin wrote:
This patch introduces a separate target to run flake8 against all Python
scripts within LuaJIT repository (i.e. debugger extensions). There are
some tweaks in .flake8rc regarding our style: one can find more info in
the config file.

The new target is a dependency for the new <LuaJIT-lint> target, that
joins both luacheck and flake8 linter runs. CI job with linters is
adjusted respectively.

Signed-off-by: Igor Munkin <imun@tarantool.org>
---
 .flake8rc                  | 12 ++++++++++++
 .github/workflows/lint.yml |  4 ++--
 test/CMakeLists.txt        | 28 ++++++++++++++++++++++++++++
 3 files changed, 42 insertions(+), 2 deletions(-)
 create mode 100644 .flake8rc

diff --git a/.flake8rc b/.flake8rc
new file mode 100644
index 00000000..b6f7ad48
--- /dev/null
+++ b/.flake8rc
@@ -0,0 +1,12 @@
+[flake8]
+extend-ignore =
+  # TODO: I have no idea, how to fix this. flake8 suggests nothing
+  # (like clang-format or checkpatch.pl do). Help needed.
+  E131,
+  # TODO: I have no idea, how to fix this. flake8 suggests nothing
+  # (like clang-format or checkpatch.pl do). Help needed.
+  E501,
+  # XXX: Suppress F821, since we have autogenerated names for
+  # 'ptr' type complements in luajit_lldb.py.
+  F821
+max-line-length = 80
diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
index 70c98104..6b420f68 100644
--- a/.github/workflows/lint.yml
+++ b/.github/workflows/lint.yml
@@ -41,7 +41,7 @@ jobs:
         run: |
           # TODO: Move this step to a separate action.
           sudo apt -y update
-          sudo apt -y install cmake ninja-build lua5.1 luarocks
+          sudo apt -y install cmake ninja-build lua5.1 luarocks flake8
           sudo luarocks install luacheck
           # Set CMAKE_BUILD_PARALLEL_LEVEL environment variable to
           # limit the number of parallel jobs for build/test step.
@@ -49,5 +49,5 @@ jobs:
       - name: configure
         run: cmake -S . -B ${{ env.BUILDDIR }} -G Ninja
       - name: test
-        run: cmake --build . --target LuaJIT-luacheck
+        run: cmake --build . --target LuaJIT-lint
         working-directory: ${{ env.BUILDDIR }}
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 47296a22..17ac5cac 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -42,6 +42,34 @@ else()
   )
 endif()
 
+find_program(FLAKE8 flake8)
+if(FLAKE8)
+  get_filename_component(FLAKE8_SOURCE_DIR "${PROJECT_SOURCE_DIR}" REALPATH)
Nit: name FLAKE8_SOURCE_DIR is confused, because dir has nothing common with flake8. May be "REAL_SOURCE_DIR"?
+  set(FLAKE8_RC ${FLAKE8_SOURCE_DIR}/.flake8rc)
+  file(GLOB_RECURSE FLAKE8_DEPS ${FLAKE8_SOURCE_DIR}/*.py)
+  add_custom_target(${PROJECT_NAME}-flake8
+    DEPENDS ${FLAKE8_DEPS}
+  )
+  add_custom_command(TARGET ${PROJECT_NAME}-flake8
+    COMMENT "Running flake8 static analysis"
+    COMMAND
+      ${FLAKE8} ${FLAKE8_DEPS}
+        --config ${FLAKE8_RC}
+        --jobs ${CMAKE_BUILD_PARALLEL_LEVEL}
+    WORKING_DIRECTORY ${FLAKE8_SOURCE_DIR}
+  )
+else()
+  add_custom_target(${PROJECT_NAME}-flake8)
+  add_custom_command(TARGET ${PROJECT_NAME}-flake8
+    COMMENT "`flake8' is not found, so ${PROJECT_NAME}-flake8 target is dummy"

Please add a command to a dummy target:

COMMAND ${CMAKE_COMMAND} -E cmake_echo_color --red "`flake8' is not found, so ${PROJECT_NAME}-flake8 target is dummy"

with added COMMAND target will print a message

+  )
+endif()
+
+add_custom_target(${PROJECT_NAME}-lint DEPENDS
+  ${PROJECT_NAME}-luacheck
+  ${PROJECT_NAME}-flake8
+)
+
 set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
 separate_arguments(LUAJIT_TEST_COMMAND)
 

You've introduced a new target LuaJIT-lint, that includes LuaJIT-luacheck and LuaJIT-flake8.

I suppose we need replace dependence "LuaJIT-luacheck" to "LuaJIT-lint" for a target "test".