Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v2] tools: fix luacheck invocation in different cases
@ 2021-02-18 16:09 Alexander Turenko via Tarantool-patches
  2021-02-25 11:59 ` Sergey Bronnikov via Tarantool-patches
  2021-02-26 18:11 ` Igor Munkin via Tarantool-patches
  0 siblings, 2 replies; 5+ messages in thread
From: Alexander Turenko via Tarantool-patches @ 2021-02-18 16:09 UTC (permalink / raw)
  To: Sergey Bronnikov; +Cc: tarantool-patches, Alexander Turenko

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Tarantool-patches] [PATCH v2] tools: fix luacheck invocation in different cases
  2021-02-18 16:09 [Tarantool-patches] [PATCH v2] tools: fix luacheck invocation in different cases Alexander Turenko via Tarantool-patches
@ 2021-02-25 11:59 ` Sergey Bronnikov via Tarantool-patches
  2021-02-25 16:35   ` Alexander Turenko via Tarantool-patches
  2021-02-26 18:11 ` Igor Munkin via Tarantool-patches
  1 sibling, 1 reply; 5+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2021-02-25 11:59 UTC (permalink / raw)
  To: Alexander Turenko; +Cc: 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()

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Tarantool-patches] [PATCH v2] tools: fix luacheck invocation in different cases
  2021-02-25 11:59 ` Sergey Bronnikov via Tarantool-patches
@ 2021-02-25 16:35   ` Alexander Turenko via Tarantool-patches
  2021-02-26  9:25     ` Sergey Bronnikov via Tarantool-patches
  0 siblings, 1 reply; 5+ messages in thread
From: Alexander Turenko via Tarantool-patches @ 2021-02-25 16:35 UTC (permalink / raw)
  To: Sergey Bronnikov; +Cc: 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Tarantool-patches] [PATCH v2] tools: fix luacheck invocation in different cases
  2021-02-25 16:35   ` Alexander Turenko via Tarantool-patches
@ 2021-02-26  9:25     ` Sergey Bronnikov via Tarantool-patches
  0 siblings, 0 replies; 5+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2021-02-26  9:25 UTC (permalink / raw)
  To: Alexander Turenko; +Cc: 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Tarantool-patches] [PATCH v2] tools: fix luacheck invocation in different cases
  2021-02-18 16:09 [Tarantool-patches] [PATCH v2] tools: fix luacheck invocation in different cases Alexander Turenko via Tarantool-patches
  2021-02-25 11:59 ` Sergey Bronnikov via Tarantool-patches
@ 2021-02-26 18:11 ` Igor Munkin via Tarantool-patches
  1 sibling, 0 replies; 5+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-02-26 18:11 UTC (permalink / raw)
  To: Alexander Turenko; +Cc: 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-02-26 18:12 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-18 16:09 [Tarantool-patches] [PATCH v2] tools: fix luacheck invocation in different cases Alexander Turenko via Tarantool-patches
2021-02-25 11:59 ` Sergey Bronnikov via Tarantool-patches
2021-02-25 16:35   ` Alexander Turenko via Tarantool-patches
2021-02-26  9:25     ` Sergey Bronnikov via Tarantool-patches
2021-02-26 18:11 ` Igor Munkin via Tarantool-patches

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git