From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 444607030C; Fri, 5 Mar 2021 01:21:14 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 444607030C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1614896474; bh=AlpSQR5XVFdDnhCB25kNJ1aDs17jiT14lJmF31kkjHA=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=ZSRXIzuaHdcQ1iBhLY/0UJySqjHTAxMy4aaK7rxbWY3KzluYDIHGZYX3UmzBO+tc+ kX6XNIGPd4jv+y2KOO4dxhvk3cYj/jVyWustvLfkNGoPYnzsqewGccO5yeiLEIzaf4 Js/U+3Ikg72DWgCaz3ykya92/KYa98ljXVKWtCBU= Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 1915D7030C for ; Fri, 5 Mar 2021 01:21:13 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1915D7030C Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1lHwLI-0004jy-0s; Fri, 05 Mar 2021 01:21:12 +0300 Date: Fri, 5 Mar 2021 01:21:06 +0300 To: Alexander Turenko Message-ID: <20210304222106.GM9042@tarantool.org> References: <524c0ce8acc18111ab4c8b36e383ff192779c780.1613661908.git.alexander.turenko@tarantool.org> <20210226181158.GD9042@tarantool.org> <20210303180257.ktityub26cd27ogh@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210303180257.ktityub26cd27ogh@tkn_work_nb> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.10.1 (2018-07-13) X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92A98208ECBDD29F5B42DD879439ED63E04973C1061E33475182A05F53808504005C66ACE54EA94E8C8EE3AF45464B59B79784B4934AECDB2E9E15457E9C87E31 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7D4A169723F56FEDEEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637C2EE9128AC0EB2C78638F802B75D45FF5571747095F342E8C7A0BC55FA0FE5FC7E36B7026578E6D928BC21D8DEC25ABEEF1E5D99A43D2801389733CBF5DBD5E913377AFFFEAFD269A417C69337E82CC2CC7F00164DA146DAFE8445B8C89999729449624AB7ADAF37F6B57BC7E64490611E7FA7ABCAF51C92176DF2183F8FC7C07E7E81EEA8A9722B8941B15DA834481F9449624AB7ADAF3735872C767BF85DA29E625A9149C048EE0A3850AC1BE2E7352629B07FD02F83A64AD6D5ED66289B524E70A05D1297E1BB35872C767BF85DA227C277FBC8AE2E8BA56E11165BA017C7EFF80C71ABB335746BA297DBC24807EA27F269C8F02392CD20465B3A5AADEC6827F269C8F02392CD5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5E999A09ADBE0BD80132EB8042A9BB9EFBD8ABE5719ACF30AD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75448CF9D3A7B2C848410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D347324AA9FA07FF01EC18E5FA9B036D7E14E7C7257F6BC10D488FA89570A2C2BA631F4ABA86E468B961D7E09C32AA3244C8691DFB8E652D8AE663E2081F08FA876B018FE5BB746DCD1927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojSsmoOoMLSh01/UjmsJEoZw== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822C92A57E7BF9912C67C7293CFBFF077E9A7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2] tools: fix luacheck invocation in different cases X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Igor Munkin via Tarantool-patches Reply-To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "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 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. do not intersect with ("true" OOS build) > > 2. is (in source build) > > 3. is a subdirectory within ("quasi" OOS build) > > > > The first case is very simple: you need only run luacheck within > > , 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 . > > > > 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 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 > > 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 , 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(-) > > > > > > > > > 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 "") > > > + # and is the same directory. > > > + set(${varname} FALSE PARENT_SCOPE) > > > + elseif (file_relative STREQUAL "..") > > > + # inside a (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 "^\\.\\./") > > > + # somewhere outside of the . > > > + set(${varname} FALSE PARENT_SCOPE) > > > + else() > > > + # is descendant to . > > > + set(${varname} TRUE PARENT_SCOPE) > > > + endif() > > > +endfunction() > > > -- > > > 2.30.0 > > > > > > > -- > > Best regards, > > IM -- Best regards, IM