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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Igor!
Thanks for the patch!

Aside from comments left by Sergey, please consider a few of mine.

On Thu, Aug 03, 2023 at 07:30:40AM +0000, 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,
I dont't think we should disable the continuation line alignment rule (E131),
which has explanation in the docs[1] and the line length limit[2] (E501), which is
easily fixed.
> +  # 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)
> +  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}
> +  )
I suggest moving this logic to a separate cmake module. It will
make the test/CMakeLists.txt cleaner and more readable.
> +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"
> +  )
> +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)
>  
> -- 
> 2.30.2
> 
[1]: https://www.flake8rules.com/rules/E131.html
[2]: https://www.flake8rules.com/rules/E501.html

Best regards,
Maxim Kokryashkin