[tarantool-patches] Re: [Re: Re: [PATCH v2 1/2] Gitlab-ci updated after reiview - w/o perf]

[tarantool-patches] Re: [Re: Re: [PATCH v2 1/2] Gitlab-ci updated after reiview - w/o perf]

[Alexander Turenko]

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'.