From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id A46DF452566 for ; Fri, 8 Nov 2019 18:22:09 +0300 (MSK) Date: Fri, 8 Nov 2019 18:22:05 +0300 From: Alexander Turenko Message-ID: <20191108152201.rujz3l66llp4jatn@tkn_work_nb> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v2] build: enable CentOS 8 packaging List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Alexander V. Tikhonov" Cc: tarantool-patches@freelists.org, tarantool-patches@dev.tarantool.org Please, look into the changes I proposed below. They are about style, nothing intruisive. I also pushed them to avtikhon/gh-4543-centos8-more-full-ci branch to look into CI and ease reading of the resulting changeset. CI is green: https://gitlab.com/tarantool/tarantool/pipelines/94633088 https://travis-ci.org/tarantool/tarantool/builds/609234538 If you're okay, I'll push the patch. WBR, Alexander Turenko. > build: enable CentOS 8 packaging > > Enabled packages build rules for CentOS 8. Aren't this duplicate of the header? (See the proposal below.) > For now CentOS 8 uses 'python2-' prefix for Python 2 modules > rather the 'python-', changed CentOS 8 packages installation > rules to use 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 test from the commit > 7732daded14f86a314d44486e306e17bbc6ab293 (lua: treat ENOENT as > success in getpwall/getgrall), temporary blocked the test (till > the fix will be pushed) with the new issue: > https://github.com/tarantool/tarantool/issues/4592 $ git show 7732daded14f86a314d44486e306e17bbc6ab293 fatal: bad object 7732daded14f86a314d44486e306e17bbc6ab293 It is part of a branch that was already deleted. Please, link only those commits that are part of of long-term branches. I would link only the issue here. (See the proposal below.) > > Found that libunwind library couldn't be installed from dependecies > which was needed by backtrace feature. Decided to block it temporary > till it will be enabled by issue: > https://github.com/tarantool/tarantool/issues/4611 I would make it clear that it is available via EPEL, but does not via the base repositories. (See the proposal below.) > Added CentOS 8 into regular testing at gitlab-ci and travis-ci. > > Closes #4543 > --- My variant of the commit message is below. Please, let me know whether you are okay with it. I left all points you highlighted and tried to make its description more easy to understand w/o much context. | travis-ci/gitlab-ci: add CentOS 8 | | Added build + test jobs in GitLab-CI and build + test + deploy jobs on | Travis-CI for CentOS 8. | | Updated testing dependencies in the RPM spec to follow the new Python 2 | package naming scheme that was introduced in CentOS 8: it uses | 'python2-' prefix rather then 'python-'. | | CentOS 8 does not provide python2-gevent and python2-greenlet packages, | so they were pushed to https://packagecloud.io/packpack/backports | repository. This repository is enabled in our build image | (packpack/packpack:el-8) by default. Those dependencies are build-time, | so nothing was changed for a user. The source RPM packages were gathered | from https://cbs.centos.org. | | Disabled app-tap/pwd.test.lua on CentOS 8 due to systemd-nss issue, | which was not worked around properly. Filed #4592 to resolved it in the | future. | | Eliminated libunwind runtime dependency (and libunwind-devel build | dependency) on CentOS 8, because the base system does not provide it. | fiber.info() backtraces and printing of a backtrace after a crash will | not be available on this system. Hopefully we'll fix it in the future, | filed #4611 on this. | | Closes #4543 Also added a docbot comment to the issue. > > Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4543-centos8-full-ci > Issue: https://github.com/tarantool/tarantool/issues/4543 > > .gitlab-ci.yml | 6 ++++++ > .travis.yml | 3 +++ > rpm/tarantool.spec | 13 ++++++++++++- > test/app-tap/pwd.skipcond | 11 +++++++++++ > 4 files changed, 32 insertions(+), 1 deletion(-) > create mode 100644 test/app-tap/pwd.skipcond > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > index cd5f2806f..cf13c382e 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/.travis.yml b/.travis.yml > index db7d80bd2..d48ef39e0 100644 > --- a/.travis.yml > +++ b/.travis.yml > @@ -43,6 +43,9 @@ jobs: > - name: "CentOS 7 build + test + deploy RPM" > env: OS=el DIST=7 > if: branch = "master" > + - name: "CentOS 8 build + test + deploy RPM" > + env: OS=el DIST=8 > + if: branch = "master" > - name: "Fedora 28 build + test + deploy RPM" > env: OS=fedora DIST=28 > if: branch = "master" > diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec > index b837214a6..416b4ebb4 100644 > --- a/rpm/tarantool.spec > +++ b/rpm/tarantool.spec > @@ -63,12 +63,18 @@ 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) I removed parentheses around the condition. > +BuildRequires: python2 >= 2.7 > +BuildRequires: python2-six >= 1.9.0 > +BuildRequires: python2-gevent >= 1.0 > +BuildRequires: python2-yaml >= 3.0.9 > +%endif > > Name: tarantool > # ${major}.${major}.${minor}.${patch}, e.g. 1.6.8.175 > @@ -125,14 +131,19 @@ C and Lua/C modules. > > %build > # RHBZ #1301720: SYSCONFDIR an LOCALSTATEDIR must be specified explicitly > +# gh-4611: temporary blocked the backtrace on CentOS 8 till completely enabled > %cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo \ > -DCMAKE_INSTALL_LOCALSTATEDIR:PATH=%{_localstatedir} \ > -DCMAKE_INSTALL_SYSCONFDIR:PATH=%{_sysconfdir} \ > +%if (0%{?rhel} >= 8) > + -DENABLE_BACKTRACE:BOOL=OFF \ > +%else > %if %{with backtrace} > -DENABLE_BACKTRACE:BOOL=ON \ > %else > -DENABLE_BACKTRACE:BOOL=OFF \ > %endif > +%endif Look at the code above: | %bcond_without backtrace # enabled by default | | %if %{with backtrace} | BuildRequires: libunwind-devel It would be better to set %{with backtrace} to appropriate value here and avoid the unnecessary build dependency. I changed it in the following way: | diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec | index 416b4ebb4..41dbaf726 100644 | --- a/rpm/tarantool.spec | +++ b/rpm/tarantool.spec | @@ -44,7 +44,14 @@ Requires(preun): chkconfig | Requires(preun): initscripts | %endif | | -%bcond_without backtrace # enabled by default | +%if 0%{?rhel} >= 8 | +# gh-4611: Disable backtraces on CentOS 8 by default due to lack | +# of libunwind package in the base system. | +%bcond_with backtrace | +%else | +# Enable backtraces by default. | +%bcond_without backtrace | +%endif | | %if %{with backtrace} | BuildRequires: libunwind-devel | @@ -131,19 +138,14 @@ C and Lua/C modules. | | %build | # RHBZ #1301720: SYSCONFDIR an LOCALSTATEDIR must be specified explicitly | -# gh-4611: temporary blocked the backtrace on CentOS 8 till completely enabled | %cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo \ | -DCMAKE_INSTALL_LOCALSTATEDIR:PATH=%{_localstatedir} \ | -DCMAKE_INSTALL_SYSCONFDIR:PATH=%{_sysconfdir} \ | -%if (0%{?rhel} >= 8) | - -DENABLE_BACKTRACE:BOOL=OFF \ | -%else | %if %{with backtrace} | -DENABLE_BACKTRACE:BOOL=ON \ | %else | -DENABLE_BACKTRACE:BOOL=OFF \ | %endif | -%endif | %if %{with systemd} | -DWITH_SYSTEMD:BOOL=ON \ | -DSYSTEMD_UNIT_DIR:PATH=%{_unitdir} \ BTW, I was wonder how it works w/o libunwind runtime dependency. The answer is here if you are interesting: https://rpm.org/user_doc/more_dependencies.html#automatic-dependencies After this change I found that packpack passes `--with backtrace` into rpmbuild and fixed it in https://github.com/packpack/packpack/pull/114 After this I found another problem and fixed it in a separate commit: | commit 41359c8b6e4e1dac20fbd211ea64f2fc3b9abf16 | Author: Alexander Turenko | Date: Fri Nov 8 14:12:24 2019 +0300 | | build: don't pass LDFLAGS from environment to curl | | After ea5929db4d400f5013a1c521870b9e14a8edcf85 ('build: fix OpenSSL | linking problems on FreeBSD') we set CFLAGS explicitly (possibly to an | empty value) when invoking a configure script for curl. When this | parameter is set the script does not use a value of environment variable | CFLAGS. | | Before this commit LDFLAGS environment variable can affect build of curl | submodule. This can lead to a problem when a user or a tool set CFLAGS | and LDFLAGS both and some linker flag assumes that some compilation flag | is present. Here we set empty LDFLAGS explicitly to avoid using of the | environment variable. | | A distributive build tool such as rpmbuild or emerge usually sets CFLAGS | and LDFLAGS. The problem with incompatible compiler / linker options has | been reveal under rpmbuild on CentOS 8 with hardened build enabled | (which is so when backtraces are disabled). | | It is not clear whether we should follow environment variables or values | determined by CMake for CFLAGS, CPPFLAGS and LDFLAGS when building a | submodule (such as luajit and curl). Let's decide about this later. | | Part of #4543. | | diff --git a/cmake/BuildLibCURL.cmake b/cmake/BuildLibCURL.cmake | index 12062ec8b..6d40ab045 100644 | --- a/cmake/BuildLibCURL.cmake | +++ b/cmake/BuildLibCURL.cmake | @@ -57,9 +57,20 @@ macro(curl_build) | | # Pass -isysroot= option on Mac OS, see | # above. | + # Note: Passing of CPPFLAGS / CFLAGS explicitly | + # discards using of corresponsing environment | + # variables. | CPPFLAGS=${LIBCURL_CPPFLAGS} | CFLAGS=${LIBCURL_CFLAGS} | | + # Pass empty LDFLAGS to discard using of | + # corresponding environment variable. | + # It is possible that a linker flag assumes that | + # some compilation flag is set. We don't pass | + # CFLAGS from environment, so we should not do it | + # for LDFLAGS too. | + LDFLAGS= | + | --prefix | --enable-static | --enable-shared > %if %{with systemd} > -DWITH_SYSTEMD:BOOL=ON \ > -DSYSTEMD_UNIT_DIR:PATH=%{_unitdir} \ > diff --git a/test/app-tap/pwd.skipcond b/test/app-tap/pwd.skipcond > new file mode 100644 > index 000000000..cebc6706c > --- /dev/null > +++ b/test/app-tap/pwd.skipcond > @@ -0,0 +1,11 @@ > +import subprocess > + > +# Disabled at CentOS 8 build due to issue #4592. > +try: > + if subprocess.check_output(["rpm --eval '%{centos_ver}'"], \ > + shell=True).strip() == '8': > + self.skip = 1 Made fixes suggested by flake8: test/app-tap/pwd.skipcond:5:64: E502 the backslash is redundant between brackets test/app-tap/pwd.skipcond:6:13: E128 continuation line under-indented for visual indent Eliminated using of shell=True: we can split arguments ourself, there is no problem with it. | diff --git a/test/app-tap/pwd.skipcond b/test/app-tap/pwd.skipcond | index cebc6706c..cf97461bc 100644 | --- a/test/app-tap/pwd.skipcond | +++ b/test/app-tap/pwd.skipcond | @@ -1,9 +1,9 @@ | import subprocess | | -# Disabled at CentOS 8 build due to issue #4592. | +# Disable the test on CentOS 8 until gh-4592 will be resolved. | try: | - if subprocess.check_output(["rpm --eval '%{centos_ver}'"], \ | - shell=True).strip() == '8': | + cmd = ['rpm', '--eval', '%{centos_ver}'] | + if subprocess.check_output(cmd).strip() == '8': | self.skip = 1 | except: | pass > +except: > + pass > + > +# vim: set ft=python: > -- > 2.17.1 >