<HTML><BODY><br><br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Четверг, 31 октября 2019, 16:10 +03:00 от Alexander Turenko <alexander.turenko@tarantool.org>:<br><br><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15725274111048204139_BODY">Alexander, your mistakes are often the same as ones in other patches and<br>
often the same that was already seen on previous review iterations: I<br>
think you need some kind of check-list to perform it before sending a<br>
patch.<br><br>
Please, organize your work in the way that will allow you to don't miss<br>
review comments.<br></div></div></div></div></blockquote><p>Created instructions on Confluence.</p><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15725274111048204139_BODY"><br>
I run build and tests and tests segfaults very often. I didn't<br>
investigate the crashes, but Vlad says they can be related to<br><a href="https://github.com/tarantool/tarantool/issues/4597" target="_blank">https://github.com/tarantool/tarantool/issues/4597</a><br><br>
I'll wait for a fix, because I'm unable to test your patch when master<br>
crashes randomly.</div></div></div></div></blockquote>Right, some latest patches broke the stability.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15725274111048204139_BODY"><br><br>
I tried to install the resulting package inside ususal centos:8 docker<br>
image and got the following error:<br><br>
| # yum install /pwd/build/tarantool-2.3.0.191-1.el8.x86_64.rpm<br>
| Error:<br>
| Problem: conflicting requests<br>
| - nothing provides libunwind-x86_64.so.8()(64bit) needed by tarantool-2.3.0.191-1.el8.x86_64<br>
| - nothing provides libunwind.so.8()(64bit) needed by tarantool-2.3.0.191-1.el8.x86_64<br>
| (try to add '--skip-broken' to skip uninstallable packages or '--nobest' to use not only best candidate packages)<br><br>
It seems libunwind was moved from base repository to epel. I think we<br>
should not depend on libunwind (and disable related features for now),<br>
but maybe build it statically and bundle into tarantool in the future<br>
(if possible).<br></div></div></div></div></blockquote><p>Added to instructions on Confluence not to miss the check again.<br></p><p>Discussed and decided to block backtrace to avoid of libunwind use. </p><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15725274111048204139_BODY"><br>
See other comments below.<br><br>
WBR, Alexander Turenko.<br><br>
> build: added centos 8<br><br>
Use imperative mood.</div></div></div></div></blockquote>Fixed.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15725274111048204139_BODY"><br><br>
><br>
> Added CentOS 8 image build, ready for Packpack use.<br><br>
Let me cite my previous review comment:<br><br>
| I don't got the first sentence. Packpack provides build images and a<br>
| tool to use them. It is not something that a user need to use<br>
| tarantool.<br><br><a href="https://lists.tarantool.org/pipermail/tarantool-patches/2019-October/012046.html" target="_blank">https://lists.tarantool.org/pipermail/tarantool-patches/2019-October/012046.html</a><br></div></div></div></div></blockquote>Changed.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15725274111048204139_BODY"><br>
> <br>
> Found that CentOS 8 completely moved to the use of python3<br>
> while Tarantool testing still uses python2, also found that<br>
> CentOS 8 changed naming of the python packages to python2.<br>
> To make these changes workable with Tarantool build added<br>
> use of the python2 packages instead of python.<br><br>
This can be said simpler: CentOS 8 uses 'python2-' prefix for Python 2<br>
modules rather the 'python-'.<br></div></div></div></div></blockquote>Used the suggested sentence with additions.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15725274111048204139_BODY"><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 which was allready<br><br>
allready -> already</div></div></div></div></blockquote>Mistake corrected.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15725274111048204139_BODY"><br><br>
> fixed in local commit 7732daded14f86a314d44486e306e17bbc6ab293<br>
> (lua: treat ENOENT as success in getpwall/getgrall), but it<br>
> wasn't merged into trunk - decided to block temporary the test<br>
> till the correct fix will be created with the new issue created:<br>
> <a href="https://github.com/tarantool/tarantool/issues/4592" target="_blank">https://github.com/tarantool/tarantool/issues/4592</a><br><br>
I don't sure this fix was right, so I would not say that 'it was already<br>
fixed'. I think than mentioning of an issue that track the problem is<br>
enough here.<br></div></div></div></div></blockquote>Removed extra information.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15725274111048204139_BODY"><br>
> <br>
> Added CentOS 8 into regular testing at gitlab-ci.<br><br>
Let me cite the comment from my previous review:<br><br>
| We should add it to Travis-CI too, because deployments are made only<br>
| from Travis-CI.<br><br><a href="https://lists.tarantool.org/pipermail/tarantool-patches/2019-October/012046.html" target="_blank">https://lists.tarantool.org/pipermail/tarantool-patches/2019-October/012046.html</a><br></div></div></div></div></blockquote>Corrected.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15725274111048204139_BODY"><br>
> <br>
> Closes #4543<br>
> ---<br>
> <br>
> Github: <a href="https://github.com/tarantool/tarantool/tree/avtikhon/gh-4543-centos8" target="_blank">https://github.com/tarantool/tarantool/tree/avtikhon/gh-4543-centos8</a><br><br>
It seems you didn't updated your branch.</div></div></div></div></blockquote>Right, forgot to push.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15725274111048204139_BODY"><br><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>
> rpm/tarantool.spec | 11 ++++++++++-<br>
> test/app-tap/pwd.skipcond | 7 +++++++<br>
> 3 files changed, 23 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 431730b67..655e68cdc 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/rpm/tarantool.spec b/rpm/tarantool.spec<br>
> index 10daf1458..5930f79d7 100644<br>
> --- a/rpm/tarantool.spec<br>
> +++ b/rpm/tarantool.spec<br>
> @@ -15,7 +15,9 @@ BuildRequires: gcc-c++ >= 4.5<br>
> BuildRequires: coreutils<br>
> BuildRequires: sed<br>
> BuildRequires: readline-devel<br>
> +%if (0%{?fedora} > 0 || 0%{?rhel} <= 7)<br>
> BuildRequires: libyaml-devel<br>
> +%endif<br><br>
Please, rebase: we removed this dependency on master and 2.2.</div></div></div></div></blockquote>Got the changes with re-base.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15725274111048204139_BODY"><br><br>
We anyway need libyaml-devel on 1.10 and 2.1, but there is no need to<br>
exclude the dependency on those branches. I already said that:<br><a href="https://github.com/packpack/packpack-docker-images/pull/39#discussion_r338324713" target="_blank">https://github.com/packpack/packpack-docker-images/pull/39#discussion_r338324713</a></div></div></div></div></blockquote>Ok.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15725274111048204139_BODY"><br><br>
> BuildRequires: openssl-devel<br>
> BuildRequires: libicu-devel<br>
> #BuildRequires: msgpuck-devel<br>
> @@ -64,12 +66,19 @@ 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>
> +BuildRequires: python2 >= 2.7<br>
> +BuildRequires: python2-six >= 1.9.0<br>
> +BuildRequires: python2-greenlet<br><br>
Is not this the dependency of python2-gevent? I think we should not add<br>
it explicitly here if it is so.</div></div></div></div></blockquote>Removed python2-greenlet due to python2-gevent installs it in dependences.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15725274111048204139_BODY"><br><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>
> diff --git a/test/app-tap/pwd.skipcond b/test/app-tap/pwd.skipcond<br>
> new file mode 100644<br>
> index 000000000..b001fb82c<br>
> --- /dev/null<br>
> +++ b/test/app-tap/pwd.skipcond<br>
> @@ -0,0 +1,7 @@<br>
> +import subprocess<br>
> +<br>
> +# Disabled at CentOS 8 build due to issue #4592.<br>
> +if subprocess.check_output(["rpm --eval '%{centos_ver}'"], shell=True).strip() == '8':<br><br>
PEP8 forbids so long lines.</div></div></div></div></blockquote>Corrected.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15725274111048204139_BODY"><br><br>
> + self.skip = 1<br>
> +<br>
> +# vim: set ft=python:<br>
> -- <br>
> 2.17.1<br>
> <br><br></div></div></div></div></blockquote>
<br>
<br>-- <br>Alexander Tikhonov<br></BODY></HTML>