Re: [Tarantool-patches] [PATCH luajit 15/15] test: run flake8 static analysis via CMake

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

[-- Attachment #1: Type: text/plain, Size: 4135 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 7168 bytes --]