From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (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 3466C4696C3 for ; Fri, 10 Apr 2020 17:53:18 +0300 (MSK) Date: Fri, 10 Apr 2020 17:53:16 +0300 From: Sergey Bronnikov Message-ID: <20200410145316.GA72590@pony.bronevichok.ru> References: <1586406055.255325095@f468.i.mail.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1586406055.255325095@f468.i.mail.ru> Subject: Re: [Tarantool-patches] [PATCH v2 6/6] gitlab-ci: enable static analysis with luacheck List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Tikhonov Cc: o.piskunov@tarantool.org, Sergey Bronnikov , tarantool-patches@dev.tarantool.org Hello, thanks for review. Please see my answers inline. On 07:20 Thu 09 Apr , Alexander Tikhonov wrote: > > Hi Sergey, thank you for the patch, please check my comments below. > >   > >Среда, 8 апреля 2020, 18:43 +03:00 от Sergey Bronnikov : > >  > >From: Sergey Bronnikov < sergeyb@tarantool.org > > > > >Closes #4681 > >--- > Please add in the review letter the link to the branch here, also the issue link. Will do next time. > > .gitlab-ci.yml | 9 +++++++++ > > .travis.mk | 12 +++++++++++- > > 2 files changed, 20 insertions(+), 1 deletion(-) > > > >diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > >index cd710027f..adbd892c8 100644 > >--- a/.gitlab-ci.yml > >+++ b/.gitlab-ci.yml > >@@ -1,4 +1,5 @@ > > stages: > >+ - static_analysis > Do we really need the separate stage for this job ? As we agreed with > A. Turenko before, we create stages only for jobs that needs additional > steps w/o next steps wont work and which can be used in the next stage > for more than single job. I think the answer is "yes". The arguments are: - it is a best practice in industry to split CI/CD pipeline for several stages. In canonical example they are: build, test, deploy. - build, static analysis, unit testing, module testing are successive steps in finding bugs. Each step requires different amount of efforts to debug in case of failure. No reasons to perform unit testing when compiler warns about non-initialized variables and no reasons to perform complex functional testing when unit tests failed. We have future plans to add clang-analyzer and more strict warnings in compilers and 'static_analysis' stage will be a good place for these tasks on pipeline. > Anyway separate stage will add depends for the rest of the jobs, which > can be blocked because of any issue with luacheck, like it can be > luacheck cloning either its depends installations: > >index f709a18b6..32222621b 100644 > >--- a/.travis.mk > >+++ b/.travis.mk > >@@ -77,8 +77,9 @@ deps_buster_clang_8: deps_debian > > # Release > >  > > build_debian: > >- cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS} > >+ cmake . -DENABLE_DIST=ON -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS} > This target is common wide along all the jobs and if ENABLE_DIST > define is not adding changes for them may be just need add the comment > the purpose on its adding. Well, I'll describe this in a commit message. > >  make -j > >+ sudo make install > This target is common wide along all the jobs and may be it better to > move the installation to the target ‘luacheck’ which only needs it. Well, I'll move 'make install' to a separate target. S.