Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Alexander Turenko <alexander.turenko@tarantool.org>,
	tarantool-patches@freelists.org,
	Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Subject: [PATCH v2] travis-ci: set right flags in release testing jobs
Date: Mon, 29 Apr 2019 15:57:06 +0300	[thread overview]
Message-ID: <5de0fe153c337e379087239bcce3b48e97b07f0f.1556542564.git.alexander.turenko@tarantool.org> (raw)
In-Reply-To: <a1a8873a-b31b-b608-7912-e15c8ee41ce0@tarantool.org>

It is important to have testing jobs that build the project with both
-Werror and -O2 to keep the code clean. -O2 is needed, because some
compiler warnings are available only after extra analyzing passes that
are disabled with lesser optimization levels.

The first attempt to add -Werror for release testing jobs was made in
da505ee72883195121b7006bbd52bc66e57321f3 ('Add -Werror for CI (1.10
part)'), but it mistakely doesn't enable -O2 for RelWithDebInfoWError
build. It is possible to fix it in this way:

 | --- a/cmake/compiler.cmake
 | +++ b/cmake/compiler.cmake
 | @@ -113,10 +113,14 @@ set (CMAKE_C_FLAGS_DEBUG
 |      "${CMAKE_C_FLAGS_DEBUG} ${CC_DEBUG_OPT} -O0")
 |  set (CMAKE_C_FLAGS_RELWITHDEBINFO
 |      "${CMAKE_C_FLAGS_RELWITHDEBINFO} ${CC_DEBUG_OPT} -O2")
 | +set (CMAKE_C_FLAGS_RELWITHDEBINFOWERROR
 | +    "${CMAKE_C_FLAGS_RELWITHDEBINFOWERROR} ${CC_DEBUG_OPT} -O2")
 |  set (CMAKE_CXX_FLAGS_DEBUG
 |      "${CMAKE_CXX_FLAGS_DEBUG} ${CC_DEBUG_OPT} -O0")
 |  set (CMAKE_CXX_FLAGS_RELWITHDEBINFO
 |      "${CMAKE_CXX_FLAGS_RELWITHDEBINFO} ${CC_DEBUG_OPT} -O2")
 | +set (CMAKE_CXX_FLAGS_RELWITHDEBINFOWERROR
 | +    "${CMAKE_CXX_FLAGS_RELWITHDEBINFOWERROR} ${CC_DEBUG_OPT} -O2")
 |
 |  unset(CC_DEBUG_OPT)

However I think that a build type (and so `tarantool --version`) should
not show whether -Werror was passed or not. So I have added
ENABLE_WERROR CMake option for that. It can be set like so:

 | cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON

Enabled the option in testing Travis-CI jobs with the RelWithDebInfo
build type. Deploy jobs don't include it as before.

Fixed all -Wmaybe-uninitialized and -Wunused-result warnings. A few
notes about the fixes:

* net.box does not validate received data in general, so I don't add a
  check for autoincrement IDs too. Set the ID to INT64_MIN, because this
  value is less probably will appear here in a normal case and so is the
  best one to signal a user that something probably going wrongly.
* xrow_decode_*() functions could read uninitialized data from
  row->body[0].iov_base in xrow_on_decode_err() when printing a hex code
  for a row. It could be possible when the received msgpack was empty
  (row->bodycnt == 0), but there were expected keys (key_map != 0).
* getcwd() is marked with __attribute__((__warn_unused_result__)) in
  glibc, but the buffer filled by this call is not used anywhere and so
  just removed.
* Vinyl -Wmaybe-uninitialized warnings are false positive ones.

Added comments and quotes into .travis.yml to ease reading. Removed
"test" word from the CentOS 6 job name, because we don't run tests on
this distro (disabled in the RPM spec).

Fixes #4178.
---

This patch targets all currently supported long-term branches.

https://github.com/tarantool/tarantool/issues/4178
https://github.com/tarantool/tarantool/tree/Totktonada/gh-4178-set-right-flags-in-release-testing-jobs

 .travis.mk            |  4 ++--
 .travis.yml           | 45 +++++++++++++++++++++++--------------------
 cmake/compiler.cmake  | 16 ++++++++++-----
 src/box/lua/net_box.c |  2 +-
 src/box/sql/func.c    | 16 +++++++--------
 src/box/vy_cache.c    |  2 +-
 src/box/vy_mem.c      |  2 +-
 src/box/xrow.c        | 30 ++++++++++++++++++-----------
 src/lua/init.c        |  2 --
 9 files changed, 67 insertions(+), 52 deletions(-)

diff --git a/.travis.mk b/.travis.mk
index 247bb2a2d..973c6a7c7 100644
--- a/.travis.mk
+++ b/.travis.mk
@@ -40,7 +40,7 @@ deps_ubuntu:
 		lcov ruby
 
 test_ubuntu: deps_ubuntu
-	cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfoWError ${CMAKE_EXTRA_PARAMS}
+	cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS}
 	make -j8
 	cd test && /usr/bin/python test-run.py -j 1
 
@@ -51,7 +51,7 @@ deps_osx:
 	pip install -r test-run/requirements.txt
 
 test_osx: deps_osx
-	cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfoWError ${CMAKE_EXTRA_PARAMS}
+	cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS}
 	# Increase the maximum number of open file descriptors on macOS
 	sudo sysctl -w kern.maxfiles=20480 || :
 	sudo sysctl -w kern.maxfilesperproc=20480 || :
diff --git a/.travis.yml b/.travis.yml
index e30540837..6e79cf2fe 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -19,69 +19,72 @@ git:
 
 jobs:
     include:
-      - name: RelWithDebInfoWError build + test (Linux, gcc)
+      # Testing targets (just run tests on Debian Stretch or OS X).
+      - name: "RelWithDebInfo build + test (Linux, gcc)"
         env: TARGET=test
-      - name: RelWithDebInfoWError build + test (Linux, clang)
+      - name: "RelWithDebInfo build + test (Linux, clang)"
         env: TARGET=test
         compiler: clang
-      - name: RelWithDebInfoWError build + test (OS X Mojave 10.14)
+      - name: "RelWithDebInfo build + test (OS X Mojave 10.14)"
         env: TARGET=test
         os: osx
-      - name: Debug build + test + coverage (Linux, gcc)
+      - name: "Debug build + test + coverage (Linux, gcc)"
         env: TARGET=coverage
-      - name: RelWithDebInfoWError build + test (OS X High Sierra 10.13)
+      - name: "RelWithDebInfo build + test (OS X High Sierra 10.13)"
         env: TARGET=test
         os: osx
         osx_image: xcode9.4
         if: branch = "master"
-      - name: LTO build + test (Linux, gcc)
+      # Special targets (only LTO for now).
+      - name: "LTO build + test (Linux, gcc)"
         env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON
         if: branch = "master"
-      - name: LTO build + test (Linux, clang)
+      - name: "LTO build + test (Linux, clang)"
         env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON
         if: branch = "master"
         compiler: clang
-      - name: LTO build + test (OS X Mojave 10.14)
+      - name: "LTO build + test (OS X Mojave 10.14)"
         os: osx
         env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON
         if: branch = "master"
-      - name: Create and deploy tarball
+      # Deploy targets (they also catch distro-specific problems).
+      - name: "Create and deploy tarball"
         env: TARGET=source
         if: branch = "master"
-      - name: CentOS 6 build + test + deploy RPM
+      - name: "CentOS 6 build + deploy RPM"
         env: OS=el DIST=6
         if: branch = "master"
-      - name: CentOS 7 build + test + deploy RPM
+      - name: "CentOS 7 build + test + deploy RPM"
         env: OS=el DIST=7
         if: branch = "master"
-      - name: Fedora 28 build + test + deploy RPM
+      - name: "Fedora 28 build + test + deploy RPM"
         env: OS=fedora DIST=28
         if: branch = "master"
-      - name: Fedora 29 build + test + deploy RPM
+      - name: "Fedora 29 build + test + deploy RPM"
         env: OS=fedora DIST=29
         if: branch = "master"
-      - name: Ubuntu Trusty (14.04) build + deploy DEB
+      - name: "Ubuntu Trusty (14.04) build + deploy DEB"
         env: OS=ubuntu DIST=trusty
         if: branch = "master"
-      - name: Ubuntu Xenial (16.04) build + deploy DEB
+      - name: "Ubuntu Xenial (16.04) build + deploy DEB"
         env: OS=ubuntu DIST=xenial
         if: branch = "master"
-      - name: Ubuntu Bionic (18.04) build + deploy DEB
+      - name: "Ubuntu Bionic (18.04) build + deploy DEB"
         env: OS=ubuntu DIST=bionic
         if: branch = "master"
-      - name: Ubuntu Cosmic (18.10) build + deploy DEB
+      - name: "Ubuntu Cosmic (18.10) build + deploy DEB"
         env: OS=ubuntu DIST=cosmic
         if: branch = "master"
-      - name: Ubuntu Disco (19.04) build + deploy DEB
+      - name: "Ubuntu Disco (19.04) build + deploy DEB"
         env: OS=ubuntu DIST=disco
         if: branch = "master"
-      - name: Debian Jessie (8) build + deploy DEB
+      - name: "Debian Jessie (8) build + deploy DEB"
         env: OS=debian DIST=jessie
         if: branch = "master"
-      - name: Debian Stretch (9) build + deploy DEB
+      - name: "Debian Stretch (9) build + deploy DEB"
         env: OS=debian DIST=stretch
         if: branch = "master"
-      - name: Debian Buster (10) build + deploy DEB
+      - name: "Debian Buster (10) build + deploy DEB"
         env: OS=debian DIST=buster
         if: branch = "master"
 
diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
index 4062d13ec..887485c80 100644
--- a/cmake/compiler.cmake
+++ b/cmake/compiler.cmake
@@ -223,6 +223,8 @@ if (NOT CMAKE_BUILD_TYPE STREQUAL "Debug")
     add_definitions("-DNDEBUG" "-DNVALGRIND")
 endif()
 
+option(ENABLE_WERROR "Make all compiler warnings into errors" OFF)
+
 macro(enable_tnt_compile_flags)
     # Tarantool code is written in GNU C dialect.
     # Additionally, compile it with more strict flags than the rest
@@ -286,11 +288,15 @@ macro(enable_tnt_compile_flags)
     add_definitions("-D__STDC_LIMIT_MACROS=1")
     add_definitions("-D__STDC_CONSTANT_MACROS=1")
 
-    # Only add -Werror if it's a Debug or RelWithDebInfoWError build, done by
-    # developers. Release builds should not cause extra trouble.
-    if (((${CMAKE_BUILD_TYPE} STREQUAL "Debug") OR
-        (${CMAKE_BUILD_TYPE} STREQUAL "RelWithDebInfoWError")) AND
-        HAVE_STD_C11 AND HAVE_STD_CXX11)
+    # Only add -Werror if it's a debug build, done by developers.
+    # Release builds should not cause extra trouble.
+    if ((${CMAKE_BUILD_TYPE} STREQUAL "Debug")
+        AND HAVE_STD_C11 AND HAVE_STD_CXX11)
+        add_compile_flags("C;CXX" "-Werror")
+    endif()
+
+    # Add -Werror if it is requested explicitly.
+    if (ENABLE_WERROR)
         add_compile_flags("C;CXX" "-Werror")
     endif()
 endmacro(enable_tnt_compile_flags)
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 2dfc4fc9d..7484a86bf 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -694,7 +694,7 @@ netbox_decode_sql_info(struct lua_State *L, const char **data)
 		assert(count > 0);
 		lua_createtable(L, 0, count);
 		for (uint32_t j = 0; j < count; ++j) {
-			int64_t id;
+			int64_t id = INT64_MIN;
 			mp_read_int64(data, &id);
 			luaL_pushint64(L, id);
 			lua_rawseti(L, -2, j + 1);
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index f3790ba01..bb7405e68 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -758,15 +758,15 @@ sql_utf8_pattern_compare(const char *pattern,
 			       SQL_END_OF_STRING) {
 				if (c == SQL_INVALID_UTF8_SYMBOL)
 					return INVALID_PATTERN;
-				if (c != MATCH_ALL_WILDCARD &&
-				    c != MATCH_ONE_WILDCARD)
+				if (c == MATCH_ONE_WILDCARD) {
+					c2 = Utf8Read(string, string_end);
+					if (c2 == SQL_INVALID_UTF8_SYMBOL)
+						return NO_MATCH;
+					if (c2 == SQL_END_OF_STRING)
+						return NO_WILDCARD_MATCH;
+				} else if (c != MATCH_ALL_WILDCARD) {
 					break;
-				if (c == MATCH_ONE_WILDCARD &&
-				    (c2 = Utf8Read(string, string_end)) ==
-				    SQL_END_OF_STRING)
-					return NO_WILDCARD_MATCH;
-				if (c2 == SQL_INVALID_UTF8_SYMBOL)
-					return NO_MATCH;
+				}
 			}
 			/*
 			 * "%" at the end of the pattern matches.
diff --git a/src/box/vy_cache.c b/src/box/vy_cache.c
index be11b986f..7007d0e35 100644
--- a/src/box/vy_cache.c
+++ b/src/box/vy_cache.c
@@ -661,7 +661,7 @@ vy_cache_iterator_seek(struct vy_cache_iterator *itr, struct vy_entry last)
 				ITER_GT : ITER_LT;
 	}
 
-	bool exact;
+	bool exact = false;
 	if (!vy_stmt_is_empty_key(key.stmt)) {
 		itr->curr_pos = iterator_type == ITER_EQ ||
 				iterator_type == ITER_GE ||
diff --git a/src/box/vy_mem.c b/src/box/vy_mem.c
index 9513d5b80..a4fae26e2 100644
--- a/src/box/vy_mem.c
+++ b/src/box/vy_mem.c
@@ -392,7 +392,7 @@ vy_mem_iterator_seek(struct vy_mem_iterator *itr, struct vy_entry last)
 				ITER_GT : ITER_LT;
 	}
 
-	bool exact;
+	bool exact = false;
 	struct vy_mem_tree_key tree_key;
 	tree_key.entry = key;
 	/* (lsn == INT64_MAX - 1) means that lsn is ignored in comparison */
diff --git a/src/box/xrow.c b/src/box/xrow.c
index aaed84e38..a3c9b3218 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -509,9 +509,12 @@ iproto_write_error(int fd, const struct error *e, uint32_t schema_version,
 			     schema_version, sizeof(body) + msg_len);
 
 	body.v_data_len = mp_bswap_u32(msg_len);
-	(void) write(fd, header, sizeof(header));
-	(void) write(fd, &body, sizeof(body));
-	(void) write(fd, e->errmsg, msg_len);
+
+	ssize_t unused;
+	unused = write(fd, header, sizeof(header));
+	unused = write(fd, &body, sizeof(body));
+	unused = write(fd, e->errmsg, msg_len);
+	(void) unused;
 }
 
 int
@@ -624,12 +627,15 @@ xrow_decode_dml(struct xrow_header *row, struct request *request,
 	request->header = row;
 	request->type = row->type;
 
+	const char *start = NULL;
+	const char *end = NULL;
+
 	if (row->bodycnt == 0)
 		goto done;
 
 	assert(row->bodycnt == 1);
-	const char *data = (const char *) row->body[0].iov_base;
-	const char *end = data + row->body[0].iov_len;
+	const char *data = start = (const char *) row->body[0].iov_base;
+	end = data + row->body[0].iov_len;
 	assert((end - data) > 0);
 
 	if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) {
@@ -701,8 +707,8 @@ error:
 done:
 	if (key_map) {
 		enum iproto_key key = (enum iproto_key) bit_ctz_u64(key_map);
-		xrow_on_decode_err(row->body[0].iov_base, end,
-				   ER_MISSING_REQUEST_FIELD, iproto_key_name(key));
+		xrow_on_decode_err(start, end, ER_MISSING_REQUEST_FIELD,
+				   iproto_key_name(key));
 		return -1;
 	}
 	return 0;
@@ -1071,12 +1077,15 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
 	ballot->is_ro = false;
 	vclock_create(&ballot->vclock);
 
+	const char *start = NULL;
+	const char *end = NULL;
+
 	if (row->bodycnt == 0)
 		goto err;
 	assert(row->bodycnt == 1);
 
-	const char *data = (const char *) row->body[0].iov_base;
-	const char *end = data + row->body[0].iov_len;
+	const char *data = start = (const char *) row->body[0].iov_base;
+	end = data + row->body[0].iov_len;
 	const char *tmp = data;
 	if (mp_check(&tmp, end) != 0 || mp_typeof(*data) != MP_MAP)
 		goto err;
@@ -1124,8 +1133,7 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
 	}
 	return 0;
 err:
-	xrow_on_decode_err(row->body[0].iov_base, end, ER_INVALID_MSGPACK,
-			   "packet body");
+	xrow_on_decode_err(start, end, ER_INVALID_MSGPACK, "packet body");
 	return -1;
 }
 
diff --git a/src/lua/init.c b/src/lua/init.c
index d18c8af94..be164bdfb 100644
--- a/src/lua/init.c
+++ b/src/lua/init.c
@@ -318,8 +318,6 @@ static void
 tarantool_lua_setpaths(struct lua_State *L)
 {
 	const char *home = getenv("HOME");
-	char cwd[PATH_MAX] = {'\0'};
-	getcwd(cwd, sizeof(cwd));
 	lua_getglobal(L, "package");
 	int top = lua_gettop(L);
 
-- 
2.21.0

  reply	other threads:[~2019-04-29 12:57 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-26  1:29 [tarantool-patches] [PATCH] " Alexander Turenko
2019-04-26 10:18 ` [tarantool-patches] " Vladislav Shpilevoy
2019-04-28  5:40   ` Alexander Turenko
2019-04-28  8:54     ` Vladislav Shpilevoy
2019-04-28  9:11       ` Vladislav Shpilevoy
2019-04-28 21:38         ` Alexander Turenko
2019-04-28 21:35       ` Alexander Turenko
2019-04-29 10:07         ` Vladislav Shpilevoy
2019-04-29 12:57           ` Alexander Turenko [this message]
2019-04-29 17:09             ` [PATCH v2] " Vladimir Davydov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5de0fe153c337e379087239bcce3b48e97b07f0f.1556542564.git.alexander.turenko@tarantool.org \
    --to=alexander.turenko@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=v.shpilevoy@tarantool.org \
    --cc=vdavydov.dev@gmail.com \
    --subject='Re: [PATCH v2] travis-ci: set right flags in release testing jobs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox