From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (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 8C2DB445320 for ; Mon, 6 Jul 2020 09:30:49 +0300 (MSK) Date: Mon, 6 Jul 2020 09:30:46 +0300 From: "Alexander V. Tikhonov" Message-ID: <20200706063046.GA11453@hpalx> References: <1cb7469ca7d4ed1c345b32707fec46663c670fa4.1587632526.git.avtikhon@tarantool.org> <20200705003736.cgur5v5l2bu44rgr@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200705003736.cgur5v5l2bu44rgr@tkn_work_nb> Subject: Re: [Tarantool-patches] [PATCH v1] build: implement SuSE build with testing List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org Hi Alexander, thanks a lot for the deep review, I've made all your suggestions. On Sun, Jul 05, 2020 at 03:37:36AM +0300, Alexander Turenko wrote: > On Thu, Apr 23, 2020 at 12:03:44PM +0300, Alexander V. Tikhonov wrote: > > Added makefile to build the SuSE based packages. > > Makefile? Is is about packpack update? It is not actual anymore (since > we decided to update existing makefile). But I would rather just remove > the comment, because it is about packpack patch, not this one. > Right, removed. > > Implemented build with testing for opensuse images: > > SuSE 15.0, 15.1, 15.2 > > Nit: SuSE **Leap** 15.0, 15.1, 15.2. > Corrected. > Are we plan to deploy them to our repositories? I guess a user who asked > for this expects that we'll offer those packages like other ones. > > Ouch, I see, the branch already contains deployment rules. So this > comment is not actual anymore. > Ok. > > Tarantool spec file changed with: > > Added checks of %{sle_version} according to > > https://en.opensuse.org/openSUSE:Packaging_for_Leap#RPM_Distro_Version_Macros > > Nit: I think that using of %{sle_version} macro is quite obvious and > does not require a comment here. But up to you. > Right, I'd like to save the link on %{sle_version} for fast search. > > > > Found that opensuse adding linker flag like '--no-undefined' which > > produces the fails on building test modules at app-tap/app/box-tap > > suites. Decided to block this flag. > > > > Running tests by make test, because opensuse uses out-of-source > > build by default. > > Set complete testing except replication suite. > > Test box-tap/cfg fails and temporary blocked according to the > > issue created for it #4594. > > Don't forget to update those explanations according to everything said > below. > Sure, I've updated it as suggested. > > Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4562-suse-pack-full-ci > > Issue: https://github.com/tarantool/tarantool/issues/4562 > > > diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec > > index ff95ed646..09de015e2 100644 > > --- a/rpm/tarantool.spec > > +++ b/rpm/tarantool.spec > > I give some recommendations how to better handle out-of-source build in > different places, but now another idea comes into my mind. We can just > define `%global %__builddir .`, that's all. (Don't forget to leave a > comment for it in the spec, like 'Force in-source build on openSUSE to > simplify the RPM spec'.) > Ok, I've used your suggested macro definition and all my changes below became unneeded. > > %build > > # RHBZ #1301720: SYSCONFDIR an LOCALSTATEDIR must be specified explicitly > > -%cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo \ > > +%if (0%{?sle_version} >= 1500) > > +# SuSE uses only out-of-source builds in its macros > > +%cmake .. \ > > + -DCMAKE_INSTALL_PREFIX:PATH=/usr \ > > +%else > > +%cmake . \ > > +%endif > > + -DCMAKE_BUILD_TYPE=RelWithDebInfo \ > > -DCMAKE_INSTALL_LOCALSTATEDIR:PATH=%{_localstatedir} \ > > -DCMAKE_INSTALL_SYSCONFDIR:PATH=%{_sysconfdir} \ > > It would look better I think if we'll just use '%cmake > %{_sourcedir} <...>'. It seems it should work with packpack. > > It seems -DCMAKE_INSTALL_PREFIX:PATH=/usr is not needed, %cmake macro > already has it on opensuse-leap-15.2. > > But I hope that %__builddir redefinition will work, it would allow to > kepp the RPM spec almost same for different distros. > Right not need any more - removed. > > +%if (0%{?sle_version} == 0) > > %install > > +%endif > > %make_install > > Do you want to do `make install` in the %build phase rather than > %install? It feels as not right way to overcome a problem. > > I don't see anything that explains this change in the commit message or > in a comment here. How should I review it? Build myself, see a result > and try to guess the reason? It would be the waste of time: you already > did this and could explain. > > I have a guess! Is it to keep CWD after 'cd build' in %cmake expansion? > If so, you can just use %cmake_install when it is defined and > %make_install otherwise. > > But I hope that redefinition of %__builddir will work. > Right not need any more - removed. > > %check > > -%if (0%{?fedora} >= 22 || 0%{?rhel} >= 7) > > +%if (0%{?fedora} >= 22 || 0%{?rhel} >= 7 || 0%{?sle_version} >= 1500) > > # https://github.com/tarantool/tarantool/issues/1227 > > echo "self.skip = True" > ./test/app/socket.skipcond > > # https://github.com/tarantool/tarantool/issues/1322 > > echo "self.skip = True" > ./test/app/digest.skipcond > > # run a safe subset of the test suite > > +%if (0%{?sle_version} >= 1500) > > +# SuSE uses only out-of-source builds always at build/ subdir in its macros > > +cd build && TEST_RUN_EXCLUDE='replication/' make test-force > > +%else > > cd test && ./test-run.py --force -j 1 unit/ app/ app-tap/ box/ box-tap/ engine/ vinyl/ > > %endif > > +%endif > > First, this hunk is different now and the branch should be rebased on > top of current master. > > Second, it is better to avoid duplication and use something like: > > | %if 0%{?__builddir:1} > | cd %__builddir > | %endif > > But I hope %__builddir redefinition will work. > Right not need any more - removed. > > diff --git a/test/app-tap/CMakeLists.txt b/test/app-tap/CMakeLists.txt > > index ee67cf533..68aa3690b 100644 > > --- a/test/app-tap/CMakeLists.txt > > +++ b/test/app-tap/CMakeLists.txt > > @@ -1 +1,2 @@ > > +SET(CMAKE_SHARED_LINKER_FLAGS "") > > Nit: We usually type 'set()' lowercased in our sources. > > After the first glance I was sceptic about redefinition of a cmake > variable that may affect build rules for the whole project. But then I > read about CMake scopes and found that the change affects only this > directory, because add_subdirectory() creates a new variable scope. > Then I asked myself, whether it applicable for cached variables > (CMAKE_SHARED_LINKER_FLAGS is cached). And found that set() will create > a new non-cached variable, which will be used directory-wide instead of > global one and will not affect the global cached value (see [1]). So it > is okay. > > But we can do it less intruisive: > > | string(REPLACE "-Wl,--no-undefined" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}") > Sure, I've changed it as you suggested. > Also I think it should be properly commented, like 'The dynamic > libraries will be loaded from tarantool executable and will use symbols > from it. So it is completely okay to have unresolved symbols at build > time'. > > BTW, you should also update luajit submodule in the same way; new tests > are added here: > > - test/gh-4427-ffi-sandwich/CMakeLists.txt > - test/lj-flush-on-trace/CMakeLists.txt > Sure, updated too. > [1]: https://cmake.org/cmake/help/v3.0/command/set.html > > > diff --git a/test/box-tap/cfg.skipcond b/test/box-tap/cfg.skipcond > > index 7950a5d93..9d5535f87 100644 > > --- a/test/box-tap/cfg.skipcond > > +++ b/test/box-tap/cfg.skipcond > > @@ -1,8 +1,14 @@ > > +import os > > import platform > > > > # Disabled on FreeBSD due to fail #4271: > > # Data segment size exceeds process limit > > if platform.system() == 'FreeBSD': > > self.skip = 1 > > +try: > > + if os.popen('lsb_release -i').read().split(':')[1].strip() == 'openSUSE': > > + self.skip = 1 > > +except IndexError: > > + pass > > > > # vim: set ft=python: > > You should place the issue number (#4594) here. > Right, added. > Neither this file, nor the issue give any idea what is wrong with the > test. We cannot disable a test just because it fails: why we writing > them so? We should at least ensure that it is the testing problem and we > can provide openSUSE packages and say that tarantool works on openSUSE > without critical problems. > > The test is disabled on FreeBSD and here there is the certain reason. It > looks like related to os.execute() and so we can state something about > FreeBSD support in tarantool: it works, but there are problems around > forking processes from tarantool. Updated issue, added comment to commit message.