Re: [Tarantool-patches] [PATCH v1] build: added centos 8

[Alexander Turenko <alexander.turenko@tarantool.org>]

Alexander, your mistakes are often the same as ones in other patches and
often the same that was already seen on previous review iterations: I
think you need some kind of check-list to perform it before sending a
patch.

Please, organize your work in the way that will allow you to don't miss
review comments.

I run build and tests and tests segfaults very often. I didn't
investigate the crashes, but Vlad says they can be related to
https://github.com/tarantool/tarantool/issues/4597

I'll wait for a fix, because I'm unable to test your patch when master
crashes randomly.

I tried to install the resulting package inside ususal centos:8 docker
image and got the following error:

 | # yum install /pwd/build/tarantool-2.3.0.191-1.el8.x86_64.rpm
 | Error:
 |  Problem: conflicting requests
 |   - nothing provides libunwind-x86_64.so.8()(64bit) needed by tarantool-2.3.0.191-1.el8.x86_64
 |   - nothing provides libunwind.so.8()(64bit) needed by tarantool-2.3.0.191-1.el8.x86_64
 | (try to add '--skip-broken' to skip uninstallable packages or '--nobest' to use not only best candidate packages)

It seems libunwind was moved from base repository to epel. I think we
should not depend on libunwind (and disable related features for now),
but maybe build it statically and bundle into tarantool in the future
(if possible).

See other comments below.

WBR, Alexander Turenko.

> build: added centos 8

Use imperative mood.

>
> Added CentOS 8 image build, ready for Packpack use.

Let me cite my previous review comment:

 | I don't got the first sentence. Packpack provides build images and a
 | tool to use them. It is not something that a user need to use
 | tarantool.

https://lists.tarantool.org/pipermail/tarantool-patches/2019-October/012046.html

> 
> Found that CentOS 8 completely moved to the use of python3
> while Tarantool testing still uses python2, also found that
> CentOS 8 changed naming of the python packages to python2.
> To make these changes workable with Tarantool build added
> use of the python2 packages instead of python.

This can be said simpler: CentOS 8 uses 'python2-' prefix for Python 2
modules rather the 'python-'.

> 
> Found that old packages like python-gevent and python-greenlet
> were completely removed from CentOS 8. To fix it the sources
> if these packages were downloaded from https://cbs.centos.org/
> and rebuilt, to make them available the binaries were saved
> at the PackageCloud packpack backport repository.
> 
> Met the issue with app-tap/pwd.test.lua which was allready

allready -> already

> fixed in local commit 7732daded14f86a314d44486e306e17bbc6ab293
> (lua: treat ENOENT as success in getpwall/getgrall), but it
> wasn't merged into trunk - decided to block temporary the test
> till the correct fix will be created with the new issue created:
> https://github.com/tarantool/tarantool/issues/4592

I don't sure this fix was right, so I would not say that 'it was already
fixed'. I think than mentioning of an issue that track the problem is
enough here.

> 
> Added CentOS 8 into regular testing at gitlab-ci.

Let me cite the comment from my previous review:

 | We should add it to Travis-CI too, because deployments are made only
 | from Travis-CI.

https://lists.tarantool.org/pipermail/tarantool-patches/2019-October/012046.html

> 
> Closes #4543
> ---
> 
> Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4543-centos8

It seems you didn't updated your branch.

> Issue: https://github.com/tarantool/tarantool/issues/4543
> 
>  .gitlab-ci.yml            |  6 ++++++
>  rpm/tarantool.spec        | 11 ++++++++++-
>  test/app-tap/pwd.skipcond |  7 +++++++
>  3 files changed, 23 insertions(+), 1 deletion(-)
>  create mode 100644 test/app-tap/pwd.skipcond
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 431730b67..655e68cdc 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -152,6 +152,12 @@ centos_7:
>      OS: 'el'
>      DIST: '7'
>  
> +centos_8:
> +  <<: *deploy_test_definition
> +  variables:
> +    OS: 'el'
> +    DIST: '8'
> +
>  fedora_28:
>    <<: *deploy_test_definition
>    variables:
> diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
> index 10daf1458..5930f79d7 100644
> --- a/rpm/tarantool.spec
> +++ b/rpm/tarantool.spec
> @@ -15,7 +15,9 @@ BuildRequires: gcc-c++ >= 4.5
>  BuildRequires: coreutils
>  BuildRequires: sed
>  BuildRequires: readline-devel
> +%if (0%{?fedora} > 0 || 0%{?rhel} <= 7)
>  BuildRequires: libyaml-devel
> +%endif

Please, rebase: we removed this dependency on master and 2.2.

We anyway need libyaml-devel on 1.10 and 2.1, but there is no need to
exclude the dependency on those branches. I already said that:
https://github.com/packpack/packpack-docker-images/pull/39#discussion_r338324713

>  BuildRequires: openssl-devel
>  BuildRequires: libicu-devel
>  #BuildRequires: msgpuck-devel
> @@ -64,12 +66,19 @@ BuildRequires: libunwind-devel
>  %endif
>  
>  # For tests
> -%if (0%{?fedora} >= 22 || 0%{?rhel} >= 7)
> +%if (0%{?fedora} >= 22 || 0%{?rhel} == 7)
>  BuildRequires: python >= 2.7
>  BuildRequires: python-six >= 1.9.0
>  BuildRequires: python-gevent >= 1.0
>  BuildRequires: python-yaml >= 3.0.9
>  %endif
> +%if (0%{?rhel} >= 8)
> +BuildRequires: python2 >= 2.7
> +BuildRequires: python2-six >= 1.9.0
> +BuildRequires: python2-greenlet

Is not this the dependency of python2-gevent? I think we should not add
it explicitly here if it is so.

> +BuildRequires: python2-gevent >= 1.0
> +BuildRequires: python2-yaml >= 3.0.9
> +%endif
>  
>  Name: tarantool
>  # ${major}.${major}.${minor}.${patch}, e.g. 1.6.8.175
> diff --git a/test/app-tap/pwd.skipcond b/test/app-tap/pwd.skipcond
> new file mode 100644
> index 000000000..b001fb82c
> --- /dev/null
> +++ b/test/app-tap/pwd.skipcond
> @@ -0,0 +1,7 @@
> +import subprocess
> +
> +# Disabled at CentOS 8 build due to issue #4592.
> +if subprocess.check_output(["rpm --eval '%{centos_ver}'"], shell=True).strip() == '8':

PEP8 forbids so long lines.

> +    self.skip = 1
> +
> +# vim: set ft=python:
> -- 
> 2.17.1
>