Alexander, I'm completely LGTM with the patch, please push it. >Пятница, 8 ноября 2019, 18:22 +03:00 от Alexander Turenko : > >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 < alexander.turenko@tarantool.org > > | 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 >> -- Alexander Tikhonov