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

[Alexander Turenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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