[PATCH] Deduplicate Seastar dependencies management in CMake scripts

[Pavel Solodovnikov]

`cmake/SeastarDependencies.cmake` module is introduced.
It contains `seastar_find_dependencies` macro which iterates through
dependencies specification and calls `find_package` on each parsed entry.

Main `CMakeLists.txt` and `cmake/SeastarConfig.cmake.in` use the new module
instead of listing and finding dependencies by itself.
---
CMakeLists.txt | 28 +------
cmake/SeastarConfig.cmake.in | 25 +-----
cmake/SeastarDependencies.cmake | 133 ++++++++++++++++++++++++++++++++
3 files changed, 138 insertions(+), 48 deletions(-)
create mode 100644 cmake/SeastarDependencies.cmake

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 40c0472c..cb476a01 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -229,33 +229,11 @@ find_package (Boost 1.64.0 REQUIRED
thread
OPTIONAL_COMPONENTS unit_test_framework)

-find_package (c-ares 1.13.0 REQUIRED MODULE)
-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)
+include (SeastarDependencies)
+seastar_find_dependencies ()

-##
-## Private and private/public dependencies.
-##
-
-find_package (Concepts)
-find_package (GnuTLS 3.3.26 REQUIRED)
-find_package (LinuxMembarrier)
-find_package (Protobuf 2.5.0 REQUIRED)
-find_package (Sanitizers)
-find_package (StdAtomic REQUIRED)
-set (StdFilesystem_CXX_DIALECT ${Seastar_CXX_DIALECT})
-find_package (StdFilesystem REQUIRED)
-find_package (hwloc 1.11.2)
-# No version information published.
-find_package (lksctp-tools REQUIRED)
-# No version information published.
-find_package (numactl)
+# Private build dependencies not visible to consumers
find_package (ragel 6.10 REQUIRED)
-find_package (rt REQUIRED)
-find_package (yaml-cpp 0.5.1 REQUIRED)

#
# Code generation helpers.
diff --git a/cmake/SeastarConfig.cmake.in b/cmake/SeastarConfig.cmake.in
index 19221871..2cba04ba 100644
--- a/cmake/SeastarConfig.cmake.in
+++ b/cmake/SeastarConfig.cmake.in
@@ -38,29 +38,8 @@ find_package (Boost
thread
unit_test_framework)

-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)
-
-#
-# Private and private/public dependencies.
-#
-
-find_package (Concepts)
-find_package (GnuTLS 3.3.26 REQUIRED)
-find_package (LinuxMembarrier)
-find_package (Protobuf 2.5.0 REQUIRED)
-find_package (Sanitizers)
-find_package (StdAtomic REQUIRED)
-set (StdFilesystem_CXX_DIALECT "@Seastar_CXX_DIALECT@")
-find_package (StdFilesystem REQUIRED)
-find_package (hwloc 1.11.2)
-find_package (lksctp-tools)
-find_package (numactl)
-find_package (rt REQUIRED)
-find_package (yaml-cpp 0.5.1 REQUIRED)
+include (SeastarDependencies)
+seastar_find_dependencies ()

if (NOT TARGET Seastar::seastar)
include ("${CMAKE_CURRENT_LIST_DIR}/SeastarTargets.cmake")
diff --git a/cmake/SeastarDependencies.cmake b/cmake/SeastarDependencies.cmake
new file mode 100644
index 00000000..b1ad6be0
--- /dev/null
+++ b/cmake/SeastarDependencies.cmake
@@ -0,0 +1,133 @@
+#
+# This file is open source software, licensed to you under the terms
+# of the Apache License, Version 2.0 (the "License"). See the NOTICE file
+# distributed with this work for additional information regarding copyright
+# ownership. You may not use this file except in compliance with the License.
+#
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+#
+# Copyright (C) 2019 Scylladb, Ltd.
+#
+
+set (StdFilesystem_CXX_DIALECT ${Seastar_CXX_DIALECT})
+
+#
+# List of Seastar dependencies that is meant to be used
+# both in Seastar configuration and by clients which
+# consume Seastar via SeastarConfig.cmake.
+#
+# Each dependency declaration starts with ENTRY keyword.
+# The following sub-keys can be used:
+# * NAME - package name to use in `find_package`
+# * MIN_VERSION - minimum version requirement for the package
+# * other keys without values (options) are left unparsed
+# (this allows to provide `find_package` with numerous options
+# without specifically parsing them, for example: REQUIRED)
+#
+# Keys other than NAME are optional.
+#
+
+set (seastar_dependencies
+ ##
+ ## Public dependencies.
+ ##
+ ENTRY
+ NAME c-ares
+ MIN_VERSION 1.13
+ REQUIRED
+ MODULE
+ ENTRY
+ NAME cryptopp
+ MIN_VERSION 5.6.5
+ REQUIRED
+ ENTRY
+ NAME dpdk
+ ENTRY
+ NAME fmt
+ MIN_VERSION 5.0.0
+ REQUIRED
+ ENTRY
+ NAME lz4
+ MIN_VERSION 1.7.3
+ REQUIRED
+ ##
+ ## Private and private/public dependencies.
+ ##
+ ENTRY
+ NAME Concepts
+ ENTRY
+ NAME GnuTLS
+ MIN_VERSION 3.3.26
+ REQUIRED
+ ENTRY
+ NAME LinuxMembarrier
+ ENTRY
+ NAME Protobuf
+ MIN_VERSION 2.5.0
+ REQUIRED
+ ENTRY
+ NAME Sanitizers
+ ENTRY
+ NAME StdAtomic
+ REQUIRED
+ ENTRY
+ NAME StdFilesystem
+ REQUIRED
+ ENTRY
+ NAME hwloc
+ MIN_VERSION 1.11.2
+ ENTRY
+ NAME lksctp-tools
+ REQUIRED # No version information published.
+ ENTRY
+ NAME numactl # No version information published.
+ ENTRY
+ NAME rt
+ REQUIRED
+ ENTRY
+ NAME yaml-cpp
+ MIN_VERSION 0.5.1
+ REQUIRED
+)
+
+#
+# Iterate through Seastar dependencies list defined above and execute `find_package`
+# with corresponding configuration in each parsed entry.
+#
+# This is intentionally a macro since we don't want to introduce non-intuitive
+# side-effects of `find_package` calls which sets some non-cache variables.
+#
+macro (seastar_find_dependencies)
+ string (LENGTH "ENTRY" entry_kw_len)
+ while (TRUE)
+ string (FIND "${seastar_dependencies}" "ENTRY" current_entry_idx)
+ if (current_entry_idx EQUAL -1)
+ break ()
+ endif()
+ # Skip ahead ENTRY keyword
+ math (EXPR begin_position "${entry_kw_len} + 1")
+ string (SUBSTRING "${seastar_dependencies}" ${begin_position} -1 seastar_dependencies)
+ # Find the next ENTRY keyword to extract the current entry contents
+ string (FIND "${seastar_dependencies}" "ENTRY" next_entry_idx)
+ if (NOT next_entry_idx EQUAL -1)
+ math (EXPR content_end_idx "${next_entry_idx} - 1")
+ string (SUBSTRING "${seastar_dependencies}" 0 ${content_end_idx} current_entry_contents)
+ string (SUBSTRING "${seastar_dependencies}" ${next_entry_idx} -1 seastar_dependencies)
+ else()
+ set (current_entry_contents "${seastar_dependencies}")
+ endif()
+ cmake_parse_arguments (ENTRY_ARG "" "NAME;MIN_VERSION" "" "${current_entry_contents}")
+ find_package ("${ENTRY_ARG_NAME}" ${ENTRY_ARG_MIN_VERSION} ${ENTRY_ARG_UNPARSED_ARGUMENTS})
+ endwhile ()
+endmacro ()
--
2.17.1

[Jesse Haber-Kucharsky]

Hi Pavel. Thanks for looking into this!

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

`cmake/SeastarDependencies.cmake` module is introduced.
It contains `seastar_find_dependencies` macro which iterates through
dependencies specification and calls `find_package` on each parsed entry.

Main `CMakeLists.txt` and `cmake/SeastarConfig.cmake.in` use the new module
instead of listing and finding dependencies by itself.
---
 CMakeLists.txt                  |  28 +------
 cmake/SeastarConfig.cmake.in    |  25 +-----
 cmake/SeastarDependencies.cmake | 133 ++++++++++++++++++++++++++++++++
 3 files changed, 138 insertions(+), 48 deletions(-)
 create mode 100644 cmake/SeastarDependencies.cmake

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 40c0472c..cb476a01 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -229,33 +229,11 @@ find_package (Boost 1.64.0 REQUIRED
     thread
   OPTIONAL_COMPONENTS unit_test_framework)

I think you can re-write the function in `SeastarDependencies.cmake` so that Boost is not a special case.

I realize that CMake macros work differently than functions, but I don't think invoking FIND_PACKAGE from a function context is harmful. Do you have evidence to suggest otherwise?

Here is a snippet of CMake code that I think accomplishes what we want. Please let me know what you think:

    function (do_thing)

      # FIND_PACKAGE would go here.

      message (WARNING "Got: ${ARGV}")

    endfunction ()

    function (seastar_find_dependencies)

      set (dep_Boost

        Boost

        REQUIRED

        COMPONENTS

          filesystem

          program_options)

      set (dep_StdAtomic

        StdAtomic

        REQUIRED)

      set (dependency_names

        Boost

        StdAtomic)

      foreach (name ${dependency_names})

        set (args "${dep_${name}}")

        do_thing (${args})

      endforeach ()

    endfunction ()

    seastar_find_dependencies ()

 

--
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/20190409203244.3606-1-pavel.al.solodovnikov%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

[Pavel Solodovnikov]

Hi Jesse,

I think you can re-write the function in `SeastarDependencies.cmake` so that Boost is not a special case.

Here is a snippet of CMake code that I think accomplishes what we want. Please let me know what you think:

    function (do_thing)

      # FIND_PACKAGE would go here.

      message (WARNING "Got: ${ARGV}")

    endfunction ()

    function (seastar_find_dependencies)

      set (dep_Boost

        Boost

        REQUIRED

        COMPONENTS

          filesystem

          program_options)

      set (dep_StdAtomic

        StdAtomic

        REQUIRED)

      set (dependency_names

        Boost

        StdAtomic)

      foreach (name ${dependency_names})

        set (args "${dep_${name}}")

        do_thing (${args})

      endforeach ()

    endfunction ()

    seastar_find_dependencies () 

Indeed, your approach is simpler than mine. I think I would stick to that one instead, thanks for noticing.

I realize that CMake macros work differently than functions, but I don't think invoking FIND_PACKAGE from a function context is harmful. Do you have evidence to suggest otherwise?

Each call to `find_package` might set some variables (and this is not uncommon in CMake world) inside the corresponding `FindXXX.cmake`.

Those variables which have been set with ordinary `set` command would live inside enclosing `function` scope.

Simple example would be:

find_library(MyLib)

#

# Depending on FindMyLib.cmake implementation, this might set

# something like `MY_LIB_FOUND`, `MY_LIB_SMTH_OPTIONS`, `MY_LIB_INCLUDE_DIRS` etc.

#

# Anything other than `find_path`, `find_library` would be restricted to the enclosing scope.

#

I have skimmed through the current `FindXYZ.cmake` modules implementation in Seastar code base, and there are no examples,

which would be affected by the issue mentioned above (all dependencies are declared as `IMPORTED` targets).

That given, `seastar_find_dependencies` could be made a function instead of macro,

but that depends on whether all `FindXYZ.cmake` modules are always gathered inside our own sources,

or some modules could be brought, for example, from stock modules that are provided as part of CMake distribution.

If latter applies, then we should not rely on that every module will define a first-class build target that is visible more-or-less independently

of the declaration scope.

I think you can re-write the function in `SeastarDependencies.cmake` so that Boost is not a special case.

There's one more reason I didn't include Boost in the first iteration of the patch.

`find_package(Boost)` is used in two places:

1. Main `CMakeLists.txt`. `unit_test_framework` is specified as `OPTIONAL_COMPONENTS`
2. `cmake/SeastarConfig.cmake.in`. `unit_test_framework` is not optional and listed among other components.

Are they intentionally different from each other?

ср, 10 апр. 2019 г. в 20:07, Jesse Haber-Kucharsky <jhab...@scylladb.com>:

[Pavel Solodovnikov]

`cmake/SeastarDependencies.cmake` module is introduced.
It contains `seastar_find_dependencies` macro which iterates through
dependencies specification and calls `find_package` on each parsed entry.

Main `CMakeLists.txt` and `cmake/SeastarConfig.cmake.in` use the new module
instead of listing and finding dependencies by itself.

Changes since v1:

1. All `XYZ_FOUND` var usages replaced with `if (TARGET)` tests.
2. `seastar_find_dependencies` is now a function instead of a macro.
3. Simplified `seastar_find_dependencies` internal logic.
---
CMakeLists.txt | 41 ++++------------
cmake/SeastarConfig.cmake.in | 24 +--------
cmake/SeastarDependencies.cmake | 86 +++++++++++++++++++++++++++++++++
3 files changed, 98 insertions(+), 53 deletions(-)

create mode 100644 cmake/SeastarDependencies.cmake

diff --git a/CMakeLists.txt b/CMakeLists.txt

index 40c0472c..157d152a 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -229,33 +229,12 @@ find_package (Boost 1.64.0 REQUIRED
thread
OPTIONAL_COMPONENTS unit_test_framework)

-find_package (c-ares 1.13.0 REQUIRED MODULE)
-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)

-

-##
-## Private and private/public dependencies.
-##
-
-find_package (Concepts)
-find_package (GnuTLS 3.3.26 REQUIRED)
-find_package (LinuxMembarrier)
-find_package (Protobuf 2.5.0 REQUIRED)
-find_package (Sanitizers)
-find_package (StdAtomic REQUIRED)

+include (SeastarDependencies)

set (StdFilesystem_CXX_DIALECT ${Seastar_CXX_DIALECT})
-find_package (StdFilesystem REQUIRED)
-find_package (hwloc 1.11.2)
-# No version information published.
-find_package (lksctp-tools REQUIRED)
-# No version information published.
-find_package (numactl)

+seastar_find_dependencies ()
+

+# Private build dependencies not visible to consumers
find_package (ragel 6.10 REQUIRED)
-find_package (rt REQUIRED)
-find_package (yaml-cpp 0.5.1 REQUIRED)

#
# Code generation helpers.

@@ -598,7 +577,7 @@ if (Seastar_COMPRESS_DEBUG)
endif()

if (CMAKE_BUILD_TYPE STREQUAL "Debug")
- if (NOT Sanitizers_FOUND)
+ if (NOT (TARGET Sanitizers::address OR TARGET Sanitizers::undefined_behavior))
message (FATAL_ERROR "Sanitizers are required for debug mode!")
endif ()
endif ()
@@ -646,7 +625,7 @@ if (Seastar_EXPERIMENTAL_COROUTINES_TS)
target_compile_definitions (seastar PUBLIC SEASTAR_COROUTINES_TS)
endif ()

-if (Seastar_GCC6_CONCEPTS OR Concepts_FOUND)
+if (Seastar_GCC6_CONCEPTS OR TARGET Concepts::Concepts)
target_compile_definitions (seastar
PUBLIC SEASTAR_HAVE_GCC6_CONCEPTS)

@@ -654,7 +633,7 @@ if (Seastar_GCC6_CONCEPTS OR Concepts_FOUND)
PUBLIC Concepts::concepts)
endif ()

-if (LinuxMembarrier_FOUND)
+if (TARGET LinuxMembarrier::membarrier)
list (APPEND Seastar_PRIVATE_COMPILE_DEFINITIONS SEASTAR_HAS_MEMBARRIER)

target_link_libraries (seastar
@@ -696,7 +675,7 @@ if (Seastar_SPLIT_DWARF AND (NOT (CMAKE_BUILD_TYPE STREQUAL "Dev")))
endif ()

if (Seastar_DPDK)
- if (NOT dpdk_FOUND)
+ if (NOT TARGET dpdk::dpdk)
message (FATAL_ERROR "dpdk support is enabled but it is not available!")
endif ()

@@ -712,7 +691,7 @@ if ((CMAKE_BUILD_TYPE STREQUAL "Debug") OR (NOT Seastar_EXCEPTION_SCALABILITY_WO
endif ()

if (Seastar_HWLOC)
- if (NOT hwloc_FOUND)
+ if (NOT TARGET hwloc::hwloc)
message (FATAL_ERROR "`hwloc` support is enabled but it is not available!")
endif ()

@@ -729,7 +708,7 @@ if (Seastar_LD_FLAGS)
endif ()

if (Seastar_NUMA)
- if (NOT numactl_FOUND)
+ if (NOT TARGET numactl::numactl)
message (FATAL_ERROR "NUMA support is enabled but `numactl` is not available!")
endif ()

diff --git a/cmake/SeastarConfig.cmake.in b/cmake/SeastarConfig.cmake.in
index 19221871..b5aecd43 100644
--- a/cmake/SeastarConfig.cmake.in
+++ b/cmake/SeastarConfig.cmake.in
@@ -38,29 +38,9 @@ find_package (Boost

thread
unit_test_framework)

-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)
-
-#
-# Private and private/public dependencies.
-#
-
-find_package (Concepts)
-find_package (GnuTLS 3.3.26 REQUIRED)
-find_package (LinuxMembarrier)
-find_package (Protobuf 2.5.0 REQUIRED)
-find_package (Sanitizers)
-find_package (StdAtomic REQUIRED)

+include (SeastarDependencies)

set (StdFilesystem_CXX_DIALECT "@Seastar_CXX_DIALECT@")
-find_package (StdFilesystem REQUIRED)
-find_package (hwloc 1.11.2)
-find_package (lksctp-tools)
-find_package (numactl)
-find_package (rt REQUIRED)
-find_package (yaml-cpp 0.5.1 REQUIRED)

+seastar_find_dependencies ()

if (NOT TARGET Seastar::seastar)
include ("${CMAKE_CURRENT_LIST_DIR}/SeastarTargets.cmake")
diff --git a/cmake/SeastarDependencies.cmake b/cmake/SeastarDependencies.cmake
new file mode 100644

index 00000000..52696ba0
--- /dev/null
+++ b/cmake/SeastarDependencies.cmake
@@ -0,0 +1,86 @@

+#
+# This file is open source software, licensed to you under the terms
+# of the Apache License, Version 2.0 (the "License"). See the NOTICE file
+# distributed with this work for additional information regarding copyright
+# ownership. You may not use this file except in compliance with the License.
+#
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+#
+# Copyright (C) 2019 Scylladb, Ltd.
+#
+

+#
+# Iterate through Seastar dependencies list defined above and execute `find_package`

+# with corresponding configuration for each 3rd-party dependency.
+#
+function (seastar_find_dependencies)
+ #
+ # List of Seastar dependencies that is meant to be used
+ # both in Seastar configuration and by clients which

+ # consume Seastar via SeastarConfig.cmake.
+ #

+ set (all_dependencies
+ # Public dependencies.
+ Boost
+ c-ares
+ cryptopp
+ dpdk # No version information published.
+ fmt
+ lz4
+ # Private and private/public dependencies.
+ Concepts
+ GnuTLS
+ LinuxMembarrier
+ Protobuf
+ Sanitizers
+ StdAtomic
+ StdFilesystem
+ hwloc
+ lksctp-tools # No version information published.
+ numactl # No version information published.
+ rt
+ yaml-cpp
+ )
+
+ # Arguments to `find_package` for each 3rd-party dependency.
+ # Note that version specification is `minimal` version requirement.
+ set (dep_args_Boost
+ 1.64.0
+ COMPONENTS
+ filesystem
+ program_options
+ thread
+ OPTIONAL_COMPONENTS unit_test_framework
+ REQUIRED
+ )
+ set (dep_args_c-ares 1.13 REQUIRED)
+ set (dep_args_cryptopp 5.6.5 REQUIRED)
+ set (dep_args_fmt 5.0.0 REQUIRED)
+ set (dep_args_lz4 1.7.3 REQUIRED)
+ set (dep_args_GnuTLS 3.3.26 REQUIRED)
+ set (dep_args_Protobuf 2.5.0 REQUIRED)
+ set (dep_args_StdAtomic REQUIRED)
+ set (dep_args_StdFilesystem REQUIRED)
+ set (dep_args_hwloc 1.11.2)
+ set (dep_args_lksctp-tools REQUIRED)
+ set (dep_args_rt REQUIRED)
+ set (dep_args_yaml-cpp 0.5.1 REQUIRED)
+
+ foreach (third_party ${all_dependencies})
+ # `find_package` works as expected in the `function` context as long
+ # as we don't rely on `XYZ_FOUND` values or other
+ # non-cache variables defined in `FindXYZ.cmake` scripts.
+ find_package ("${third_party}" ${dep_args_${third_party}})
+ endforeach ()
+endfunction ()
--
2.17.1

[Pavel Solodovnikov]

`cmake/SeastarDependencies.cmake` module is introduced.
It contains `seastar_find_dependencies` macro which iterates through
dependencies specification and calls `find_package` on each parsed entry.

Main `CMakeLists.txt` and `cmake/SeastarConfig.cmake.in` use the new module
instead of listing and finding dependencies by itself.

Changes since v2:

1. Remove redundant `find_package` for Boost in `CMakeLists.txt` and `SeastarConfig.cmake.in`.
2. Minor fixes to comments in cmake code.
---
CMakeLists.txt | 52 ++++----------------
cmake/SeastarConfig.cmake.in | 35 ++------------
cmake/SeastarDependencies.cmake | 86 +++++++++++++++++++++++++++++++++
3 files changed, 99 insertions(+), 74 deletions(-)

create mode 100644 cmake/SeastarDependencies.cmake

diff --git a/CMakeLists.txt b/CMakeLists.txt

index 40c0472c..9422846a 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -218,44 +218,12 @@ set (Seastar_GEN_BINARY_DIR ${Seastar_BINARY_DIR}/gen)
# Dependencies.
#

-##
-## Public dependencies.
-##
-
-find_package (Boost 1.64.0 REQUIRED
- COMPONENTS
- filesystem
- program_options
- thread
- OPTIONAL_COMPONENTS unit_test_framework)
-

@@ -598,7 +566,7 @@ if (Seastar_COMPRESS_DEBUG)

endif()

if (CMAKE_BUILD_TYPE STREQUAL "Debug")
- if (NOT Sanitizers_FOUND)
+ if (NOT (TARGET Sanitizers::address OR TARGET Sanitizers::undefined_behavior))
message (FATAL_ERROR "Sanitizers are required for debug mode!")
endif ()
endif ()

@@ -646,7 +614,7 @@ if (Seastar_EXPERIMENTAL_COROUTINES_TS)

target_compile_definitions (seastar PUBLIC SEASTAR_COROUTINES_TS)
endif ()

-if (Seastar_GCC6_CONCEPTS OR Concepts_FOUND)
+if (Seastar_GCC6_CONCEPTS OR TARGET Concepts::Concepts)
target_compile_definitions (seastar
PUBLIC SEASTAR_HAVE_GCC6_CONCEPTS)

@@ -654,7 +622,7 @@ if (Seastar_GCC6_CONCEPTS OR Concepts_FOUND)

PUBLIC Concepts::concepts)
endif ()

-if (LinuxMembarrier_FOUND)
+if (TARGET LinuxMembarrier::membarrier)
list (APPEND Seastar_PRIVATE_COMPILE_DEFINITIONS SEASTAR_HAS_MEMBARRIER)

target_link_libraries (seastar

@@ -696,7 +664,7 @@ if (Seastar_SPLIT_DWARF AND (NOT (CMAKE_BUILD_TYPE STREQUAL "Dev")))

endif ()

if (Seastar_DPDK)
- if (NOT dpdk_FOUND)
+ if (NOT TARGET dpdk::dpdk)
message (FATAL_ERROR "dpdk support is enabled but it is not available!")
endif ()

@@ -712,7 +680,7 @@ if ((CMAKE_BUILD_TYPE STREQUAL "Debug") OR (NOT Seastar_EXCEPTION_SCALABILITY_WO

endif ()

if (Seastar_HWLOC)
- if (NOT hwloc_FOUND)
+ if (NOT TARGET hwloc::hwloc)
message (FATAL_ERROR "`hwloc` support is enabled but it is not available!")
endif ()

@@ -729,7 +697,7 @@ if (Seastar_LD_FLAGS)

endif ()

if (Seastar_NUMA)
- if (NOT numactl_FOUND)
+ if (NOT TARGET numactl::numactl)
message (FATAL_ERROR "NUMA support is enabled but `numactl` is not available!")
endif ()

diff --git a/cmake/SeastarConfig.cmake.in b/cmake/SeastarConfig.cmake.in

index 19221871..ebf96e3d 100644
--- a/cmake/SeastarConfig.cmake.in
+++ b/cmake/SeastarConfig.cmake.in
@@ -26,41 +26,12 @@
list (APPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_LIST_DIR})

#
-# Public dependencies.
+# Dependencies.
#

-find_package (Boost
- 1.64.0
- REQUIRED
- COMPONENTS
- filesystem
- program_options
- thread
- unit_test_framework)
-

[Jesse Haber-Kucharsky]

Ah! That makes sense.

 

Simple example would be:

find_library(MyLib)

#

# Depending on FindMyLib.cmake implementation, this might set

# something like `MY_LIB_FOUND`, `MY_LIB_SMTH_OPTIONS`, `MY_LIB_INCLUDE_DIRS` etc.

#

# Anything other than `find_path`, `find_library` would be restricted to the enclosing scope.

#

I have skimmed through the current `FindXYZ.cmake` modules implementation in Seastar code base, and there are no examples,

which would be affected by the issue mentioned above (all dependencies are declared as `IMPORTED` targets).

That given, `seastar_find_dependencies` could be made a function instead of macro,

but that depends on whether all `FindXYZ.cmake` modules are always gathered inside our own sources,

or some modules could be brought, for example, from stock modules that are provided as part of CMake distribution.

That sounds fairly restrictive. I agree with you that it makes sense to not introduce a scope that prevents variables from being "seen".

 

If latter applies, then we should not rely on that every module will define a first-class build target that is visible more-or-less independently

of the declaration scope.

I think you can re-write the function in `SeastarDependencies.cmake` so that Boost is not a special case.

There's one more reason I didn't include Boost in the first iteration of the patch.

`find_package(Boost)` is used in two places:

1. Main `CMakeLists.txt`. `unit_test_framework` is specified as `OPTIONAL_COMPONENTS`
2. `cmake/SeastarConfig.cmake.in`. `unit_test_framework` is not optional and listed among other components.

Are they intentionally different from each other?

They are, but perhaps they don't need to be.

The reason is that in the testing library (`seastar_testing`) is not defined unless Seastar is being installed or tests are enabled. Since `SeastarConfig.cmake.in` is only used when Seastar is installed, `unit_test_framework` is a mandatory component.

[Pavel Solodovnikov]

`cmake/SeastarDependencies.cmake` module is introduced.
It contains `seastar_find_dependencies` macro which iterates through
dependencies specification and calls `find_package` on each parsed entry.

Main `CMakeLists.txt` and `cmake/SeastarConfig.cmake.in` use the new module
instead of listing and finding dependencies by itself.

Changes since v3:

1. `unit_test_framework` is made to be required component for Boost.
---
CMakeLists.txt | 52 ++++---------------
cmake/SeastarConfig.cmake.in | 35 ++-----------
cmake/SeastarDependencies.cmake | 89 +++++++++++++++++++++++++++++++++
3 files changed, 102 insertions(+), 74 deletions(-)

create mode 100644 cmake/SeastarDependencies.cmake

diff --git a/CMakeLists.txt b/CMakeLists.txt

index 40c0472c..9422846a 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt

@@ -218,44 +218,12 @@ set (Seastar_GEN_BINARY_DIR ${Seastar_BINARY_DIR}/gen)
# Dependencies.
#

-##
-## Public dependencies.
-##
-
-find_package (Boost 1.64.0 REQUIRED
- COMPONENTS
- filesystem
- program_options
- thread
- OPTIONAL_COMPONENTS unit_test_framework)
-

-find_package (c-ares 1.13.0 REQUIRED MODULE)
-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)

-

-##
-## Private and private/public dependencies.
-##
-
-find_package (Concepts)
-find_package (GnuTLS 3.3.26 REQUIRED)
-find_package (LinuxMembarrier)
-find_package (Protobuf 2.5.0 REQUIRED)
-find_package (Sanitizers)
-find_package (StdAtomic REQUIRED)

+include (SeastarDependencies)

set (StdFilesystem_CXX_DIALECT ${Seastar_CXX_DIALECT})
-find_package (StdFilesystem REQUIRED)
-find_package (hwloc 1.11.2)
-# No version information published.
-find_package (lksctp-tools REQUIRED)
-# No version information published.
-find_package (numactl)

+seastar_find_dependencies ()
+

+# Private build dependencies not visible to consumers
find_package (ragel 6.10 REQUIRED)
-find_package (rt REQUIRED)
-find_package (yaml-cpp 0.5.1 REQUIRED)

#
# Code generation helpers.

diff --git a/cmake/SeastarConfig.cmake.in b/cmake/SeastarConfig.cmake.in
index 19221871..ebf96e3d 100644
--- a/cmake/SeastarConfig.cmake.in
+++ b/cmake/SeastarConfig.cmake.in

@@ -26,41 +26,12 @@
list (APPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_LIST_DIR})

#
-# Public dependencies.
+# Dependencies.
#

-find_package (Boost
- 1.64.0
- REQUIRED
- COMPONENTS
- filesystem
- program_options
- thread
- unit_test_framework)
-

-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)
-
-#
-# Private and private/public dependencies.
-#
-
-find_package (Concepts)
-find_package (GnuTLS 3.3.26 REQUIRED)
-find_package (LinuxMembarrier)
-find_package (Protobuf 2.5.0 REQUIRED)
-find_package (Sanitizers)
-find_package (StdAtomic REQUIRED)

+include (SeastarDependencies)

set (StdFilesystem_CXX_DIALECT "@Seastar_CXX_DIALECT@")
-find_package (StdFilesystem REQUIRED)
-find_package (hwloc 1.11.2)
-find_package (lksctp-tools)
-find_package (numactl)
-find_package (rt REQUIRED)
-find_package (yaml-cpp 0.5.1 REQUIRED)

+seastar_find_dependencies ()

if (NOT TARGET Seastar::seastar)
include ("${CMAKE_CURRENT_LIST_DIR}/SeastarTargets.cmake")
diff --git a/cmake/SeastarDependencies.cmake b/cmake/SeastarDependencies.cmake
new file mode 100644

index 00000000..3e5be767
--- /dev/null
+++ b/cmake/SeastarDependencies.cmake
@@ -0,0 +1,89 @@

+#
+# This file is open source software, licensed to you under the terms
+# of the Apache License, Version 2.0 (the "License"). See the NOTICE file
+# distributed with this work for additional information regarding copyright
+# ownership. You may not use this file except in compliance with the License.
+#
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+#
+# Copyright (C) 2019 Scylladb, Ltd.
+#
+

+#
+# Iterate through Seastar dependencies list defined above and execute `find_package`

+ # `unit_test_framework` is mandatory component since `SeastarConfig.cmake.in`
+ # is only used when Seastar is installed, and so `seastar_testing` library too.

+ set (dep_args_Boost
+ 1.64.0
+ COMPONENTS
+ filesystem
+ program_options
+ thread

+ unit_test_framework

[Pavel Solodovnikov]

They are, but perhaps they don't need to be.

The reason is that in the testing library (`seastar_testing`) is not defined unless Seastar is being installed or tests are enabled. Since `SeastarConfig.cmake.in` is only used when Seastar is installed, `unit_test_framework` is a mandatory component.

Thanks for clarifying, I will remove OPTIONAL from `unit_test_framework` component.  

That sounds fairly restrictive. I agree with you that it makes sense to not introduce a scope that prevents variables from being "seen".

 

I have come to the conclusion that maybe it would still be better to make it a function since we don't want to leak 

unnecessarily `all_dependencies` variable and individual dependency configuration arguments to the caller site.

I have modified the rest of the CMake code in `CMakeLists.txt` to test for `IMPORTED` targets existence instead of

testing against `XXX_FOUND` variables. It would fit better into terms of `modern CMake` :)

Sent a new revision of the patch to the mailing list. 

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

[Jesse Haber-Kucharsky]

Apologies for the delay in following up with you, Pavel.

I mostly think this patch goes in the right direction, but I dislike the requirement on not using `*_FOUND` variables: I think it will lend to brittleness that we can avoid.

What if we make `seastar_find_dependencies` a macro again and just prefix the variables with "_seastar_"?

--
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/20190416140540.2658-1-pavel.al.solodovnikov%40gmail.com.

[Pavel Solodovnikov]

I mostly think this patch goes in the right direction, but I dislike the requirement on not using `*_FOUND` variables: I think it will lend to brittleness that we can avoid.

There are two reasons it can be bad:

1. `FindXXX.cmake` can define more than one resulting target, some of them can be mistakenly skipped in `if` checks that test whether library is found or not. Also these conditions can become too verbose.
2. As you already mentioned, it constrains users in the sense that there is only one way to do things right and it would be necessary to document this behavior appropriately.

What if we make `seastar_find_dependencies` a macro again and just prefix the variables with "_seastar_"?

I think you are right that it would be better not to force users to remember every time whether `XXX_FOUND` or `TARGET XXX` should be used.

вт, 23 апр. 2019 г. в 21:17, Jesse Haber-Kucharsky <jhab...@scylladb.com>:

[Pavel Solodovnikov]

`cmake/SeastarDependencies.cmake` module is introduced.
It contains `seastar_find_dependencies` macro which iterates through
dependencies specification and calls `find_package` on each parsed entry.

Main `CMakeLists.txt` and `cmake/SeastarConfig.cmake.in` use the new module
instead of listing and finding dependencies by itself.

Changes since v4:

1. `seastar_find_dependencies` is a macro again.
2. Variables in `SeastarDependencies.cmake` are prefixed with `_seastar_`.
3. Revert `if(TARGET)` testing for 3rd-parties in the main `CMakeLists.txt`.
---
CMakeLists.txt | 40 ++-------------

cmake/SeastarConfig.cmake.in | 35 ++------------
cmake/SeastarDependencies.cmake | 86 +++++++++++++++++++++++++++++++++

3 files changed, 93 insertions(+), 68 deletions(-)

create mode 100644 cmake/SeastarDependencies.cmake

diff --git a/CMakeLists.txt b/CMakeLists.txt

index b97a40a7..ae7ebec1 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -226,44 +226,12 @@ set (Seastar_GEN_BINARY_DIR ${Seastar_BINARY_DIR}/gen)

index 00000000..0f27e246
--- /dev/null
+++ b/cmake/SeastarDependencies.cmake
@@ -0,0 +1,86 @@

+macro (seastar_find_dependencies)

+ #
+ # List of Seastar dependencies that is meant to be used
+ # both in Seastar configuration and by clients which
+ # consume Seastar via SeastarConfig.cmake.
+ #

+ set (_seastar_all_dependencies

+ set (_seastar_dep_args_Boost

+ 1.64.0
+ COMPONENTS
+ filesystem
+ program_options
+ thread
+ unit_test_framework
+ REQUIRED
+ )

+ set (_seastar_dep_args_c-ares 1.13 REQUIRED)
+ set (_seastar_dep_args_cryptopp 5.6.5 REQUIRED)
+ set (_seastar_dep_args_fmt 5.0.0 REQUIRED)
+ set (_seastar_dep_args_lz4 1.7.3 REQUIRED)
+ set (_seastar_dep_args_GnuTLS 3.3.26 REQUIRED)
+ set (_seastar_dep_args_Protobuf 2.5.0 REQUIRED)
+ set (_seastar_dep_args_StdAtomic REQUIRED)
+ set (_seastar_dep_args_StdFilesystem REQUIRED)
+ set (_seastar_dep_args_hwloc 1.11.2)
+ set (_seastar_dep_args_lksctp-tools REQUIRED)
+ set (_seastar_dep_args_rt REQUIRED)
+ set (_seastar_dep_args_yaml-cpp 0.5.1 REQUIRED)
+
+ foreach (third_party ${_seastar_all_dependencies})
+ find_package ("${third_party}" ${_seastar_dep_args_${third_party}})
+ endforeach ()
+endmacro ()
--
2.17.1

[Jesse Haber-Kucharsky]

Thanks, Pavel!

I have some minor comments below.

Have you tested these changes? An easy way is to run the "dist" test: `ninja -C build/dev test_dist`

"Iterate through [the] dependency list defined [...]"

"above" -> "below"?

"Note that [the] version [...]"

 

+
+  # `unit_test_framework` is mandatory component since `SeastarConfig.cmake.in`
+  # is only used when Seastar is installed, and so `seastar_testing` library too.

I don't understand this comment.

Technically, the `unit_test_framework` is not mandatory since it's only required when testing is enabled or Seastar is installed.

However, my understanding was that we're making it mandatory to avoid the complication of having different specifications for the Boost dependency.

 

--
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/20190424105007.12189-1-pavel.al.solodovnikov%40gmail.com.

[Pavel Solodovnikov]

Have you tested these changes? An easy way is to run the "dist" test: `ninja -C build/dev test_dist`

Thanks for the reminder! Found a bug while running `test_dist`: `SeastarDependencies.cmake` should be installed

as well as the other cmake modules (e.g. `cmake/FindXXX.cmake`) while packaging Seastar for distribution. Fixed that in a follow-up patch.

"Iterate through [the] dependency list defined [...]"

"above" -> "below"?

 

"Note that [the] version [...]" 

Fixed.

+
+  # `unit_test_framework` is mandatory component since `SeastarConfig.cmake.in`
+  # is only used when Seastar is installed, and so `seastar_testing` library too.

I don't understand this comment.

Technically, the `unit_test_framework` is not mandatory since it's only required when testing is enabled or Seastar is installed.

However, my understanding was that we're making it mandatory to avoid the complication of having different specifications for the Boost dependency.

Yup, bad wording. This should be rewritten to reflect the initial motivation behind the decision to not mark the component as `OPTIONAL`.

чт, 25 апр. 2019 г. в 17:39, Jesse Haber-Kucharsky <jhab...@scylladb.com>:

[Pavel Solodovnikov]

`cmake/SeastarDependencies.cmake` module is introduced.
It contains `seastar_find_dependencies` macro which iterates through
dependencies specification and calls `find_package` on each parsed entry.

Main `CMakeLists.txt` and `cmake/SeastarConfig.cmake.in` use the new module
instead of listing and finding dependencies by itself.

Changes since v5:

1. `cmake/SeastarDependencies.cmake` is installed along with other cmake modules.
2. Fixed the wording and some typos in comments.
---
CMakeLists.txt | 41 ++--------------
cmake/SeastarConfig.cmake.in | 35 ++-----------
cmake/SeastarDependencies.cmake | 87 +++++++++++++++++++++++++++++++++
3 files changed, 95 insertions(+), 68 deletions(-)

create mode 100644 cmake/SeastarDependencies.cmake

diff --git a/CMakeLists.txt b/CMakeLists.txt

index b97a40a7..7177137e 100644

@@ -1020,6 +988,7 @@ if (Seastar_INSTALL)
${CMAKE_CURRENT_SOURCE_DIR}/cmake/Findragel.cmake
${CMAKE_CURRENT_SOURCE_DIR}/cmake/Findrt.cmake
${CMAKE_CURRENT_SOURCE_DIR}/cmake/Findyaml-cpp.cmake
+ ${CMAKE_CURRENT_SOURCE_DIR}/cmake/SeastarDependencies.cmake
DESTINATION ${install_cmakedir})

install (

index 00000000..781e7921
--- /dev/null
+++ b/cmake/SeastarDependencies.cmake
@@ -0,0 +1,87 @@

+#
+# This file is open source software, licensed to you under the terms
+# of the Apache License, Version 2.0 (the "License"). See the NOTICE file
+# distributed with this work for additional information regarding copyright
+# ownership. You may not use this file except in compliance with the License.
+#
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+#
+# Copyright (C) 2019 Scylladb, Ltd.
+#
+
+#

+# Iterate through the dependency list defined below and execute `find_package`
+# with the corresponding configuration for each 3rd-party dependency.

+ # Note that the version specification is `minimal` version requirement.
+
+ # `unit_test_framework` is not required in the case we are building Seastar
+ # without the testing library, however the component is always specified as required
+ # to keep the CMake code minimalistic and easy-to-use.

[Jesse Haber-Kucharsky]

On Thu, Apr 25, 2019 at 2:47 PM Pavel Solodovnikov <pavel.al.s...@gmail.com> wrote:

`cmake/SeastarDependencies.cmake` module is introduced.
It contains `seastar_find_dependencies` macro which iterates through
dependencies specification and calls `find_package` on each parsed entry.

Main `CMakeLists.txt` and `cmake/SeastarConfig.cmake.in` use the new module
instead of listing and finding dependencies by itself.

Changes since v5:

1. `cmake/SeastarDependencies.cmake` is installed along with other cmake modules.
2. Fixed the wording and some typos in comments.

Thanks, Pavel! This looks great.

I verified that Seastar configures and that `test_dist` passes. I also observed that all of the version requirements have been maintained in the new macro.

I think this is ready to be merged after some very minor cosmetic changes.

')' on the same line as yaml-cpp.

 

+
+  # Arguments to `find_package` for each 3rd-party dependency.
+  # Note that the version specification is `minimal` version requirement.

[...] specification is a "minimal" version requirement.

 

+
+  # `unit_test_framework` is not required in the case we are building Seastar
+  # without the testing library, however the component is always specified as required
+  # to keep the CMake code minimalistic and easy-to-use.
+  set (_seastar_dep_args_Boost
+    1.64.0
+    COMPONENTS
+      filesystem
+      program_options
+      thread
+      unit_test_framework
+    REQUIRED
+  )

')' on the same line as `REQUIRED`, and newline after multi-line statement.

 

+  set (_seastar_dep_args_c-ares 1.13 REQUIRED)
+  set (_seastar_dep_args_cryptopp 5.6.5 REQUIRED)
+  set (_seastar_dep_args_fmt 5.0.0 REQUIRED)
+  set (_seastar_dep_args_lz4 1.7.3 REQUIRED)
+  set (_seastar_dep_args_GnuTLS 3.3.26 REQUIRED)
+  set (_seastar_dep_args_Protobuf 2.5.0 REQUIRED)
+  set (_seastar_dep_args_StdAtomic REQUIRED)
+  set (_seastar_dep_args_StdFilesystem REQUIRED)
+  set (_seastar_dep_args_hwloc 1.11.2)
+  set (_seastar_dep_args_lksctp-tools REQUIRED)
+  set (_seastar_dep_args_rt REQUIRED)
+  set (_seastar_dep_args_yaml-cpp 0.5.1 REQUIRED)
+
+  foreach (third_party ${_seastar_all_dependencies})
+    find_package ("${third_party}" ${_seastar_dep_args_${third_party}})
+  endforeach ()
+endmacro ()
--
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/20190425184659.17883-1-pavel.al.solodovnikov%40gmail.com.

[Pavel Solodovnikov]

`cmake/SeastarDependencies.cmake` module is introduced.
It contains `seastar_find_dependencies` macro which iterates through
dependencies specification and calls `find_package` on each parsed entry.

Main `CMakeLists.txt` and `cmake/SeastarConfig.cmake.in` use the new module
instead of listing and finding dependencies by itself.

Changes since v6:
Fix code-style issues and a minor wording issue in cmake comments.
---
CMakeLists.txt | 41 ++--------------
cmake/SeastarConfig.cmake.in | 35 ++------------
cmake/SeastarDependencies.cmake | 86 +++++++++++++++++++++++++++++++++
3 files changed, 94 insertions(+), 68 deletions(-)

index 00000000..0495eca2
--- /dev/null
+++ b/cmake/SeastarDependencies.cmake
@@ -0,0 +1,86 @@

+ yaml-cpp)

+
+ # Arguments to `find_package` for each 3rd-party dependency.

+ # Note that the version specification is a "minimal" version requirement.

+
+ # `unit_test_framework` is not required in the case we are building Seastar
+ # without the testing library, however the component is always specified as required
+ # to keep the CMake code minimalistic and easy-to-use.
+ set (_seastar_dep_args_Boost
+ 1.64.0
+ COMPONENTS
+ filesystem
+ program_options
+ thread
+ unit_test_framework

+ REQUIRED)
+

[Pavel Solodovnikov]

Thanks, Jesse.

Sent a new patch revision to address these issues.

пн, 29 апр. 2019 г. в 17:35, Jesse Haber-Kucharsky <jhab...@scylladb.com>:

[Jesse Haber-Kucharsky]

On Mon, Apr 29, 2019 at 10:54 AM Pavel Solodovnikov <pavel.al.s...@gmail.com> wrote:

`cmake/SeastarDependencies.cmake` module is introduced.
It contains `seastar_find_dependencies` macro which iterates through
dependencies specification and calls `find_package` on each parsed entry.

Main `CMakeLists.txt` and `cmake/SeastarConfig.cmake.in` use the new module
instead of listing and finding dependencies by itself.

Changes since v6:
Fix code-style issues and a minor wording issue in cmake comments.

LGTM! Thanks again.

 

--
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/20190429145359.12873-1-pavel.al.solodovnikov%40gmail.com.

[Pavel Solodovnikov]

Hello Avi,

Does anyone else need to review the topic before it can be merged into upstream?

Cheers, Pavel

[Commit Bot]

From: Pavel Solodovnikov <pavel.al.s...@gmail.com>
Committer: Avi Kivity <a...@scylladb.com>
Branch: master

Deduplicate Seastar dependencies management in CMake scripts

`cmake/SeastarDependencies.cmake` module is introduced.
It contains `seastar_find_dependencies` macro which iterates through
dependencies specification and calls `find_package` on each parsed entry.

Main `CMakeLists.txt` and `cmake/SeastarConfig.cmake.in` use the new module
instead of listing and finding dependencies by itself.

Message-Id: <20190429145359.12873-1...@gmail.com>

---
diff --git a/CMakeLists.txt b/CMakeLists.txt

@@ -1031,6 +999,7 @@ if (Seastar_INSTALL)

${CMAKE_CURRENT_SOURCE_DIR}/cmake/Findragel.cmake
${CMAKE_CURRENT_SOURCE_DIR}/cmake/Findrt.cmake
${CMAKE_CURRENT_SOURCE_DIR}/cmake/Findyaml-cpp.cmake
+ ${CMAKE_CURRENT_SOURCE_DIR}/cmake/SeastarDependencies.cmake
DESTINATION ${install_cmakedir})

install (
diff --git a/cmake/SeastarConfig.cmake.in b/cmake/SeastarConfig.cmake.in

--- a/cmake/SeastarDependencies.cmake