[PATCH] Fixed `-Wunused-lambda-capture` and some other warnings to enable building `libseastar` with clang (tested with 7.0.1).

[Pavel Solodovnikov]

Also raise `lz4` version requirement from 1.7.3 to 1.8.1 since it includes
fixes necessary for building with clang (e.g. https://github.com/lz4/lz4/pull/393).
---
include/seastar/net/inet_address.hh | 2 ++
src/core/reactor.cc | 2 +-
src/net/dns.cc | 8 --------
src/net/inet_address.cc | 9 +++++++++
CMakeLists.txt | 2 +-
cmake/SeastarConfig.cmake.in | 2 +-
cooking_recipe.cmake | 4 ++--
pkgconfig/seastar.pc.in | 2 +-
8 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/include/seastar/net/inet_address.hh b/include/seastar/net/inet_address.hh
index 79798601..4b00b784 100644
--- a/include/seastar/net/inet_address.hh
+++ b/include/seastar/net/inet_address.hh
@@ -29,6 +29,7 @@

#include <seastar/core/future.hh>
#include <seastar/core/sstring.hh>
+#include <seastar/util/std-compat.hh>

namespace seastar {
namespace net {
@@ -105,6 +106,7 @@ class inet_address {

std::ostream& operator<<(std::ostream&, const inet_address&);
std::ostream& operator<<(std::ostream&, const inet_address::family&);
+std::ostream& operator<<(std::ostream& os, const compat::optional<inet_address::family>& f);

}
}
diff --git a/src/core/reactor.cc b/src/core/reactor.cc
index 9368e4aa..63936d3c 100644
--- a/src/core/reactor.cc
+++ b/src/core/reactor.cc
@@ -3011,7 +3011,7 @@ reactor::file_accessible(sstring pathname, access_flags flags) {
auto aflags = std::underlying_type_t<access_flags>(flags);
auto ret = ::access(pathname.c_str(), aflags);
return wrap_syscall(ret);
- }).then([this, pathname, flags] (syscall_result<int> sr) {
+ }).then([pathname, flags] (syscall_result<int> sr) {
if (sr.result < 0) {
if ((sr.error == ENOENT && flags == access_flags::exists) ||
(sr.error == EACCES && flags != access_flags::exists)) {
diff --git a/src/net/dns.cc b/src/net/dns.cc
index 9a7d0409..6ab3c434 100644
--- a/src/net/dns.cc
+++ b/src/net/dns.cc
@@ -38,14 +38,6 @@

namespace seastar {

-std::ostream& operator<<(std::ostream& os, const compat::optional<net::inet_address::family>& f) {
- if (f) {
- return os << *f;
- } else {
- return os << "ANY";
- }
-}
-
static logger dns_log("dns_resolver");

class ares_error_category : public std::error_category {
diff --git a/src/net/inet_address.cc b/src/net/inet_address.cc
index 6a474ec7..b405afbb 100644
--- a/src/net/inet_address.cc
+++ b/src/net/inet_address.cc
@@ -292,6 +292,15 @@ std::ostream& seastar::net::operator<<(std::ostream& os, const inet_address::fam
return os;
}

+std::ostream& seastar::net::operator<<(std::ostream& os,
+ const seastar::compat::optional<inet_address::family>& f) {
+ if (f) {
+ return os << *f;
+ } else {
+ return os << "ANY";
+ }
+}
+
std::ostream& seastar::operator<<(std::ostream& os, const socket_address& a) {
auto addr = a.addr();
// CMH. maybe skip brackets for ipv4-mapped
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 40c0472c..a902b82d 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -234,7 +234,7 @@ find_package (cryptopp 5.6.5 REQUIRED)
# No version information published.
find_package (dpdk)
find_package (fmt 5.0.0 REQUIRED)
-find_package (lz4 1.7.3 REQUIRED)
+find_package (lz4 1.8.1 REQUIRED)

##
## Private and private/public dependencies.
diff --git a/cmake/SeastarConfig.cmake.in b/cmake/SeastarConfig.cmake.in
index 19221871..1d045ea9 100644
--- a/cmake/SeastarConfig.cmake.in
+++ b/cmake/SeastarConfig.cmake.in
@@ -42,7 +42,7 @@ find_package (c-ares 1.13 REQUIRED MODULE)
find_package (cryptopp 5.6.5 REQUIRED)
find_package (dpdk)
find_package (fmt 5.0.0 REQUIRED)
-find_package (lz4 1.7.3 REQUIRED)
+find_package (lz4 1.8.1 REQUIRED)

#
# Private and private/public dependencies.
diff --git a/cooking_recipe.cmake b/cooking_recipe.cmake
index 88d4e602..b9e20c2a 100644
--- a/cooking_recipe.cmake
+++ b/cooking_recipe.cmake
@@ -287,8 +287,8 @@ cooking_ingredient (fmt

cooking_ingredient (lz4
EXTERNAL_PROJECT_ARGS
- URL https://github.com/lz4/lz4/archive/v1.8.0.tar.gz
- URL_MD5 6247bf0e955899969d1600ff34baed6b
+ URL https://github.com/lz4/lz4/archive/v1.8.1.2.tar.gz
+ URL_MD5 343538e69ba752a386c669b1a28111e2
# This is upsetting.
BUILD_IN_SOURCE ON
CONFIGURE_COMMAND <DISABLE>
diff --git a/pkgconfig/seastar.pc.in b/pkgconfig/seastar.pc.in
index db000748..8b81307c 100644
--- a/pkgconfig/seastar.pc.in
+++ b/pkgconfig/seastar.pc.in
@@ -38,7 +38,7 @@ numactl_libs=$<JOIN:@numactl_LIBRARIES@, >
seastar_cflags=${seastar_include_flags} $<JOIN:$<TARGET_PROPERTY:seastar,INTERFACE_COMPILE_OPTIONS>, > -D$<JOIN:$<TARGET_PROPERTY:seastar,INTERFACE_COMPILE_DEFINITIONS>, -D>
seastar_libs=${libdir}/$<TARGET_FILE_NAME:seastar> @Seastar_SPLIT_DWARF_FLAG@

-Requires: liblz4 >= 1.7.3
+Requires: liblz4 >= 1.8.1
Requires.private: gnutls >= 3.2.26, protobuf >= 2.5.0, hwloc >= 1.11.2, yaml-cpp >= 0.5.1
Conflicts:
Cflags: ${boost_cflags} ${c_ares_cflags} ${cryptopp_cflags} ${fmt_cflags} ${lksctp_tools_cflags} ${numactl_cflags} ${seastar_cflags}
--
2.17.1

[Nadav Har'El]

On Wed, Apr 3, 2019 at 10:18 AM Pavel Solodovnikov <pavel.al.s...@gmail.com> wrote:

Also raise `lz4` version requirement from 1.7.3 to 1.8.1 since it includes

fixes necessary for building with clang (e.g. https://github.com/lz4/lz4/pull/393).

 

I am not strongly opposed to this increased requirement (1.8.1 was released a bit more than a year ago),

but it makes me wonder, yet again, whether we should be looking at version numbers or at specific

features (as was customary in the autoconf world). In this case, we could have checked the buggy

feature against the current compiler, and allow gcc+1.7.3 to work, without forcing someone who

doesn't even have clang, to upgrade to 1.8.1.

 

---
 include/seastar/net/inet_address.hh | 2 ++
 src/core/reactor.cc                 | 2 +-
 src/net/dns.cc                      | 8 --------
 src/net/inet_address.cc             | 9 +++++++++
 CMakeLists.txt                      | 2 +-
 cmake/SeastarConfig.cmake.in        | 2 +-
 cooking_recipe.cmake                | 4 ++--
 pkgconfig/seastar.pc.in             | 2 +-
 8 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/include/seastar/net/inet_address.hh b/include/seastar/net/inet_address.hh
index 79798601..4b00b784 100644
--- a/include/seastar/net/inet_address.hh
+++ b/include/seastar/net/inet_address.hh
@@ -29,6 +29,7 @@

 #include <seastar/core/future.hh>
 #include <seastar/core/sstring.hh>
+#include <seastar/util/std-compat.hh>

 namespace seastar {
 namespace net {
@@ -105,6 +106,7 @@ class inet_address {

 std::ostream& operator<<(std::ostream&, const inet_address&);
 std::ostream& operator<<(std::ostream&, const inet_address::family&);
+std::ostream& operator<<(std::ostream& os, const compat::optional<inet_address::family>& f);

I didn't understand why this movement of operator<< from one file to the other is part of this patch.

Did clang have a particular problem with this location?

If not clang-specific, I think this movement should be discussed separately. In particular, is there

any API in inet_address.hh which requires an optional<inet_address::family>? If not, this header

file rightly does not concern itself with printing it.

I just realized we have this list of dependencies twice, in CMakeLists.txt andcmake/SeastarConfig.cmake.in.

Jesse, could we somehow avoid this duplication?

 #
 # Private and private/public dependencies.
diff --git a/cooking_recipe.cmake b/cooking_recipe.cmake
index 88d4e602..b9e20c2a 100644
--- a/cooking_recipe.cmake
+++ b/cooking_recipe.cmake
@@ -287,8 +287,8 @@ cooking_ingredient (fmt

 cooking_ingredient (lz4
   EXTERNAL_PROJECT_ARGS
-    URL https://github.com/lz4/lz4/archive/v1.8.0.tar.gz
-    URL_MD5 6247bf0e955899969d1600ff34baed6b
+    URL https://github.com/lz4/lz4/archive/v1.8.1.2.tar.gz
+    URL_MD5 343538e69ba752a386c669b1a28111e2

Looks ok, but note that 1.8.3 is already out. I don't know what is our policy here, do we go with the latest version, or the oldest allowed version?

     # This is upsetting.
     BUILD_IN_SOURCE ON
     CONFIGURE_COMMAND <DISABLE>
diff --git a/pkgconfig/seastar.pc.in b/pkgconfig/seastar.pc.in
index db000748..8b81307c 100644
--- a/pkgconfig/seastar.pc.in
+++ b/pkgconfig/seastar.pc.in
@@ -38,7 +38,7 @@ numactl_libs=$<JOIN:@numactl_LIBRARIES@, >
 seastar_cflags=${seastar_include_flags} $<JOIN:$<TARGET_PROPERTY:seastar,INTERFACE_COMPILE_OPTIONS>, > -D$<JOIN:$<TARGET_PROPERTY:seastar,INTERFACE_COMPILE_DEFINITIONS>, -D>
 seastar_libs=${libdir}/$<TARGET_FILE_NAME:seastar> @Seastar_SPLIT_DWARF_FLAG@

-Requires: liblz4 >= 1.7.3
+Requires: liblz4 >= 1.8.1

Sad, this is the *third* place we have this specification :-(

Not related to your patch, of course, but it would have been nice to reduce this duplication.

 

 Requires.private: gnutls >= 3.2.26, protobuf >= 2.5.0, hwloc >= 1.11.2, yaml-cpp >= 0.5.1
 Conflicts:
 Cflags: ${boost_cflags} ${c_ares_cflags} ${cryptopp_cflags} ${fmt_cflags} ${lksctp_tools_cflags} ${numactl_cflags} ${seastar_cflags}
--
2.17.1

--
You received this message because you are subscribed to the Google Groups "seastar-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to seastar-dev...@googlegroups.com.
To post to this group, send email to seast...@googlegroups.com.
Visit this group at https://groups.google.com/group/seastar-dev.
To view this discussion on the web visit https://groups.google.com/d/msgid/seastar-dev/20190403071805.14066-1-pavel.al.solodovnikov%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

[Pavel Solodovnikov]

Thanks for the reply, Nadav,

I am not strongly opposed to this increased requirement (1.8.1 was released a bit more than a year ago), 

but it makes me wonder, yet again, whether we should be looking at version numbers or at specific

features (as was customary in the autoconf world). In this case, we could have checked the buggy

feature against the current compiler, and allow gcc+1.7.3 to work, without forcing someone who

doesn't even have clang, to upgrade to 1.8.1.

I have found some inconsistencies in lz4 versioning here and there too and unfortunately, I don't know what is right or not: build scripts specify minimum version requirement as 1.7.3, though `cooking` recipe uses `1.8.0` already. Also, I wonder why there are several separate build systems being used (pkgconfig, CMake, cooking, configure.py)? What kind of workflow is used in production builds? IMHO these should be reduced to using just one (production build variant) and expand it with functionality needed for other types of workflow and deployment (local development, etc.). This would eliminate copy-paste and additional duplicating boilerplate.

 

I didn't understand why this movement of operator<< from one file to the other is part of this patch.

Did clang have a particular problem with this location?

 

 Yes, this one is clang specific. Specifically this is used to get around the following compilation error:

FAILED: CMakeFiles/seastar.dir/src/net/dns.cc.o  
/usr/bin/c++  -DSEASTAR_ASAN_ENABLED -DSEASTAR_DEBUG -DSEASTAR_DEBUG_SHARED_PTR -DSEASTAR_DEFAULT_ALLOCATOR -DSEASTAR_HAVE_ASAN_FIBER_SUPPORT -DSEASTAR_HAVE_HWLOC -DSEASTAR_HAVE_NUMA -DSEASTAR_NO_EXCEPTION_HACK -DSEA
STAR_SHUFFLE_TASK_QUEUE -DSEASTAR_THREAD_STACK_GUARDS -DSEASTAR_TYPE_ERASE_MORE -I../include -Igen/include -I../src -Igen/src -isystem _cooking/installed/include -g   -std=gnu++17 -U_FORTIFY_SOURCE -fvisibility=hidde
n -UNDEBUG -Wall -Werror -Wno-error=deprecated-declarations -gz -fsanitize=address -fsanitize=undefined -fno-sanitize=vptr -MD -MT CMakeFiles/seastar.dir/src/net/dns.cc.o -MF CMakeFiles/seastar.dir/src/net/dns.cc.o.d
-o CMakeFiles/seastar.dir/src/net/dns.cc.o -c ../src/net/dns.cc
In file included from ../src/net/dns.cc:28:
In file included from ../include/seastar/net/ip.hh:36:
In file included from ../include/seastar/net/arp.hh:25:
In file included from ../include/seastar/net/net.hh:24:
In file included from ../include/seastar/core/reactor.hh:76:
../include/seastar/util/log.hh:91:20: error: call to function 'operator<<' that is neither visible in the template definition nor found by argument-dependent lookup
               os << *static_cast<const std::remove_reference_t<Arg>*>(object);
                  ^
../include/seastar/util/log.hh:314:8: note: in instantiation of function template specialization 'seastar::logger::stringer_for<const std::experimental::fundamentals_v1::optional<seastar::net::inet_address::family> &
>' requested here
   } (stringer_for<Args>(std::forward<Args>(args))...);
      ^
../include/seastar/util/log.hh:124:17: note: in instantiation of function template specialization 'seastar::logger::do_log<const seastar::basic_sstring<char, unsigned int, 15, true> &, const std::experimental::fundam
entals_v1::optional<seastar::net::inet_address::family> &>' requested here
               do_log(level, fmt, args...);
               ^
../include/seastar/util/log.hh:181:9: note: in instantiation of function template specialization 'seastar::logger::log<seastar::basic_sstring<char, unsigned int, 15, true>, std::experimental::fundamentals_v1::optiona
l<seastar::net::inet_address::family> >' requested here
       log(log_level::debug, fmt, std::forward<Args>(args)...);
       ^
../src/net/dns.cc:228:17: note: in instantiation of function template specialization 'seastar::logger::debug<seastar::basic_sstring<char, unsigned int, 15, true> &, std::experimental::fundamentals_v1::optional<seasta
r::net::inet_address::family> &>' requested here
       dns_log.debug("Query name {} ({})", name, family);
               ^
../src/net/dns.cc:41:15: note: 'operator<<' should be declared prior to the call site or in namespace 'seastar::net'

std::ostream& operator<<(std::ostream& os, const compat::optional<net::inet_address::family>& f) {

             ^
1 error generated.

I agree that exposing additional operator<< overload is not desirable, especially because <std-compat.hh> needs to be brought in scope as well. If extra overload is an issue, this could be fixed the other way, of course. 

Looks ok, but note that 1.8.3 is already out. I don't know what is our policy here, do we go with the latest version, or the oldest allowed version?

This depends on what the policy for updating 3rd party dependencies is, as you have already noticed, I would like to know that too.

Sad, this is the *third* place we have this specification :-(

Not related to your patch, of course, but it would have been nice to reduce this duplication.

I would be happy to eliminate copy-paste and refactor build workflow, but I would need some guidance (as already mentioned above) to ensure that refactoring plays nicely with every kind of existing workflows and meets everyone's needs.

With regards,

Pavel

[dejan@scylladb.com]

On Wednesday, April 3, 2019 at 5:14:21 AM UTC-4, Nadav Har'El wrote:

Could it be that one is for building Seastar, while the others are for using installed Seastar in another project (via find_package(Seastar) or pkg-config --static $seastar_dir/build/release/seastar.pc)?

https://github.com/scylladb/seastar#using-seastar-from-its-build-directory-without-installation

 

To unsubscribe from this group and stop receiving emails from it, send an email to seast...@googlegroups.com.

[Jesse Haber-Kucharsky]

That's correct. 

To unsubscribe from this group and stop receiving emails from it, send an email to seastar-dev...@googlegroups.com.

To post to this group, send email to seast...@googlegroups.com.
Visit this group at https://groups.google.com/group/seastar-dev.

To view this discussion on the web visit https://groups.google.com/d/msgid/seastar-dev/e1724bd4-bfed-428e-9a11-456589fa757e%40googlegroups.com.

[Jesse Haber-Kucharsky]

We could populate the pkgconfig versions from CMake during configure time, but the versions in both CMake files and in `cooking_recipe.cmake` are required:

- The version specification in `CMakeLists.txt` is checked at the time of configuration when building the project

- The version specification in `cmake/SeastarConfig.cmake.in` is for checking the version when Seastar is used as part of another  project (ie, `find_package (SeastarA)`)

- The versions in `cooking_recipe.cmake` are used for setting up a local development environment.

We previously decided that the first two of these will reflect minimums (as best as we can guess them), while the last (for cmake-cooking) are supposed to reflect a reasonably up-to-date environment.

[Pavel Solodovnikov]

Hello Jesse,

- The version specification in `CMakeLists.txt` is checked at the time of configuration when building the project

- The version specification in `cmake/SeastarConfig.cmake.in` is for checking the version when Seastar is used as part of another  project (ie, `find_package (SeastarA)`) 

Dependencies shared across multiple CMake files could be wrapped into a separate `.cmake` script, which does

series of `find_package(...)` calls (or alternatively populates some variable with the dependencies list), this way duplicates are gone.

We previously decided that the first two of these will reflect minimums (as best as we can guess them), while the last (for cmake-cooking) are supposed to reflect a reasonably up-to-date environment.

So I think I could leave a note in both README and in the header of `cooking_recipe.cmake` to

document this policy, what do you think?

ср, 3 апр. 2019 г. в 21:08, Jesse Haber-Kucharsky <jhab...@scylladb.com>:

[Jesse Haber-Kucharsky]

On Thu, Apr 4, 2019 at 4:32 AM Pavel Solodovnikov <pavel.al.s...@gmail.com> wrote:

Hello Jesse,

- The version specification in `CMakeLists.txt` is checked at the time of configuration when building the project

- The version specification in `cmake/SeastarConfig.cmake.in` is for checking the version when Seastar is used as part of another  project (ie, `find_package (SeastarA)`) 

Dependencies shared across multiple CMake files could be wrapped into a separate `.cmake` script, which does

series of `find_package(...)` calls (or alternatively populates some variable with the dependencies list), this way duplicates are gone.

I suppose we could define a bunch of variables like

    set (Seastar_fmt_minimum_version 3.14159)

and then `INCLUDE` it.

This would be a separate patch than this one, of course.

 

We previously decided that the first two of these will reflect minimums (as best as we can guess them), while the last (for cmake-cooking) are supposed to reflect a reasonably up-to-date environment.

So I think I could leave a note in both README and in the header of `cooking_recipe.cmake` to

document this policy, what do you think?

That would be excellent. Thanks!

[Dejan Mircevski]

On Fri, Apr 5, 2019 at 3:07 PM Jesse Haber-Kucharsky <jhab...@scylladb.com> wrote:

On Thu, Apr 4, 2019 at 4:32 AM Pavel Solodovnikov <pavel.al.s...@gmail.com> wrote:

Hello Jesse,

- The version specification in `CMakeLists.txt` is checked at the time of configuration when building the project

- The version specification in `cmake/SeastarConfig.cmake.in` is for checking the version when Seastar is used as part of another  project (ie, `find_package (SeastarA)`) 

Dependencies shared across multiple CMake files could be wrapped into a separate `.cmake` script, which does

series of `find_package(...)` calls (or alternatively populates some variable with the dependencies list), this way duplicates are gone.

I suppose we could define a bunch of variables like

    set (Seastar_fmt_minimum_version 3.14159)

and then `INCLUDE` it.

Actually, I'm hoping for a single list of dependencies that the parent file can iterate through.  When we add a new dependency tomorrow, we should only have to update the INCLUDEd file.  Each list entry would (somehow) provide both the package name and any version info we need.

 

You received this message because you are subscribed to a topic in the Google Groups "seastar-dev" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/seastar-dev/-uJLib1jKMU/unsubscribe.
To unsubscribe from this group and all its topics, send an email to seastar-dev...@googlegroups.com.

To post to this group, send email to seast...@googlegroups.com.
Visit this group at https://groups.google.com/group/seastar-dev.

To view this discussion on the web visit https://groups.google.com/d/msgid/seastar-dev/CALNeFX-0-f0yybKqakx4sadSSC3%2B-u4U%3D4cjbf9A29V3EV4bWw%40mail.gmail.com.

[Jesse Haber-Kucharsky]

On Fri, Apr 5, 2019 at 5:15 PM Dejan Mircevski <de...@scylladb.com> wrote:

On Fri, Apr 5, 2019 at 3:07 PM Jesse Haber-Kucharsky <jhab...@scylladb.com> wrote:

On Thu, Apr 4, 2019 at 4:32 AM Pavel Solodovnikov <pavel.al.s...@gmail.com> wrote:

Hello Jesse,

- The version specification in `CMakeLists.txt` is checked at the time of configuration when building the project

- The version specification in `cmake/SeastarConfig.cmake.in` is for checking the version when Seastar is used as part of another  project (ie, `find_package (SeastarA)`) 

Dependencies shared across multiple CMake files could be wrapped into a separate `.cmake` script, which does

series of `find_package(...)` calls (or alternatively populates some variable with the dependencies list), this way duplicates are gone.

I suppose we could define a bunch of variables like

    set (Seastar_fmt_minimum_version 3.14159)

and then `INCLUDE` it.

Actually, I'm hoping for a single list of dependencies that the parent file can iterate through.  When we add a new dependency tomorrow, we should only have to update the INCLUDEd file.  Each list entry would (somehow) provide both the package name and any version info we need.

I agree that this would be a better approach in general. Do keep in mind that some dependencies of `CMakeLists.txt` are not present in `SeastarConfig.cmake.in` since they are not necessary for "consuming" Seastar.

[Pavel Solodovnikov]

Actually, I'm hoping for a single list of dependencies that the parent file can iterate through.  When we add a new dependency tomorrow, we should only have to update the INCLUDEd file.  Each list entry would (somehow) provide both the package name and any version info we need.

 

Thanks for all the suggestions, I absolutely agree, though this is subject for another patch, of course.

пн, 8 апр. 2019 г. в 15:42, Jesse Haber-Kucharsky <jhab...@scylladb.com>:

[Avi Kivity]

Can you send the warning fixes separately? Let's commit what we can first.

[Pavel Solodovnikov]

Can you send the warning fixes separately? Let's commit what we can first.

Ok, I will accumulate the warning and compilation error fixes in another patch.

пн, 8 апр. 2019 г. в 18:25, Avi Kivity <a...@scylladb.com>: