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

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

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