From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 7313D30BE7 for ; Sat, 22 Jun 2019 20:00:57 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id eNsRgAsqkfiQ for ; Sat, 22 Jun 2019 20:00:57 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id BFF6430BE2 for ; Sat, 22 Jun 2019 20:00:56 -0400 (EDT) Date: Sun, 23 Jun 2019 03:00:34 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [Re: Re: [PATCH v2 1/2] Gitlab-ci updated after reiview - w/o perf] Message-ID: <20190623000034.nikvvi22ddibvtov@tkn_work_nb> References: <20190622235857.rvmop5w4hjz5kbqa@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190622235857.rvmop5w4hjz5kbqa@tkn_work_nb> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: "Alexander V. Tikhonov" Cc: tarantool-patches@freelists.org Hi! Please, send answers for reviews to my @tarantool.org e-mail. I passed through your answers and looked into the actual code. Some old comments are still actual, I commented them here. Also I leave several comments re things that are not more actual just to clarify my initial ideas (where it seems that we misunderstood each other). WBR, Alexander Turenko. > >> +variables: > >> + VERSION_SUFFIX: "master" > >> + IMAGE_TEST: "${CI_REGISTRY}/${CI_PROJECT_PATH}:debian-stretch_${VERSION_SUFFIX}" > >> + IMAGE_TEST_LTO: "${CI_REGISTRY}/${CI_PROJECT_PATH}:debian-buster_${VERSION_SUFFIX}" > > > >This suffix does not resolve any problems with changed dependencies. So, > >we should either remove it for now or really resolve the problem: say, > >use $(md5sum .travis.mk | awk '{ print $1 }') as a tag name. > > > Not possible to use scripts for variables, neither use API to remove images from registry. Not resolved. Now I fixed it myself. See [1]. [1]: https://www.freelists.org/post/tarantool-patches/PATCH-v4-02-Implement-Gitlabci-testing-process,1 > >> + FLAG_LTO: "-DENABLE_LTO=ON" > > > >I think -DENABLE_LTO-ON is more self-descriptive then this variable > >name. There is no need to extract it to a variable. > > > Fixed, set name to: 'DENABLE_LTO_ON' I mean that there is no need in this variable at all. Please, inline the variable value. > >> +debian10_lto: > >> + only: > >> + refs: > >> + - master > >> + - /.*/ > > > >As we discusssed voicely: /.*/ should not be here in a final patch. We > >however can provide an ability to run full testing on a branch different > >from master. Say, match /.*-full/. > > > >(The same is applicable for all such patterns below.) > > > Working on. Please, don't forget. It is not done for now as I see. > >> +freebsd_release: > >> + stage: test > >> + tags: > >> + - vms_freebsd > >> + variables: > >> + VMS_NAME: 'Freebsd' > > > >I would include a version number into the virtual machine name (and the > >job name too). > Fixed. VMS_NAME still does not contain an OS version. Is it hard to change? > >> +# build sources with different flags and test > > > >It is the title for the set of goals below? Looks as a comment for one > >goal. Also here I see that bootstrap, submodules update and runnign > >tests are doing, but not build sources. It seems the comment is not > >precise. > > > Right, the comment for a set of goals there below. So it should be separated from the next block of code with an empty like. Consider examples: | # comment for the next block | line 1 | line 2 | # comment for several blocks below | | line 1 | line 2 | | line 3 | line 4 Aside that more decorations often used to visually indent a file sections. I had rewrote docker_bootstrap goal and split .gitlab.mk to sections. See [1]. So this is now fixed. Please, be attend: you don't answer re 'the comment is not precise' part of my review comment. But this is anyway fixed already with my changes. > >Is there real need in this flag (some specific case when it is needed)? > >What will going on CentOS 6 if we'll run into this case? > > > Once I got the broken repository some how, '--force' flag fixed it, on CentOS 6 will need to fix it manually. It seems we now going to run submodules update outside of containers / virtual machines, but okay, let's leave both attempts to update the submodules: with --force and w/o it. > >> +deps_freebsd: > >> + # don't use bad py27-msgpack-python > > > >Can you link exact problem description here or in the commit message? > Removed, obvious message. It is not obvious for me why py27-msgpack-python is 'bad'. It seems we just don't need it, yep? > >> +runtest_freebsd: build_freebsd > >> + cd test && /usr/bin/python test-run.py --force $(NO_PARALLEL_TESTING) || true > > > >Volkswagen mode? > Currently FreeBSD testing is not ready and will fail pipeline, the issue in github exists, > but we really need the test result to check the new fails. So, please, use skipcond's to disable some tests on FreeBSD. This is very misleading to have a testing job that always report 'ok'.