[Tarantool-patches] [PATCH v4 0/2] gitlab-ci: implement openSUSE testing

[Tarantool-patches] [PATCH v4 0/2] gitlab-ci: implement openSUSE testing

[Alexander V. Tikhonov]

Implement openSUSE testing in gitlab-ci. It needed the following changes

1) packpack openSUSE build implementation - commited [3].
2) added '--no-undefined' linker flag leading to fails while building tests - commited [1].
3) new fix suggestion from A.Turenko with new test [2].
4) patch for implementation testing for openSUSE.

[1]: https://github.com/tarantool/tarantool/commit/f526debcd84ae2d7bdc6c172f9a75d894ecc15dd
[2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-August/019031.html
[3]: https://github.com/packpack/packpack/pull/121

Alexander V. Tikhonov (2):
  vinyl: check vinyl_dir existence at bootstrap
  gitlab-ci: add openSUSE packages build jobs

---

Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4562-suse-pack-full-ci
Issue: https://github.com/tarantool/tarantool/issues/4562
Issue: https://github.com/tarantool/tarantool/issues/4594

 .gitlab-ci.yml            | 24 ++++++++++++++++++++++++
 rpm/tarantool.spec        | 16 +++++++++++-----
 src/box/memtx_engine.c    |  2 +-
 src/box/recovery.cc       |  4 ++--
 src/box/vy_log.c          |  4 ++--
 src/box/wal.c             |  2 +-
 src/box/xlog.c            |  4 +++-
 src/box/xlog.h            |  6 +++---
 test/box-tap/cfg.test.lua | 23 ++++++++++++++++++++++-
 tools/update_repo.sh      |  6 ++++--
 10 files changed, 73 insertions(+), 18 deletions(-)

-- 
2.17.1

[Tarantool-patches] [PATCH v4 1/2] vinyl: check vinyl_dir existence at bootstrap

[Alexander V. Tikhonov]

During implementation of openSUSE got failed box-tap/cfg.test.lua test.
Found that when memtx_dir wasn't existed, while vinyl_dir existed and
errno was set to ENOENT, box configuration succeeded, but it shouldn't.
Reason of this wrong behaviour was that not all of the failure paths in
xdir_scan() were set errno, but the caller assumed it.

Usual C convention is to report success or failure using a return
value and set errno at any error. So a caller usually just checks a
return value and if it means a failure (usually -1), it checks errno
to determine an exact reason.

Usual convention in tarantool is a bit different: we use a special
diagnostics area to report a reason of a failure.

Not all failure paths of xdir_scan() sets errno (including our
'invalid instance UUID' case), so we cannot be sure that errno is
not remains unchanged after a failure of the function.

However the solution with checking errno against ENOENT (No such file
or directory) is not good. For example:

- What if xdir_scan() would be changed in future and, say, some call
  will rewrite errno after the opendir() call?
- What if some other call inside xdir_scan() will set ENOENT: say,
  open() in xdir_open_cursor() due to some race?

We lean on implementation details of the callee, not its contract. This
way is too fragile and it should either check whether the directory
exists before xdir_scan() call or pass a flag to xdir_scan() whether
the directory should exist. Decided to use second variant - it does not
lead to code duplication.

Added subtest to box-tap/cfg.test.lua test file, to check the currently
fixed issue.

Closes #4594
Needed for #4562

Co-authored-by: Alexander Turenko <alexander.turenko@tarantool.org>
---
 src/box/memtx_engine.c    |  2 +-
 src/box/recovery.cc       |  4 ++--
 src/box/vy_log.c          |  4 ++--
 src/box/wal.c             |  2 +-
 src/box/xlog.c            |  4 +++-
 src/box/xlog.h            |  6 +++---
 test/box-tap/cfg.test.lua | 23 ++++++++++++++++++++++-
 7 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index dfd6fce6e..9f079a6b5 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -992,7 +992,7 @@ memtx_engine_new(const char *snap_dirname, bool force_recovery,
 		    &xlog_opts_default);
 	memtx->snap_dir.force_recovery = force_recovery;
 
-	if (xdir_scan(&memtx->snap_dir) != 0)
+	if (xdir_scan(&memtx->snap_dir, true) != 0)
 		goto fail;
 
 	/*
diff --git a/src/box/recovery.cc b/src/box/recovery.cc
index d1a503cfc..cd33e7635 100644
--- a/src/box/recovery.cc
+++ b/src/box/recovery.cc
@@ -121,7 +121,7 @@ void
 recovery_scan(struct recovery *r, struct vclock *end_vclock,
 	      struct vclock *gc_vclock)
 {
-	xdir_scan_xc(&r->wal_dir);
+	xdir_scan_xc(&r->wal_dir, true);
 
 	if (xdir_last_vclock(&r->wal_dir, end_vclock) < 0 ||
 	    vclock_compare(end_vclock, &r->vclock) < 0) {
@@ -307,7 +307,7 @@ recover_remaining_wals(struct recovery *r, struct xstream *stream,
 	struct vclock *clock;
 
 	if (scan_dir)
-		xdir_scan_xc(&r->wal_dir);
+		xdir_scan_xc(&r->wal_dir, true);
 
 	if (xlog_cursor_is_open(&r->cursor)) {
 		/* If there's a WAL open, recover from it first. */
diff --git a/src/box/vy_log.c b/src/box/vy_log.c
index 311985c72..da3c50e87 100644
--- a/src/box/vy_log.c
+++ b/src/box/vy_log.c
@@ -1014,7 +1014,7 @@ vy_log_rebootstrap(void)
 int
 vy_log_bootstrap(void)
 {
-	if (xdir_scan(&vy_log.dir) < 0 && errno != ENOENT)
+	if (xdir_scan(&vy_log.dir, false) < 0)
 		return -1;
 	if (xdir_last_vclock(&vy_log.dir, &vy_log.last_checkpoint) >= 0)
 		return vy_log_rebootstrap();
@@ -1036,7 +1036,7 @@ vy_log_begin_recovery(const struct vclock *vclock)
 	 * because vinyl might not be even in use. Complain only
 	 * on an attempt to write a vylog.
 	 */
-	if (xdir_scan(&vy_log.dir) < 0 && errno != ENOENT)
+	if (xdir_scan(&vy_log.dir, false) < 0)
 		return NULL;
 
 	if (xdir_last_vclock(&vy_log.dir, &vy_log.last_checkpoint) < 0) {
diff --git a/src/box/wal.c b/src/box/wal.c
index d8c92aa36..2b894d680 100644
--- a/src/box/wal.c
+++ b/src/box/wal.c
@@ -559,7 +559,7 @@ wal_enable(void)
 	 * existing WAL files. Required for garbage collection,
 	 * see wal_collect_garbage().
 	 */
-	if (xdir_scan(&writer->wal_dir))
+	if (xdir_scan(&writer->wal_dir, true))
 		return -1;
 
 	/* Open the most recent WAL file. */
diff --git a/src/box/xlog.c b/src/box/xlog.c
index 6ccd3d68d..74f761994 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -511,13 +511,15 @@ xdir_open_cursor(struct xdir *dir, int64_t signature,
  * @return nothing.
  */
 int
-xdir_scan(struct xdir *dir)
+xdir_scan(struct xdir *dir, bool is_dir_required)
 {
 	DIR *dh = opendir(dir->dirname);        /* log dir */
 	int64_t *signatures = NULL;             /* log file names */
 	size_t s_count = 0, s_capacity = 0;
 
 	if (dh == NULL) {
+		if (!is_dir_required && errno == ENOENT)
+			return 0;
 		diag_set(SystemError, "error reading directory '%s'",
 			  dir->dirname);
 		return -1;
diff --git a/src/box/xlog.h b/src/box/xlog.h
index 9ffce598b..3400eb75f 100644
--- a/src/box/xlog.h
+++ b/src/box/xlog.h
@@ -187,7 +187,7 @@ xdir_destroy(struct xdir *dir);
  * snapshot or scan through all logs.
  */
 int
-xdir_scan(struct xdir *dir);
+xdir_scan(struct xdir *dir, bool is_dir_required);
 
 /**
  * Check that a directory exists and is writable.
@@ -821,9 +821,9 @@ xdir_open_cursor(struct xdir *dir, int64_t signature,
 #include "exception.h"
 
 static inline void
-xdir_scan_xc(struct xdir *dir)
+xdir_scan_xc(struct xdir *dir, bool is_dir_required)
 {
-	if (xdir_scan(dir) == -1)
+	if (xdir_scan(dir, is_dir_required) == -1)
 		diag_raise();
 }
 
diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
index 569b5f463..a60aa848e 100755
--- a/test/box-tap/cfg.test.lua
+++ b/test/box-tap/cfg.test.lua
@@ -6,7 +6,7 @@ local socket = require('socket')
 local fio = require('fio')
 local uuid = require('uuid')
 local msgpack = require('msgpack')
-test:plan(108)
+test:plan(109)
 
 --------------------------------------------------------------------------------
 -- Invalid values
@@ -605,5 +605,26 @@ test:ok(not box.info.listen:match(':0'), 'real port in info.listen')
 box.cfg{listen = box.NULL}
 test:is(nil, box.info.listen, 'cfg.listen reset drops info.listen')
 
+--
+-- gh-4594: when memtx_dir is not exists, but vinyl_dir exists and
+-- errno is set to ENOENT, box configuration succeeds, however it
+-- should not
+--
+vinyl_dir = fio.tempdir()
+run_script(string.format([[
+box.cfg{vinyl_dir = '%s'}
+s = box.schema.space.create('test', {engine = 'vinyl'})
+s:create_index('pk')
+os.exit(0)
+]], vinyl_dir))
+code = string.format([[
+local errno = require('errno')
+errno(errno.ENOENT)
+box.cfg{vinyl_dir = '%s'}
+os.exit(0)
+]], vinyl_dir)
+test:is(run_script(code), PANIC, "bootstrap with ENOENT from non-empty vinyl_dir")
+fio.rmtree(vinyl_dir)
+
 test:check()
 os.exit(0)
-- 
2.17.1

[Tarantool-patches] [PATCH v4 2/2] gitlab-ci: add openSUSE packages build jobs

[Alexander V. Tikhonov]

Implemented openSUSE packages build with testing for images:
opensuse-leap:15.[0-2]

Added %{sle_version} checks in Tarantool spec file according to
https://en.opensuse.org/openSUSE:Packaging_for_Leap#RPM_Distro_Version_Macros

Added opensuse-leap of 15.1 and 15.2 versions to Gitlab-CI packages
building/deploing jobs.

Closes #4562
---
 .gitlab-ci.yml       | 24 ++++++++++++++++++++++++
 rpm/tarantool.spec   | 16 +++++++++++-----
 tools/update_repo.sh |  6 ++++--
 3 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 0ead08711..05d40b013 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -430,6 +430,18 @@ debian_10:
     OS: 'debian'
     DIST: 'buster'
 
+opensuse_15_1:
+  <<: *pack_definition
+  variables:
+    OS: 'opensuse-leap'
+    DIST: '15.1'
+
+opensuse_15_2:
+  <<: *pack_definition
+  variables:
+    OS: 'opensuse-leap'
+    DIST: '15.2'
+
 # Deploy
 
 sources_deploy:
@@ -527,6 +539,18 @@ debian_10_deploy:
     OS: 'debian'
     DIST: 'buster'
 
+opensuse_15_1_deploy:
+  <<: *deploy_definition
+  variables:
+    OS: 'opensuse-leap'
+    DIST: '15.1'
+
+opensuse_15_2_deploy:
+  <<: *deploy_definition
+  variables:
+    OS: 'opensuse-leap'
+    DIST: '15.2'
+
 # Static builds
 
 static_build:
diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec
index 88b1d6b5c..eedc0312c 100644
--- a/rpm/tarantool.spec
+++ b/rpm/tarantool.spec
@@ -1,5 +1,5 @@
 # Enable systemd for on RHEL >= 7 and Fedora >= 15
-%if (0%{?fedora} >= 15 || 0%{?rhel} >= 7)
+%if (0%{?fedora} >= 15 || 0%{?rhel} >= 7 || 0%{?sle_version} >= 1500)
 %bcond_without systemd
 %else
 %bcond_with systemd
@@ -7,7 +7,7 @@
 
 BuildRequires: cmake >= 2.8
 BuildRequires: make
-%if (0%{?fedora} >= 22 || 0%{?rhel} >= 7)
+%if (0%{?fedora} >= 22 || 0%{?rhel} >= 7 || 0%{?sle_version} >= 1500)
 # RHEL 6 requires devtoolset
 BuildRequires: gcc >= 4.5
 BuildRequires: gcc-c++ >= 4.5
@@ -53,6 +53,12 @@ Requires(preun): initscripts
 %bcond_without backtrace
 %endif
 
+# openSuSE sets its own build directory in its macros, but we
+# want to use in-source build there to simplify the RPM spec.
+%if (0%{?sle_version} >= 1500)
+%global __builddir .
+%endif
+
 %if %{with backtrace}
 BuildRequires: libunwind-devel
 #
@@ -78,7 +84,7 @@ BuildRequires: python2-six >= 1.9.0
 BuildRequires: python2-gevent >= 1.0
 BuildRequires: python2-yaml >= 3.0.9
 %else
-%if (0%{?rhel} != 6)
+%if (0%{?rhel} != 6 || 0%{?sle_version} >= 1500)
 BuildRequires: python >= 2.7
 BuildRequires: python-six >= 1.9.0
 BuildRequires: python-gevent >= 1.0
@@ -105,7 +111,7 @@ Requires: /etc/services
 # Deps for built-in package manager
 # https://github.com/tarantool/tarantool/issues/2612
 Requires: openssl
-%if (0%{?fedora} >= 22 || 0%{?rhel} >= 8)
+%if (0%{?fedora} >= 22 || 0%{?rhel} >= 8 || 0%{?sle_version} >= 1500)
 # RHEL <= 7 doesn't support Recommends:
 Recommends: tarantool-devel
 Recommends: git-core
@@ -164,7 +170,7 @@ rm -rf %{buildroot}%{_datarootdir}/doc/tarantool/
 
 %check
 %if "%{_ci}" == "travis"
-%if (0%{?fedora} >= 22 || 0%{?rhel} >= 7)
+%if (0%{?fedora} >= 22 || 0%{?rhel} >= 7 || 0%{?sle_version} >= 1500)
 cd test && ./test-run.py --force -j 1 unit/ app/ app-tap/ box/ box-tap/ engine/ vinyl/
 %endif
 %else
diff --git a/tools/update_repo.sh b/tools/update_repo.sh
index 5a68e3e05..02def11ff 100755
--- a/tools/update_repo.sh
+++ b/tools/update_repo.sh
@@ -6,7 +6,7 @@ rm_dir='rm -rf'
 mk_dir='mkdir -p'
 ws_prefix=/tmp/tarantool_repo_s3
 
-alloss='ubuntu debian el fedora'
+alloss='ubuntu debian el fedora opensuse-leap'
 product=tarantool
 remove=
 force=
@@ -31,6 +31,8 @@ function get_os_dists {
         alldists='6 7 8'
     elif [ "$os" == "fedora" ]; then
         alldists='27 28 29 30 31'
+    elif [ "$os" == "opensuse-leap" ]; then
+        alldists='15.0 15.1 15.2'
     fi
 
     echo "$alldists"
@@ -934,7 +936,7 @@ if [ "$os" == "ubuntu" -o "$os" == "debian" ]; then
     # unlock the publishing
     $rm_file $ws_lockfile
     popd
-elif [ "$os" == "el" -o "$os" == "fedora" ]; then
+elif [ "$os" == "el" -o "$os" == "fedora" -o "$os" == "opensuse-leap" ]; then
     # RPM packages structure needs different paths for binaries and sources
     # packages, in this way it is needed to call the packages registering
     # script twice with the given format:
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v4 1/2] vinyl: check vinyl_dir existence at bootstrap

[Alexander Turenko]

> vinyl: check vinyl_dir existence at bootstrap

It looks like you added the check, but it was present before the change.
'Fix check' better describes the change.

> During implementation of openSUSE got failed box-tap/cfg.test.lua test.
> Found that when memtx_dir wasn't existed, while vinyl_dir existed and
> errno was set to ENOENT, box configuration succeeded, but it shouldn't.
> Reason of this wrong behaviour was that not all of the failure paths in
> xdir_scan() were set errno, but the caller assumed it.

In fact this sentence describes everything that I asked in [1].
However, I don't mind including more information. But it should be clear
for a reader: see the suggestions below.

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-August/019031.html

> Usual C convention is to report success or failure using a return
> value and set errno at any error. So a caller usually just checks a
> return value and if it means a failure (usually -1), it checks errno
> to determine an exact reason.
> 
> Usual convention in tarantool is a bit different: we use a special
> diagnostics area to report a reason of a failure.
> 
> Not all failure paths of xdir_scan() sets errno (including our
> 'invalid instance UUID' case), so we cannot be sure that errno is
> not remains unchanged after a failure of the function.
> 
> However the solution with checking errno against ENOENT (No such file
> or directory) is not good. For example:
> 
> - What if xdir_scan() would be changed in future and, say, some call
>   will rewrite errno after the opendir() call?
> - What if some other call inside xdir_scan() will set ENOENT: say,
>   open() in xdir_open_cursor() due to some race?
> 
> We lean on implementation details of the callee, not its contract. This
> way is too fragile and it should either check whether the directory
> exists before xdir_scan() call or pass a flag to xdir_scan() whether
> the directory should exist. Decided to use second variant - it does not
> lead to code duplication.

(Yes, there is a lot of text below. But I'm sure it deserves a time to
read it. I hope it sounds friendly, not edgy.)

This part of the commit message is like a bag of facts. But what is the
idea or ideas you want to express?

I'll give a couple of examples from your message. There are facts about
failure reporting conventions. But why it is highlighted here? No
conclusions, no connections to the rest of the message. Or, say, copy of
parts of the discussion about possible solutions: `errno = 0;` vs
`is_dir_required` without mention of the former. A reader has no chance
to understand why it is written here.

I don't want to continue pointing places, where a reader will be
confused: you can (and should) do it youself. It is better to give
general suggestions, how to explain things. I'm not much experienced
here, to be honest, but there are points I would share.

First of all, I strongly suggest to avoid copying of another person
wording. When you write an explanation youself, you will ask youself
questions about it. Whether it is correct? Whether it gives ideas you
want to express? How to better structurize it? The only way to improve
this skill is practice.

I suggest to decide about core ideas, read all materials (code, test
results, discussions), but than move the maretials aside and explain the
ideas in your words.

This is the main point of this email, so I'll stop here again. Don't
copy-paste. My past review comments had sense in its context, but just
confuses a reader here. Some of them were given as background
information for you, some comments compare different ways to solve the
problem. It is the information for you: to analyze and decide how to
make your patch better. So, again: decide about core ideas and express
them. In your words.

Of course, you may find youself on the point "I don't know how to
express it". That's the key moment. Here I usually ask myself whether I
really understand what is going on. I got back to the code and
materials, re-read them, perform additional tests, experiment with
different solutions. If things become clear for me after this, I can
continue describing them. But sometimes it appears that I'm unable to
defend a choosen way to solve a problem and I'm going to rewrite my
code.

It may also appear that my description becomes large and vague: an idea
is missed between details. Maybe cut it off entirely? Or give just core
idea, without details? Or structurize to make it clear where the idea
itself and where details that can be skipped by a reader?

The last suggestion is to track context of a reader. Imagine that you
know nothing about the problem. After the first paragraph you got ideas
it gives. The next paragraph should be clear in this context. For
example, it may start with:

- 'There are pitfalls a developer should aware',
- 'There are alternative solutions',
- 'The implementation lean on the following assumptions',
- 'The problem appears only under the following conditions',
- or, say, just 'Usage example',
- maybe even just 'Usual convention is' if it is connected to the
  general context later.

All those clauses marks how paragraphs (ideas, in fact) are connected to
each other and allows a reader to don't lose context while reading.

> Added subtest to box-tap/cfg.test.lua test file, to check the currently
> fixed issue.

This is redundant, IMHO. Almost every change should be accompanied by a
test case.

> diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
> index 569b5f463..a60aa848e 100755
> --- a/test/box-tap/cfg.test.lua
> +++ b/test/box-tap/cfg.test.lua

Our guidelines suggest to extract a bugfix test into a separate file.

Re: [Tarantool-patches] [PATCH v4 1/2] vinyl: check vinyl_dir existence at bootstrap

[Alexander V. Tikhonov]

Hi Alexander, thanks a lot for your comments, I've corrected topic
message of the commit as you suggested and completely rewrote the
commit message in myown words. Also test split from the common one.

On Mon, Aug 17, 2020 at 12:26:59AM +0300, Alexander Turenko wrote:
> > vinyl: check vinyl_dir existence at bootstrap
> 
> It looks like you added the check, but it was present before the change.
> 'Fix check' better describes the change.
> 
> > During implementation of openSUSE got failed box-tap/cfg.test.lua test.
> > Found that when memtx_dir wasn't existed, while vinyl_dir existed and
> > errno was set to ENOENT, box configuration succeeded, but it shouldn't.
> > Reason of this wrong behaviour was that not all of the failure paths in
> > xdir_scan() were set errno, but the caller assumed it.
> 
> In fact this sentence describes everything that I asked in [1].
> However, I don't mind including more information. But it should be clear
> for a reader: see the suggestions below.
> 
> [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-August/019031.html
> 
> > Usual C convention is to report success or failure using a return
> > value and set errno at any error. So a caller usually just checks a
> > return value and if it means a failure (usually -1), it checks errno
> > to determine an exact reason.
> > 
> > Usual convention in tarantool is a bit different: we use a special
> > diagnostics area to report a reason of a failure.
> > 
> > Not all failure paths of xdir_scan() sets errno (including our
> > 'invalid instance UUID' case), so we cannot be sure that errno is
> > not remains unchanged after a failure of the function.
> > 
> > However the solution with checking errno against ENOENT (No such file
> > or directory) is not good. For example:
> > 
> > - What if xdir_scan() would be changed in future and, say, some call
> >   will rewrite errno after the opendir() call?
> > - What if some other call inside xdir_scan() will set ENOENT: say,
> >   open() in xdir_open_cursor() due to some race?
> > 
> > We lean on implementation details of the callee, not its contract. This
> > way is too fragile and it should either check whether the directory
> > exists before xdir_scan() call or pass a flag to xdir_scan() whether
> > the directory should exist. Decided to use second variant - it does not
> > lead to code duplication.
> 
> (Yes, there is a lot of text below. But I'm sure it deserves a time to
> read it. I hope it sounds friendly, not edgy.)
> 
> This part of the commit message is like a bag of facts. But what is the
> idea or ideas you want to express?
> 
> I'll give a couple of examples from your message. There are facts about
> failure reporting conventions. But why it is highlighted here? No
> conclusions, no connections to the rest of the message. Or, say, copy of
> parts of the discussion about possible solutions: `errno = 0;` vs
> `is_dir_required` without mention of the former. A reader has no chance
> to understand why it is written here.
> 
> I don't want to continue pointing places, where a reader will be
> confused: you can (and should) do it youself. It is better to give
> general suggestions, how to explain things. I'm not much experienced
> here, to be honest, but there are points I would share.
> 
> First of all, I strongly suggest to avoid copying of another person
> wording. When you write an explanation youself, you will ask youself
> questions about it. Whether it is correct? Whether it gives ideas you
> want to express? How to better structurize it? The only way to improve
> this skill is practice.
> 
> I suggest to decide about core ideas, read all materials (code, test
> results, discussions), but than move the maretials aside and explain the
> ideas in your words.
> 
> This is the main point of this email, so I'll stop here again. Don't
> copy-paste. My past review comments had sense in its context, but just
> confuses a reader here. Some of them were given as background
> information for you, some comments compare different ways to solve the
> problem. It is the information for you: to analyze and decide how to
> make your patch better. So, again: decide about core ideas and express
> them. In your words.
> 
> Of course, you may find youself on the point "I don't know how to
> express it". That's the key moment. Here I usually ask myself whether I
> really understand what is going on. I got back to the code and
> materials, re-read them, perform additional tests, experiment with
> different solutions. If things become clear for me after this, I can
> continue describing them. But sometimes it appears that I'm unable to
> defend a choosen way to solve a problem and I'm going to rewrite my
> code.
> 
> It may also appear that my description becomes large and vague: an idea
> is missed between details. Maybe cut it off entirely? Or give just core
> idea, without details? Or structurize to make it clear where the idea
> itself and where details that can be skipped by a reader?
> 
> The last suggestion is to track context of a reader. Imagine that you
> know nothing about the problem. After the first paragraph you got ideas
> it gives. The next paragraph should be clear in this context. For
> example, it may start with:
> 
> - 'There are pitfalls a developer should aware',
> - 'There are alternative solutions',
> - 'The implementation lean on the following assumptions',
> - 'The problem appears only under the following conditions',
> - or, say, just 'Usage example',
> - maybe even just 'Usual convention is' if it is connected to the
>   general context later.
> 
> All those clauses marks how paragraphs (ideas, in fact) are connected to
> each other and allows a reader to don't lose context while reading.
> 
> > Added subtest to box-tap/cfg.test.lua test file, to check the currently
> > fixed issue.
> 
> This is redundant, IMHO. Almost every change should be accompanied by a
> test case.
> 
> > diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
> > index 569b5f463..a60aa848e 100755
> > --- a/test/box-tap/cfg.test.lua
> > +++ b/test/box-tap/cfg.test.lua
> 
> Our guidelines suggest to extract a bugfix test into a separate file.

Re: [Tarantool-patches] [PATCH v4 2/2] gitlab-ci: add openSUSE packages build jobs

[Alexander Turenko]

LGTM.

WBR, Alexander Turenko.

On Fri, Aug 14, 2020 at 12:59:58PM +0300, Alexander V. Tikhonov wrote:
> Implemented openSUSE packages build with testing for images:
> opensuse-leap:15.[0-2]
> 
> Added %{sle_version} checks in Tarantool spec file according to
> https://en.opensuse.org/openSUSE:Packaging_for_Leap#RPM_Distro_Version_Macros
> 
> Added opensuse-leap of 15.1 and 15.2 versions to Gitlab-CI packages
> building/deploing jobs.
> 
> Closes #4562
> ---
>  .gitlab-ci.yml       | 24 ++++++++++++++++++++++++
>  rpm/tarantool.spec   | 16 +++++++++++-----
>  tools/update_repo.sh |  6 ++++--
>  3 files changed, 39 insertions(+), 7 deletions(-)

Re: [Tarantool-patches] [PATCH v4 0/2] gitlab-ci: implement openSUSE testing

[Kirill Yukhin]

Hello,

On 14 авг 12:59, Alexander V. Tikhonov wrote:
> Implement openSUSE testing in gitlab-ci. It needed the following changes
> 
> 1) packpack openSUSE build implementation - commited [3].
> 2) added '--no-undefined' linker flag leading to fails while building tests - commited [1].
> 3) new fix suggestion from A.Turenko with new test [2].
> 4) patch for implementation testing for openSUSE.
> 
> [1]: https://github.com/tarantool/tarantool/commit/f526debcd84ae2d7bdc6c172f9a75d894ecc15dd
> [2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-August/019031.html
> [3]: https://github.com/packpack/packpack/pull/121
> 
> Alexander V. Tikhonov (2):
>   vinyl: check vinyl_dir existence at bootstrap
>   gitlab-ci: add openSUSE packages build jobs
> 
> ---
> 
> Github: https://github.com/tarantool/tarantool/tree/avtikhon/gh-4562-suse-pack-full-ci
> Issue: https://github.com/tarantool/tarantool/issues/4562
> Issue: https://github.com/tarantool/tarantool/issues/4594

I've checked your patch into 1.10, 2.4, 2.5 and master.

--
Regards, Kirill Yukhin