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

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

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