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

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

Sasha,

Thanks for your patch!

TL;DR: the patch LGTM (but I agree with Sergey regarding the whitespace
in <if> statement). At the same time I see a small rationale for such
complex logic, considering the upcoming LuaJIT build system refactoring
and the fact your solution doesn't work in the most general case (but
nobody asked for it).

As we discussed before we have three possible cases of configuration:
1. <bindir> do not intersect with <srcdir> ("true" OOS build)
2. <bindir> is <srcdir> (in source build)
3. <bindir> is a subdirectory within <srcdir> ("quasi" OOS build)

The first case is very simple: you need only run luacheck within
<srcdir>, since all paths in .luacheckrc are considered relative to the
current working directory. This issue is solved via WORKING_DIRECTORY
property and you even resolve all symlinks for <srcdir>.

The second case is a bit tricky: there might be autogenerated Lua chunks
(e.g. jit/vmdef.lua). These files are not considered as Lua sources per
se, so there is no need to check these files with luacheck. Then simply
exclude the whole <bindir> recursively and the issue is completely gone.
Unfortunately, it's not.

The third case is the most complex one, though it doesn't look so. In
case of in source build, those autogenerated Lua chunks are build within
<srcdir> and there is no other way than explicitly exclude those files
from the list to be checked with luacheck. We don't face this case,
since everything within third_party/luajit/ is excluded from check. I
even haven't faced this in LuaJIT submodule, since src/ directory is
excluded from the check, so src/jit/vmdef.lua is not checked. Hence if
there would be an autogenerated Lua chunk violating .luacheckrc rules in
scope of Tarantool src/ directory, you had to explicitly suppress it to
make luacheck happy.

I hope my arguments are clear enough.

Let's return to LuaJIT build system enhancements. If out of source build
is used now, LuaJIT submodule is fully copied to the <bindir>, so all
Lua sources are moved in Tarantool source tree. Hence they are checked
with luacheck and there are many warnings produced. In scope of #4862 I
reimplemented LuaJIT source tree manipulations, so all LuaJIT sources
are left within third_party/luajit despite to the chosen build type.

As a result, there is a single Lua file violating luacheck warnings:
jit/vmdef.lua that is generated within Tarantool source tree (in "quasi"
out of source build case). It looks to be much easier to explicitly
exclude this single file via --exclude-files option and leave the
comment with the rationale, since you complex solution doesn't work in a
general case.

Anyway, this is not a major point against applying your changes, but
rather common sense. Everything except the point above is OK, so if you
are sure with your solution feel free to proceed with the patch.

On 18.02.21, 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.
> 
> 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
> 
> Changes since v1:
> 
> * Moved the logic to CMake, dropped the shell wrapper.
> * Shrink comments.
> * Handled the case, when a build directory is in the source directory,
>   and cmake is called not like `cmake ..`, but `cmake /path/to/source`,
>   where the path is not a real path.
> 
>  CMakeLists.txt    | 28 ++++++++++++++++++++++++++--
>  cmake/utils.cmake | 22 ++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 2 deletions(-)

<snipped>

> diff --git a/cmake/utils.cmake b/cmake/utils.cmake
> index eaec821b3..e9b5fed5d 100644
> --- a/cmake/utils.cmake
> +++ b/cmake/utils.cmake
> @@ -86,3 +86,25 @@ function(bin_source varname srcfile dstfile)
>  
>  endfunction()
>  
> +#
> +# Whether a file is descendant to a directory.
> +#
> +# If the file is the directory itself, the answer is FALSE.
> +#
> +function(file_is_in_directory varname file dir)
> +    file(RELATIVE_PATH file_relative "${dir}" "${file}")
> +    if (file_relative STREQUAL "")
> +        # <file> and <dir> is the same directory.
> +        set(${varname} FALSE PARENT_SCOPE)
> +    elseif (file_relative STREQUAL "..")
> +        # <dir> inside a <file> (so it is a directory too), not
> +        # vice versa.
> +        set(${varname} FALSE PARENT_SCOPE)

It looks this branch is excess and is covered by the next one (if you
remove the trailing slash).

> +    elseif (file_relative MATCHES "^\\.\\./")
> +        # <file> somewhere outside of the <dir>.
> +        set(${varname} FALSE PARENT_SCOPE)
> +    else()
> +        # <file> is descendant to <dir>.
> +        set(${varname} TRUE PARENT_SCOPE)
> +    endif()
> +endfunction()
> -- 
> 2.30.0
> 

-- 
Best regards,
IM