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