<HTML><BODY><div>Hi Sergey, please check my comments below.<br><br> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Пятница, 10 апреля 2020, 17:53 +03:00 от Sergey Bronnikov <sergeyb@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><style type="text/css"></style><div><div id="style_15865303970808476146_BODY">Hello,<br><br>thanks for review. Please see my answers inline.<br><br>On 07:20 Thu 09 Apr , Alexander Tikhonov wrote:<br>><br>> Hi Sergey, thank you for the patch, please check my comments below.<br>><br>> <br>> >Среда, 8 апреля 2020, 18:43 +03:00 от Sergey Bronnikov <<a href="/compose?To=estetus@gmail.com">estetus@gmail.com</a>>:<br>> > <br>> >From: Sergey Bronnikov < <a href="/compose?To=sergeyb@tarantool.org">sergeyb@tarantool.org</a> ><br>> ><br>> >Closes #4681<br>> >---<br>> Please add in the review letter the link to the branch here, also the issue link.<br><br>Will do next time.<br><br>> > .gitlab-ci.yml | 9 +++++++++<br>> > .travis.mk | 12 +++++++++++-<br>> > 2 files changed, 20 insertions(+), 1 deletion(-)<br>> ><br>> >diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml<br>> >index cd710027f..adbd892c8 100644<br>> >--- a/.gitlab-ci.yml<br>> >+++ b/.gitlab-ci.yml<br>> >@@ -1,4 +1,5 @@<br>> > stages:<br>> >+ - static_analysis<br>> Do we really need the separate stage for this job ? As we agreed with<br>> A. Turenko before, we create stages only for jobs that needs additional<br>> steps w/o next steps wont work and which can be used in the next stage<br>> for more than single job.<br><br>I think the answer is "yes". The arguments are:<br>- it is a best practice in industry to split CI/CD pipeline for several<br>stages. In canonical example they are: build, test, deploy.</div></div></div></div></blockquote></div><div>Actually right that canonical way is good, but do we really need it. Is<br>it actually better than have the faster way to find the issues? I mean<br>that out gitlab-ci is using for now not for the automate production, but<br>for the fast developers testing — it means that they use it not to check</div><div>the final code, but to develop it and run it a lot of times during it. I can</div><div>absolutely agree that we need CI/CD process and we can start create</div><div>it here, but it should be the other separate pipeline from branches</div><div>testings. So my suggestion is to left the branch testing flat to be fast,<br>and add your change just for releases branches testings. By the way<br>gitlab-ci is too weak for CI/CD processes — much better to use Jenkins</div><div>either other CI/CD tools which can use pipelines in parallel instead of</div><div>gitlab-ci which can’t do it.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>- build, static analysis, unit testing, module testing are successive<br>steps in finding bugs. Each step requires different amount of efforts to<br>debug in case of failure. No reasons to perform unit testing when compiler<br>warns about non-initialized variables and no reasons to perform complex<br>functional testing when unit tests failed.<br><br>We have future plans to add clang-analyzer and more strict warnings in<br>compilers and 'static_analysis' stage will be a good place for these<br>tasks on pipeline.<br><br>> Anyway separate stage will add depends for the rest of the jobs, which<br>> can be blocked because of any issue with luacheck, like it can be<br>> luacheck cloning either its depends installations:<br><br><snipped><br><br>> >index f709a18b6..32222621b 100644<br>> >--- a/.travis.mk<br>> >+++ b/.travis.mk<br>> >@@ -77,8 +77,9 @@ deps_buster_clang_8: deps_debian<br>> > # Release<br>> > <br>> > build_debian:<br>> >- cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS}<br>> >+ cmake . -DENABLE_DIST=ON -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS}<br>> This target is common wide along all the jobs and if ENABLE_DIST<br>> define is not adding changes for them may be just need add the comment<br>> the purpose on its adding.<br><br>Well, I'll describe this in a commit message.<br><br>> > make -j<br>> >+ sudo make install<br>> This target is common wide along all the jobs and may be it better to<br>> move the installation to the target ‘luacheck’ which only needs it.<br><br>Well, I'll move 'make install' to a separate target.<br><br><snipped><br><br>S.</div></div></div></div></blockquote> <div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Alexander Tikhonov</div></div></div><div> </div></div></BODY></HTML>