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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergey,

Thanks for your review!

On 03.08.23, Sergey Bronnikov wrote:
> 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
> > 

<snipped>

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

The semantics are the same as for LUACHECK_SOURCE_DIR: it should be read
as "the directory with sources to be checked via flake8". Ignoring.

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

Well, 0 days since "CMake is doing something unexpected". COMMENT is
totally fine, the problem is in Ninja generated artefacts: I'm not an
expert in Ninja but looks like CMake generates kinda nop if COMMAND is
omitted. Here is the dump:

| $ cmake --version
| cmake version 3.26.4
|
| CMake suite maintained and supported by Kitware (kitware.com/cmake).
| $ rm -f CMakeCache.txt
| $ cmake . -G Ninja
| -- The C compiler identification is GNU 13.2.0
| -- Detecting C compiler ABI info
| -- Detecting C compiler ABI info - done
| -- Check for working C compiler: /usr/bin/cc - skipped
| -- Detecting C compile features
| -- Detecting C compile features - done
| -- [SetVersion] Reading version from VCS: v2.1.0-beta3-355-g6dd0b0e2
| -- [SetBuildParallelLevel] CMAKE_BUILD_PARALLEL_LEVEL is 4
| -- The ASM compiler identification is GNU
| -- Found assembler: /usr/bin/cc
| -- Configuring done (0.3s)
| -- Generating done (0.0s)
| -- Build files have been written to: /home/imun/projects/tarantool-luajit
| $ cmake --build . --target LuaJIT-flake8
| ninja: no work to do.
| $ rm -f CMakeCache.txt
| $ cmake .
| -- The C compiler identification is GNU 13.2.0
| -- Detecting C compiler ABI info
| -- Detecting C compiler ABI info - done
| -- Check for working C compiler: /usr/bin/cc - skipped
| -- Detecting C compile features
| -- Detecting C compile features - done
| -- [SetVersion] Reading version from VCS: v2.1.0-beta3-355-g6dd0b0e2
| -- [SetBuildParallelLevel] CMAKE_BUILD_PARALLEL_LEVEL is 4
| -- The ASM compiler identification is GNU
| -- Found assembler: /usr/bin/cc
| -- Configuring done (0.3s)
| -- Generating done (0.1s)
| -- Build files have been written to: /home/imun/projects/tarantool-luajit
| $ cmake --build . --target LuaJIT-flake8
| `flake8' is not found, so LuaJIT-flake8 target is dummy
| Built target LuaJIT-flake8

Hence, I suggest to leave everything as is (see luacheck-related part),
but in scope of the CTest series implement another way of interaction
with user. Does it work for you?

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

Oops, my bad, thanks! Fixed.

================================================================================

diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 17ac5cac..58cba5ba 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -102,6 +102,6 @@ if(LUAJIT_USE_TEST)
 
   add_custom_target(test DEPENDS
     ${PROJECT_NAME}-test
-    ${PROJECT_NAME}-luacheck
+    ${PROJECT_NAME}-lint
   )
 endif()

================================================================================

-- 
Best regards,
IM