From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 3B22745C304 for ; Wed, 16 Dec 2020 19:47:53 +0300 (MSK) References: From: Sergey Bronnikov Message-ID: <872de52f-3250-a673-9161-77d5e0543795@tarantool.org> Date: Wed, 16 Dec 2020 19:47:51 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [Tarantool-patches] [PATCH] tools: fix luacheck invocation in different cases List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.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