From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (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 A51AE4429E1 for ; Sun, 5 Jul 2020 03:38:25 +0300 (MSK) Date: Sun, 5 Jul 2020 03:37:36 +0300 From: Alexander Turenko Message-ID: <20200705003736.cgur5v5l2bu44rgr@tkn_work_nb> References: <1cb7469ca7d4ed1c345b32707fec46663c670fa4.1587632526.git.avtikhon@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1cb7469ca7d4ed1c345b32707fec46663c670fa4.1587632526.git.avtikhon@tarantool.org> 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 V. Tikhonov" Cc: Oleg Piskunov , tarantool-patches@dev.tarantool.org 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. > Implemented build with testing for opensuse images: > SuSE 15.0, 15.1, 15.2 Nit: SuSE **Leap** 15.0, 15.1, 15.2. 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. > 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. > > 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. > 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'.) > %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. > +%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. > %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. > 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}") 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 [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. 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.