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

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

[Alexander Turenko]

On Thu, Oct 03, 2019 at 02:49:36PM +0300, Alexander V. Tikhonov wrote:
> Implemented ability to build CentOS 8 image ready for
> Packpack use. Added CentOS 8 into regular testing under gitlab-ci.

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.

> 
> Closes #4543
> ---
> 
> Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4543-centos8
> Issue: https://github.com/tarantool/tarantool/issues/4534
> 
>  .gitlab-ci.yml     | 6 ++++++

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

>  rpm/tarantool.spec | 9 ++++++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> 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..5d9277576 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

It will work only for 2.2+, see the comment re PowerTools in
https://github.com/packpack/packpack-docker-images/pull/39

>  BuildRequires: openssl-devel
>  BuildRequires: libicu-devel
>  #BuildRequires: msgpuck-devel
> @@ -64,12 +66,17 @@ 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%{?fedora} >= 22 || 0%{?rhel} >= 8)

It seems it should be done only on rhel >= 8, not on Fedora.

> +BuildRequires: python2 >= 2.7
> +BuildRequires: python2-six >= 1.9.0
> +BuildRequires: python2-yaml >= 3.0.9

We should provide python2-gevent anyway: let's add to to backports
repository in https://packagecloud.io/packpack/backports

> +%endif
>  
>  Name: tarantool
>  # ${major}.${major}.${minor}.${patch}, e.g. 1.6.8.175
> -- 
> 2.17.1
> 

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

[Alexander V. Tikhonov]

Added CentOS 8 image build, ready for Packpack use.

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.

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

Added CentOS 8 into regular testing at gitlab-ci.

Closes #4543
---

Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4543-centos8
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
 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
+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':
+    self.skip = 1
+
+# vim: set ft=python:
-- 
2.17.1

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

[Alexander Turenko]

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
>