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

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

[Alexander Turenko via Tarantool-patches]

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(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 4fbd19558..97cfff7ae 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -158,12 +158,36 @@ add_custom_target(ctags DEPENDS tags)
 #
 # Enable 'make luacheck' target.
 #
-
+# The code looks tricky, because of luacheck side problems
+# (see [1]).
+#
+# The following circumstances may lead to missing of source files
+# or exclusions:
+#
+# * Calling `luacheck "${dir}/.luacheckrc" "${dir}"` from
+#   outside of ${dir} or when ${dir} is not a real path.
+# * Using of a path with symlink components in --exclude-files.
+#
+# [1]: https://github.com/mpeterv/luacheck/issues/208
+#
+set(EXCLUDE_FILES)
+get_filename_component(BINARY_REALPATH "${PROJECT_BINARY_DIR}" REALPATH)
+get_filename_component(SOURCE_REALPATH "${PROJECT_SOURCE_DIR}" REALPATH)
+file_is_in_directory(BINARY_DIR_INSIDE_SOURCE_DIR "${BINARY_REALPATH}"
+                     "${SOURCE_REALPATH}")
+if (BINARY_DIR_INSIDE_SOURCE_DIR)
+    set(EXCLUDE_FILES --exclude-files "${BINARY_REALPATH}/**/*.lua")
+endif()
 add_custom_target(luacheck)
 add_custom_command(TARGET luacheck
-    COMMAND ${LUACHECK} --codes --config "${PROJECT_SOURCE_DIR}/.luacheckrc" "${PROJECT_SOURCE_DIR}"
+    COMMAND ${LUACHECK} --codes --config .luacheckrc . ${EXCLUDE_FILES}
+    WORKING_DIRECTORY ${SOURCE_REALPATH}
     COMMENT "Perform static analysis of Lua code"
 )
+unset(BINARY_REALPATH)
+unset(SOURCE_REALPATH)
+unset(BINARY_DIR_INSIDE_SOURCE_DIR)
+unset(EXCLUDE_FILES)
 
 if (WITH_JEPSEN)
     ExternalProject_Add(
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)
+    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

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

[Sergey Bronnikov via Tarantool-patches]

Hello,

thanks for the patch!

LGTM and see my comment below

On 18.02.2021 19:09, 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(-)
>
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 4fbd19558..97cfff7ae 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -158,12 +158,36 @@ add_custom_target(ctags DEPENDS tags)
>   #
>   # Enable 'make luacheck' target.
>   #
> -
> +# The code looks tricky, because of luacheck side problems
> +# (see [1]).
> +#
> +# The following circumstances may lead to missing of source files
> +# or exclusions:
> +#
> +# * Calling `luacheck "${dir}/.luacheckrc" "${dir}"` from
> +#   outside of ${dir} or when ${dir} is not a real path.
> +# * Using of a path with symlink components in --exclude-files.
> +#
> +# [1]: https://github.com/mpeterv/luacheck/issues/208
> +#
> +set(EXCLUDE_FILES)
> +get_filename_component(BINARY_REALPATH "${PROJECT_BINARY_DIR}" REALPATH)
> +get_filename_component(SOURCE_REALPATH "${PROJECT_SOURCE_DIR}" REALPATH)
> +file_is_in_directory(BINARY_DIR_INSIDE_SOURCE_DIR "${BINARY_REALPATH}"
> +                     "${SOURCE_REALPATH}")
> +if (BINARY_DIR_INSIDE_SOURCE_DIR)
extra whitespace after "if"
> +    set(EXCLUDE_FILES --exclude-files "${BINARY_REALPATH}/**/*.lua")
> +endif()
>   add_custom_target(luacheck)
>   add_custom_command(TARGET luacheck
> -    COMMAND ${LUACHECK} --codes --config "${PROJECT_SOURCE_DIR}/.luacheckrc" "${PROJECT_SOURCE_DIR}"
> +    COMMAND ${LUACHECK} --codes --config .luacheckrc . ${EXCLUDE_FILES}
> +    WORKING_DIRECTORY ${SOURCE_REALPATH}
>       COMMENT "Perform static analysis of Lua code"
>   )
> +unset(BINARY_REALPATH)
> +unset(SOURCE_REALPATH)
> +unset(BINARY_DIR_INSIDE_SOURCE_DIR)
> +unset(EXCLUDE_FILES)
>   
>   if (WITH_JEPSEN)
>       ExternalProject_Add(
> 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)
> +    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()

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

[Alexander Turenko via Tarantool-patches]

> > +if (BINARY_DIR_INSIDE_SOURCE_DIR)
>
> extra whitespace after "if"

To be honest, it was always unclear for me, which style is right and I
just tried to be consistent with the surrounding code.

For CMakeLists.txt (this file, on master):

 | $ grep '\bif(' CMakeLists.txt | wc -l
 | 10
 | $ grep '\bif (' CMakeLists.txt | wc -l
 | 22

For tarantool at whole:

 | $ find . -name CMakeLists.txt -not -path '*third_party*' -or -name '*.cmake' -not -path '*third_party*' -print | xargs -I {} grep '\bif(' {} | wc -l
 | 74
 | $ find . -name CMakeLists.txt -not -path '*third_party*' -or -name '*.cmake' -not -path '*third_party*' -print | xargs -I {} grep '\bif (' {} | wc -l
 | 163

For CMake built-in modules:

 | $ find /usr/share/cmake/Modules -name '*.cmake' -print | xargs -I {} grep '\bif(' {} | wc -l
 | 5752
 | $ find /usr/share/cmake/Modules -name '*.cmake' -print | xargs -I {} grep '\bif (' {} | wc -l
 | 1492

What the world think:

* CMake follows `if()` style in its docs ([1]).
* WebKit agreed on using `if ()` ([2]).
* cmakelint ([3]) requires `if()`.
* polysquare-cmake-linter ([4]) requires `if ()`.
* cmake-lint from cmake_format ([5]) does not complain about any variant
  by default, but the default config has the
  `separate_ctrl_name_with_space` option (False). Bug?

(JFYI: CMake autoformatter / linter issue: [6].)

If we'll attempt to apply some common sense:

1. `if()` is consistent with `else()` and `endif()`.
2. `if ()` looks usual for a C developer (visual differentiation of
   control flow constructions from function calls).

So, to be honest, I still don't know. There is no general rule neither
in tarantool, nor in the world. There is no good choice, because of the
language syntax.

My personal preference is `if ()`, because it differentiates control
flow constructions from macro / function calls (that's usual in C).

[1]: https://cmake.org/cmake/help/latest/command/if.html
[2]: http://trac.webkit.org/changeset/136790/webkit
[3]: https://github.com/cmake-lint/cmake-lint
[4]: https://github.com/polysquare/polysquare-cmake-linter
[5]: https://github.com/cheshirekow/cmake_format
[6]: https://gitlab.kitware.com/cmake/cmake/-/issues/17441

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

[Sergey Bronnikov via Tarantool-patches]

Hi,

On 25.02.2021 19:35, Alexander Turenko wrote:
>>> +if (BINARY_DIR_INSIDE_SOURCE_DIR)
>> extra whitespace after "if"
> To be honest, it was always unclear for me, which style is right and I
> just tried to be consistent with the surrounding code.
>
> For CMakeLists.txt (this file, on master):
>
>   | $ grep '\bif(' CMakeLists.txt | wc -l
>   | 10
>   | $ grep '\bif (' CMakeLists.txt | wc -l
>   | 22
>
> For tarantool at whole:
>
>   | $ find . -name CMakeLists.txt -not -path '*third_party*' -or -name '*.cmake' -not -path '*third_party*' -print | xargs -I {} grep '\bif(' {} | wc -l
>   | 74
>   | $ find . -name CMakeLists.txt -not -path '*third_party*' -or -name '*.cmake' -not -path '*third_party*' -print | xargs -I {} grep '\bif (' {} | wc -l
>   | 163
>
> For CMake built-in modules:
>
>   | $ find /usr/share/cmake/Modules -name '*.cmake' -print | xargs -I {} grep '\bif(' {} | wc -l
>   | 5752
>   | $ find /usr/share/cmake/Modules -name '*.cmake' -print | xargs -I {} grep '\bif (' {} | wc -l
>   | 1492
>
> What the world think:
>
> * CMake follows `if()` style in its docs ([1]).
> * WebKit agreed on using `if ()` ([2]).
> * cmakelint ([3]) requires `if()`.
> * polysquare-cmake-linter ([4]) requires `if ()`.
> * cmake-lint from cmake_format ([5]) does not complain about any variant
>    by default, but the default config has the
>    `separate_ctrl_name_with_space` option (False). Bug?
Thanks for analysis! We can keep whitespace as is in your patch :)
>
> (JFYI: CMake autoformatter / linter issue: [6].)
>
> If we'll attempt to apply some common sense:
>
> 1. `if()` is consistent with `else()` and `endif()`.
> 2. `if ()` looks usual for a C developer (visual differentiation of
>     control flow constructions from function calls).
>
> So, to be honest, I still don't know. There is no general rule neither
> in tarantool, nor in the world. There is no good choice, because of the
> language syntax.
>
> My personal preference is `if ()`, because it differentiates control
> flow constructions from macro / function calls (that's usual in C).
>
> [1]: https://cmake.org/cmake/help/latest/command/if.html
> [2]: http://trac.webkit.org/changeset/136790/webkit
> [3]: https://github.com/cmake-lint/cmake-lint
> [4]: https://github.com/polysquare/polysquare-cmake-linter
> [5]: https://github.com/cheshirekow/cmake_format
> [6]: https://gitlab.kitware.com/cmake/cmake/-/issues/17441

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

[Igor Munkin via Tarantool-patches]

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

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

[Alexander Turenko via Tarantool-patches]

I asked Igor for a voice discussion to clarify his points. The summary
is below.

Second and third points were mixed a bit, so I'll reword and repeat
Igor's points.

The key idea of Igor's proposal: we have two exclusions mechanisms:
.luacheckrc and --exclude-files. He stated that we can use just one.

There si also the statement that the --exclude-files (the builddir
exclusion) is necessary solely to get rid of luajit autogenerated files.

Both statements have their flaws.

The build dir exclusion is not solely for luajit generated files. We
have test files in the build directory, copied from the source directory
during `make test`. .luacheckrc contains file specific rules for
particular test files and the paths are like 'test/box/box.lua' --
relative to the source dir. It'll not match the copies in the build dir,
so we'll get warnings for them (when a build directory is under the
source directory). It seems that excluding of the builddir entirely with
--exclude-files is the easiest way to solve the problem.

I'll note that a build directory name is arbitrary, so we unable to
exclude it like `build/**/*.lua` using .luacheckrc (or we should
generate the config...).

On the other hand, we unable to entirely remove .luacheckrc exclusions,
because there is no separate build dir for in-source build (`cmake .`).

So I see the following alternatives to my solution:

1. Generate .luacheckrc and get rid of --exclude-files.
   - So we should store a template in the repo, generate the file.
   - We unable to call luacheck directly, without cmake/make (or should
     explicitly set the template as the config).
   - I dislike this alternative, it is too complex and restrictive.
2. Use exclusions / matching patterns like '**/box/box.lua' in
   .luacheckrc to match a build directory content too.
   - This looks fragile: it may be broken by a build layout / test
     vardir layout changes.
   - It'll not be quite obvious, why the config is written in such
     strange way.
   - I'm not sure that such path patterns will not match something that
     we don't intend to match.
   - I don't know, to be honest, whether it'll work at all.

So I would keep the proposed solution, because it still looks as the
simplest one.

Igor, please, agree explicitly or challenge my point.

BTW, while looking into the problem with Igor, we found that luajit's
luacheck rule fails on source / build paths with symlinks components.
The solution is to use real paths everywhere.

On Fri, Feb 26, 2021 at 09:11:58PM +0300, Igor Munkin wrote:
> 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

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

[Igor Munkin via Tarantool-patches]

Sasha,

On 03.03.21, Alexander Turenko wrote:
> I asked Igor for a voice discussion to clarify his points. The summary
> is below.
> 
> Second and third points were mixed a bit, so I'll reword and repeat
> Igor's points.
> 
> The key idea of Igor's proposal: we have two exclusions mechanisms:
> .luacheckrc and --exclude-files. He stated that we can use just one.
> 
> There si also the statement that the --exclude-files (the builddir
> exclusion) is necessary solely to get rid of luajit autogenerated files.
> 
> Both statements have their flaws.
> 
> The build dir exclusion is not solely for luajit generated files. We
> have test files in the build directory, copied from the source directory
> during `make test`. .luacheckrc contains file specific rules for
> particular test files and the paths are like 'test/box/box.lua' --
> relative to the source dir. It'll not match the copies in the build dir,
> so we'll get warnings for them (when a build directory is under the
> source directory). It seems that excluding of the builddir entirely with
> --exclude-files is the easiest way to solve the problem.
> 
> I'll note that a build directory name is arbitrary, so we unable to
> exclude it like `build/**/*.lua` using .luacheckrc (or we should
> generate the config...).
> 
> On the other hand, we unable to entirely remove .luacheckrc exclusions,
> because there is no separate build dir for in-source build (`cmake .`).
> 
> So I see the following alternatives to my solution:
> 
> 1. Generate .luacheckrc and get rid of --exclude-files.
>    - So we should store a template in the repo, generate the file.
>    - We unable to call luacheck directly, without cmake/make (or should
>      explicitly set the template as the config).
>    - I dislike this alternative, it is too complex and restrictive.

Yes, it's overcomplicated for maintenance.

> 2. Use exclusions / matching patterns like '**/box/box.lua' in
>    .luacheckrc to match a build directory content too.
>    - This looks fragile: it may be broken by a build layout / test
>      vardir layout changes.
>    - It'll not be quite obvious, why the config is written in such
>      strange way.
>    - I'm not sure that such path patterns will not match something that
>      we don't intend to match.
>    - I don't know, to be honest, whether it'll work at all.

Neither do I.

> 
> So I would keep the proposed solution, because it still looks as the
> simplest one.

Yes, your one is quite robust and covers everything. BTW, it worth to
mention the fact, in-source build is covered via .luacheckrc (in case of
LuaJIT sources and its autogenerated files).

> 
> Igor, please, agree explicitly or challenge my point.

I explicitly agree and you still have my LGTM (but I see no reaction
about the nit below). Feel free to proceed with the patch.

> 
> BTW, while looking into the problem with Igor, we found that luajit's
> luacheck rule fails on source / build paths with symlinks components.
> The solution is to use real paths everywhere.

I take this upon myself.

> 
> On Fri, Feb 26, 2021 at 09:11:58PM +0300, Igor Munkin wrote:
> > 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

-- 
Best regards,
IM

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

[Alexander Turenko via Tarantool-patches]

Aside of the changes below I rebased my patch on top of the latest
master and verified that it applies cleanly on 2.7 and 2.6 as well.

Force-pushed Totktonada/fix-luacheck-invocation to
40fc60b070c1a4474dd9173b1e72a8c425bd28d9.

> > So I would keep the proposed solution, because it still looks as the
> > simplest one.
> 
> Yes, your one is quite robust and covers everything. BTW, it worth to
> mention the fact, in-source build is covered via .luacheckrc (in case of
> LuaJIT sources and its autogenerated files).

I added the brief explanation. Thank you for participating in the
problem investigation!

 |  #   outside of ${dir} or when ${dir} is not a real path.
 |  # * Using of a path with symlink components in --exclude-files.
 |  #
 | +# We should exclude the build directory (when it is inside the
 | +# source tree), at least because it contains LuaJIT's generated
 | +# files and the temporary directory for testing (test/var).
 | +#
 | +# .luacheckrc provides corresponding exclusion rules for the
 | +# in-source build case (`cmake .`).
 | +#
 |  # [1]: https://github.com/mpeterv/luacheck/issues/208
 |  #
 |  set(EXCLUDE_FILES)

> > Igor, please, agree explicitly or challenge my point.
> 
> I explicitly agree and you still have my LGTM (but I see no reaction
> about the nit below). Feel free to proceed with the patch.

Sorry. I answered you in my mind, but forgot to write my answer :)
Responded below.

> > BTW, while looking into the problem with Igor, we found that luajit's
> > luacheck rule fails on source / build paths with symlinks components.
> > The solution is to use real paths everywhere.
> 
> I take this upon myself.

Thank you!

I should mention that there is the problem with the LuaJIT CMake rule in
the commit message.

 | -Now `make luacheck` gracefully handles different cases: in-source
 | +Now `make luacheck` gracefully handles different cases[^1]: 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 have the similar problems with LuaJIT's luacheck rule. They
 | +      will be fixed in a separate patch.
 |
 |  [1]: https://github.com/mpeterv/luacheck/issues/208
 |
 | +Reviewed-by: Sergey Bronnikov <sergeyb@tarantool.org>
 | +Reviewed-by: Igor Munkin <imun@tarantool.org>

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

Clarified in the following comment. Thanks for pointing it out!

 |  function(file_is_in_directory varname file dir)
 |      file(RELATIVE_PATH file_relative "${dir}" "${file}")
 | +
 | +    # Tricky point: one may find <STREQUAL ".."> and
 | +    # <MATCHES "^\\.\\./"> if-branches quite similar and coalesce
 | +    # them as <MATCHES "^\\.\\.">. However it'll match paths like
 | +    # "..." or "..foo/bar", whose are definitely descendant to
 | +    # the directory.
 |      if (file_relative STREQUAL "")
 |          # <file> and <dir> is the same directory.
 |          set(${varname} FALSE PARENT_SCOPE)

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

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

[Alexander Turenko via Tarantool-patches]

Pushed to master (2.8.0-105-gaf448464d), 2.7 (2.7.1-95-gf1754e788), 2.6
(2.6.2-92-gaae0c189a).

$ git push origin :Totktonada/fix-luacheck-invocation
To github.com:tarantool/tarantool.git
 - [deleted]             Totktonada/fix-luacheck-invocation

WBR, Alexander Turenko.

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

[Igor Munkin via Tarantool-patches]

Sasha,

On 05.03.21, Alexander Turenko wrote:

<snipped>

> 
> > > BTW, while looking into the problem with Igor, we found that luajit's
> > > luacheck rule fails on source / build paths with symlinks components.
> > > The solution is to use real paths everywhere.
> > 
> > I take this upon myself.
> 
> Thank you!

Here is the patch[1].

> 
> I should mention that there is the problem with the LuaJIT CMake rule in
> the commit message.
> 
>  | -Now `make luacheck` gracefully handles different cases: in-source
>  | +Now `make luacheck` gracefully handles different cases[^1]: 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 have the similar problems with LuaJIT's luacheck rule. They
>  | +      will be fixed in a separate patch.
>  |
>  |  [1]: https://github.com/mpeterv/luacheck/issues/208
>  |
>  | +Reviewed-by: Sergey Bronnikov <sergeyb@tarantool.org>
>  | +Reviewed-by: Igor Munkin <imun@tarantool.org>
> 

<snipped>

[1]: https://lists.tarantool.org/tarantool-patches/de1139d94e7501628e69e56d7837fc76cf6b5120.1614986102.git.imun@tarantool.org/T/#u

-- 
Best regards,
IM