From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 8FF6C445320 for ; Tue, 21 Jul 2020 15:48:20 +0300 (MSK) Date: Tue, 21 Jul 2020 15:38:01 +0300 From: Igor Munkin Message-ID: <20200721123801.GJ18920@tarantool.org> References: <20200720194653.GI18920@tarantool.org> <20200721115236.GB88846@pony.bronevichok.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200721115236.GB88846@pony.bronevichok.ru> Subject: Re: [Tarantool-patches] [PATCH 18/19] Add luacheck supression for luajit test List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org, alexander.turenko@tarantool.org Sergey, On 21.07.20, Sergey Bronnikov wrote: > Igor, thanks for review! > > > By the way, I see another problem: > > - Wait, it's all just luacheck failing on luajit tests? > > | $ git co HEAD^ > > | $ git log --oneline -1 | cat > > | c9653cbec Add luacheck supression for luajit test > > | $ make luacheck || echo "Always has been" > > | > > | Total: 95 warnings / 0 errors in 492 files > > | make[3]: *** [CMakeFiles/luacheck.dir/build.make:58: luacheck] Error 1 > > | make[2]: *** [CMakeFiles/Makefile2:430: CMakeFiles/luacheck.dir/all] Error 2 > > | make[1]: *** [CMakeFiles/Makefile2:437: CMakeFiles/luacheck.dir/rule] Error 2 > > | make: *** [Makefile:316: luacheck] Error 2 > > | Always has been > > > > I guess the agreement to make the series passing after > > *every* patch still stands (like you made for src/ and extra/ > > directories). I doubt we should violate this rule for test/. > > Actually changes in a patch series are atomic and problem described > above is not related to atomicity of changes. It definitely does relate to the subj. > Paths in a luacheck config are interpreted relatively to the directory > from which config was loaded, see [1]. When you run 'make luacheck' in > separate build directory it uses absolute path to .luacheckrc and > therefore absolute paths to .lua files: One more time: I checked out the commit prior to the branch HEAD (i.e. c9653cbec), configured Tarantool in-source build and ran available only in project root directory (otherwise the corresponding error is reported by make util). The root cause is not about the paths for out of source build (I didn't even test this build type) but the exclude list, that doesn't contain any line related to luajit test directory (i.e. test/luajit-tap). As a result luacheck fails until the submodule is bumped, in other words until the last commit. We already discussed this in the previous series[1], but you ignored it. Again. Well, it's not a problem for me to point out your attention to this. Again. This is about the 10th overall series for luacheck, isn't it? How more iterations do we need for it? > $ make luacheck VERBOSE=1 > ... > Perform static analysis of Lua code > ../.rocks/bin/luacheck --codes --config /home/s.bronnikov/tarantool/.luacheckrc /home/s.bronnikov/tarantool > Checking /home/s.bronnikov/tarantool/build/extra/luarocks/hardcoded.lua > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > ... > $ > > in such case exclude and include paths specified in config become > broken, luacheck simply ignores it and checks only .lua files in a build/ > directory. > > I made a small change on top of branch to fix case when we run 'make > luacheck' in a build dir: > > --- a/CMakeLists.txt > +++ b/CMakeLists.txt > @@ -158,7 +158,8 @@ add_custom_target(ctags DEPENDS tags) > > 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 . > + WORKING_DIRECTORY "${PROJECT_SOURCE_DIR}" > COMMENT "Perform static analysis of Lua code" > ) > > CI passed with a patch, see [2]. > > 1. https://luacheck.readthedocs.io/en/stable/config.html > 2. https://gitlab.com/tarantool/tarantool/-/pipelines/168992688 Spoiler alert: here is the missing line in .luacheckrc, that you've removed in scope of post-review fixes for this patch[1]. ================================================================================ diff --git a/.luacheckrc b/.luacheckrc index 21680c08c..76807b716 100644 --- a/.luacheckrc +++ b/.luacheckrc @@ -50,6 +50,7 @@ exclude_files = { "test/vinyl/*.test.lua", "test/wal_off/*.test.lua", "test/xlog/*.test.lua", + "test/luajit-tap/*.test.lua", "third_party/**/*.lua", ".rocks/**/*.lua", ".git/**/*.lua", ================================================================================ With this line everything works fine. However I proposed to bump luajit submodule prior to unmasking test/ directory, that looks more natural and convenient to me. Does it look good to you? > > > -- > sergeyb@ [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/016332.html -- Best regards, IM