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

Thu Oct 31 18:09:27 MSK 2019



>Четверг, 31 октября 2019, 16:10 +03:00 от Alexander Turenko <alexander.turenko at 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.
Created instructions on Confluence.
>
>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. 
Right, some latest patches broke the stability.
>
>
>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).
Added to instructions on Confluence not to miss the check again.
Discussed and decided to block backtrace to avoid of libunwind use. 
>
>See other comments below.
>
>WBR, Alexander Turenko.
>
>> build: added centos 8
>
>Use imperative mood. 
Fixed.
>
>
>>
>> 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
Changed.
>
>> 
>> 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-'.
Used the suggested sentence with additions.
>
>> 
>> 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 
Mistake corrected.
>
>
>> 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.
Removed extra information.
>
>> 
>> 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
Corrected.
>
>> 
>> Closes #4543
>> ---
>> 
>> Github:  https://github.com/tarantool/tarantool/tree/avtikhon/gh-4543-centos8
>
>It seems you didn't updated your branch. 
Right, forgot to push.
>
>
>> 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. 
Got the changes with re-base.
>
>
>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 
Ok.
>
>
>>  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. 
Removed python2-greenlet due to python2-gevent installs it in dependences.
>
>
>> +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. 
Corrected.
>
>
>> +    self.skip = 1
>> +
>> +# vim: set ft=python:
>> -- 
>> 2.17.1
>> 
>


-- 
Alexander Tikhonov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20191031/088dd1b7/attachment.html>