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

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

[Alexander Turenko]

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.

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:-}"
+
+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
-- 
2.25.0

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

[Sergey Bronnikov]

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

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

[Alexander Turenko via Tarantool-patches]

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

Out of source build would not work this way if the build directory is
outside of the source directory. I don't think that it is acceptable.

I'll rewrite my solution to CMake to don't spread logic across files.

<..stripped code and testing results..>

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

It would be nice in theory, but it would require quite accurate handling
of paths that contain spaces. Likely IFS redefinition and so on. I would
not step into this until there will be a real need, but rather provide a
simple wrapper that works always.

Anyway, now everything in CMake (see patch v2).