Re: [Tarantool-patches] [PATCH] tools: fix luacheck invocation in different cases

[Sergey Bronnikov <sergeyb@tarantool.org>]

Hello,

thanks for the patch!

see my two comments below

On 01.12.2020 15:32, Alexander Turenko wrote:
> Now `make luacheck` gracefully handles different cases: in-source and
> out-of-source build (within the source tree or outside), current working
> directory as a real path or with symlink components.

1. We already discussed it verbally and personally I don't like 
suggested solution with wrapper

because it makes the code more complex.

 From side I have another solution that much more simple:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index fa6818f8e..b162b28da 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -159,8 +159,11 @@ add_custom_target(ctags DEPENDS tags)
  #

  add_custom_target(luacheck)
+# luacheck requires a real path to a directory
+# see https://github.com/mpeterv/luacheck/issues/208
+get_filename_component(REALPATH ${PROJECT_SOURCE_DIR} REALPATH)
  add_custom_command(TARGET luacheck
-    COMMAND ${LUACHECK} --codes --config 
"${PROJECT_SOURCE_DIR}/.luacheckrc" "${PROJECT_SOURCE_DIR}"
+    COMMAND ${LUACHECK} --codes --config .luacheckrc "${REALPATH}"
      COMMENT "Perform static analysis of Lua code"
  )

Let's go through all scenarios and compare both solutions:

1. separate build dir inside a source directory, both are real paths

wrapper - SUCCESS

cmake fix - SUCESS

2. build dir is the same as source dir, it is a real path

wrapper - SUCCESS

cmake fix - SUCCESS, but checked 46 files instead of 45 (perhaps it 
checks additional path?)

3. build dir on the same level as a source dir in a dir tree, both are 
real paths

wrapper - SUCCESS

cmake fix - FAIL

sergeyb@pony:~/sources/MRG/build$ make luacheck
Perform static analysis of Lua code
Critical error: Couldn't find configuration file .luacheckrc
make[3]: *** [CMakeFiles/luacheck.dir/build.make:58: luacheck] Error 4
make[2]: *** [CMakeFiles/Makefile2:1251: CMakeFiles/luacheck.dir/all] 
Error 2
make[1]: *** [CMakeFiles/Makefile2:1258: CMakeFiles/luacheck.dir/rule] 
Error 2
make: *** [Makefile:225: luacheck] Error 2

4. build dir is the same as a source dir and source dir is a symlink to 
a real source dir

sergeyb@pony:~/sources/MRG$ readlink -f tarantool-symlink/
/home/sergeyb/sources/MRG/tarantool/

wrapper - SUCCESS, but checked 46 files instead of 45 (perhaps it checks 
additional path?)

cmake fix - SUCCESS, but checked 46 files instead of 45 (perhaps it 
checks additional path?)

4. separate build dir is inside a source dir and source dir is a symlink 
to a real source dir

sergeyb@pony:~/sources/MRG$ readlink -f tarantool-symlink/build/
/home/sergeyb/sources/MRG/tarantool/build

wrapper - SUCCESS

cmake fix - SUCCESS

>
> As result of looking into those problems I filed the issue [1] against
> luacheck. It seems, there are problems around absolute paths with
> symlinks components.
>
> [1]: https://github.com/mpeterv/luacheck/issues/208
> ---
>
> no issue
> Totktonada/fix-luacheck-invocation
> https://github.com/tarantool/tarantool/tree/Totktonada/fix-luacheck-invocation
>
>   CMakeLists.txt        |  5 ++-
>   tools/run-luacheck.sh | 71 +++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 73 insertions(+), 3 deletions(-)
>   create mode 100755 tools/run-luacheck.sh
>
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index fa6818f8e..10206b943 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -25,7 +25,6 @@ find_program(BASH bash)
>   find_program(GIT git)
>   find_program(LD ld)
>   find_program(CTAGS ctags)
> -find_program(LUACHECK luacheck ENV PATH)
>   
>   # Define PACKAGE macro in tarantool/config.h
>   set(PACKAGE "Tarantool" CACHE STRING "Package name.")
> @@ -157,10 +156,10 @@ add_custom_target(ctags DEPENDS tags)
>   #
>   # Enable 'make luacheck' target.
>   #
> -
>   add_custom_target(luacheck)
>   add_custom_command(TARGET luacheck
> -    COMMAND ${LUACHECK} --codes --config "${PROJECT_SOURCE_DIR}/.luacheckrc" "${PROJECT_SOURCE_DIR}"
> +    COMMAND "${PROJECT_SOURCE_DIR}/tools/run-luacheck.sh"
> +            "${PROJECT_SOURCE_DIR}" "${PROJECT_BINARY_DIR}"
>       COMMENT "Perform static analysis of Lua code"
>   )
>   
> diff --git a/tools/run-luacheck.sh b/tools/run-luacheck.sh
> new file mode 100755
> index 000000000..e6ebb78b3
> --- /dev/null
> +++ b/tools/run-luacheck.sh
> @@ -0,0 +1,71 @@
> +#!/bin/sh
> +
> +set -eux
> +
> +SOURCE_DIR="${1:-}"
> +BUILD_DIR="${2:-}"

2. I don't like such approach. I think wrapper should be transparent for 
user

so that it allow to pass any luacheck option and it will be passed to a 
real luacheck.

Otherwise you need to modify wrapper when you need to pass any other 
luacheck option.

> +
> +if [ -z "${SOURCE_DIR}" ] || [ -z "${BUILD_DIR}" ]; then
> +    printf "Usage: ${0} /path/to/the/source/tree /path/to/the/build/directory\n"
> +    exit 1
> +fi
> +
> +if ! type luacheck; then
> +    printf "Unable to find luacheck\n"
> +    exit 1
> +fi
> +
> +# Workaround luacheck behaviour around the `include_files`
> +# configuration option, a current working directory and a
> +# directory path passed as an argument.
> +#
> +# https://github.com/mpeterv/luacheck/issues/208
> +#
> +# If we'll just invoke the following command under the
> +# `make luacheck` target:
> +#
> +#  | luacheck --codes                                 \
> +#  |     --config "${PROJECT_SOURCE_DIR}/.luacheckrc" \
> +#  |     "${PROJECT_SOURCE_DIR}"
> +#
> +# The linter will fail to find Lua sources in the following cases:
> +#
> +# 1. In-source build. The current working directory is not a
> +#    real path (contains components, which are symlinks).
> +# 2. Out-of-source build.
> +#
> +# It seems, the only reliable way to verify sources is to change
> +# the current directory prior to the luacheck call.
> +cd "${SOURCE_DIR}"
> +
> +# Exclude the build directory if it is under the source directory.
> +#
> +# Except the case, when the build directory is the same as the
> +# source directory.
> +#
> +# We lean on the following assumptions:
> +#
> +# 1. "${SOURCE_DIR}" and "${BUILD_DIR}" have no the trailing slash.
> +# 2. "${SOURCE_DIR}" and "${BUILD_DIR}" are either real paths
> +#    (with resolved symlink components) or absolute paths with the
> +#    same symlink components (where applicable).
> +#
> +# Those assumptions should be true when the variables are passed
> +# from the CMake variables "${PROJECT_SOURCE_DIR}" and
> +# "${PROJECT_BINARY_DIR}".
> +#
> +# When the prerequisites are hold true, the EXCLUDE_FILES pattern
> +# will be relative to the "${SOURCE_DIR}" and luacheck will work
> +# as expected.
> +EXCLUDE_FILES=""
> +case "${BUILD_DIR}" in
> +"${SOURCE_DIR}/"*)
> +    EXCLUDE_FILES="${BUILD_DIR#"${SOURCE_DIR}/"}/**/*.lua"
> +    ;;
> +esac
> +
> +if [ -z "${EXCLUDE_FILES}" ]; then
> +    luacheck --codes --config .luacheckrc .
> +else
> +    luacheck --codes --config .luacheckrc . --exclude-files "${EXCLUDE_FILES}"
> +fi