Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(#12358): NuRaft recipe #12360

Merged
merged 34 commits into from
Nov 25, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
d3f3c52
(#12358): NuRaft recipe
szmyd Aug 16, 2022
3479815
Remove old test runner.
szmyd Aug 18, 2022
760f049
Adjust tools imports
szmyd Aug 18, 2022
e15648c
New tools defined as member methods.
szmyd Aug 18, 2022
1b7b17a
Adjust variable name.
szmyd Aug 18, 2022
4508f76
Remove shared library option from Macos.
szmyd Aug 18, 2022
1a4fc0f
Do not remove options later referenced.
szmyd Aug 18, 2022
6e34aca
Fix import for v2.
szmyd Aug 18, 2022
54c0077
Remove running unit tests.
szmyd Aug 19, 2022
4cd84fe
Remove fPIC option on Windows.
szmyd Aug 19, 2022
3cfbbe0
Cleanup recipe with 2.x migrations.
szmyd Aug 19, 2022
d99e916
Disable Windows builds entirely.
szmyd Aug 19, 2022
243c5ad
Typo
szmyd Aug 19, 2022
a25ae02
Add libm to systemlibs linkages.
szmyd Aug 20, 2022
4b4d568
Provide patch descriptions.
szmyd Aug 26, 2022
936eea4
Simplify test_package.
szmyd Aug 26, 2022
5fcba08
Use CONFIG mode for test_package.
szmyd Aug 26, 2022
38a000e
Update recipes/nuraft/all/conanfile.py
szmyd Aug 30, 2022
4a892d2
Remove example code from library.
szmyd Aug 30, 2022
b625742
Adjust install path for headers.
szmyd Aug 30, 2022
a74bd31
Use built-on install target.
szmyd Aug 30, 2022
5604fd1
Cleanup test_package some more.
szmyd Aug 31, 2022
a6d1846
Merge branch 'upstream_master' into nuraft
szmyd Sep 12, 2022
4059bda
Allow both Boost::ASIO and standalone ASIO libraries.
szmyd Sep 12, 2022
e756ad6
Remove unused toolchain file.
szmyd Sep 12, 2022
5f160cf
Remove conans.tools import
szmyd Sep 12, 2022
7ede518
Remove unused import
szmyd Sep 12, 2022
121d34d
Cleanup patchwork
szmyd Sep 12, 2022
e6a2b6e
Remove unused recipe property.
szmyd Sep 12, 2022
4d342a3
Fix missing namespace.
szmyd Sep 14, 2022
08b2020
Add back original fPIC option from upstream CMake.
szmyd Sep 14, 2022
872ea8a
Review comments addressed.
szmyd Sep 15, 2022
ee6e727
Update to v2.0.0
szmyd Nov 2, 2022
f4798d4
Remove old reference.
szmyd Nov 21, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions recipes/nuraft/all/conandata.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
sources:
"2.0.0":
url: https://github.com/ebay/nuraft/archive/refs/tags/v2.0.0.tar.gz
sha256: f7f535f0e5c0417fb9a0ab87514a1b77647fc8e7ed967b85ca611df1cae14023
patches:
"2.0.0":
- patch_file: "patches/0001-cmake_patches.patch"
patch_description: "Provide CMake correct dependency discovery logic."
patch_type: "conan"
sha256: 8147e2f3b950f944433220539376b1c8e0e93207902f48aa2d3bcbca814a674a
78 changes: 78 additions & 0 deletions recipes/nuraft/all/conanfile.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
from os.path import join
from conan import ConanFile
from conan.errors import ConanInvalidConfiguration
from conan.tools.files import copy, get, apply_conandata_patches
from conan.tools.cmake import CMake, CMakeDeps, CMakeToolchain
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
from conan.tools.cmake import CMake, CMakeDeps, CMakeToolchain
from conan.tools.cmake import CMake, CMakeDeps, CMakeToolchain, cmake_layout

from conan.tools.build import check_min_cppstd

required_conan_version = ">=1.50.0"

class NuRaftConan(ConanFile):
name = "nuraft"
homepage = "https://github.corp.ebay.com/sds/NuRaft"
description = """Cornerstone based RAFT library."""
topics = ("raft")
url = "https://github.com/conan-io/conan-center-index"
license = "Apache-2.0"

settings = "os", "compiler", "arch", "build_type"

options = {
"asio": ["boost", "standalone"],
"shared": ['True', 'False'],
"fPIC": ['True', 'False'],
}
default_options = {
"asio": "boost",
"shared": False,
"fPIC": True,
}

def export_sources(self):
for p in self.conan_data.get("patches", {}).get(self.version, []):
copy(self, p["patch_file"], self.recipe_folder, self.export_sources_folder)

def requirements(self):
if self.options.asio == "boost":
self.requires("boost/1.79.0")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.requires("boost/1.79.0")
self.requires("boost/1.80.0")

We have Boost 1.80.0 available on Conan Center.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it not better to target a lower MINOR since it won’t require recompile should someone want 1.80.0 as opposed to the reverse which would?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

soon or later, all recipes will use 1.80.0, then your recipe will need to be updated too. Doing it now, it are just doing what should be done later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In ConanCenter we always try to stay on latest to avoid conflicts... two recipes using boost should work together (hopefully)

else:
self.requires("asio/1.22.1")
self.requires("openssl/1.1.1s")

def validate(self):
if self.info.settings.os in ["Windows"]:
raise ConanInvalidConfiguration("{} Builds are unsupported".format(self.info.settings.os))
if self.info.settings.os in ["Macos"] and self.options.shared:
raise ConanInvalidConfiguration("Building Shared Object for {} unsupported".format(self.info.settings.os))
if self.info.settings.compiler.cppstd:
check_min_cppstd(self, 11)
szmyd marked this conversation as resolved.
Show resolved Hide resolved

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def layout(self):
cmake_layout(self, src_folder="src")

def configure(self):
if self.options.shared:
del self.options.fPIC

def source(self):
get(self, **self.conan_data["sources"][self.version], strip_root=True, destination=self.source_folder)

def generate(self):
tc = CMakeToolchain(self)
tc.generate()
cmake = CMakeDeps(self)
cmake.generate()

def build(self):
apply_conandata_patches(self)
cmake = CMake(self)
cmake.configure()
cmake.build()

def package(self):
cmake = CMake(self)
cmake.install()
copy(self, "LICENSE", self.source_folder, join(self.package_folder, "licenses"), keep_path=False)

def package_info(self):
self.cpp_info.libs = ["nuraft"]
self.cpp_info.system_libs = ["m"]
if self.settings.os in ["Linux", "FreeBSD"]:
self.cpp_info.system_libs.extend(["pthread"])
147 changes: 147 additions & 0 deletions recipes/nuraft/all/patches/0001-cmake_patches.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 980d466..9258209 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -1,5 +1,6 @@
cmake_minimum_required(VERSION 3.5)
project(NuRaft VERSION 1.0.0 LANGUAGES CXX)
+set (CMAKE_CXX_STANDARD 11)

# === Build type (default: RelWithDebInfo, O2) ===========
if (NOT CMAKE_BUILD_TYPE)
@@ -26,41 +27,23 @@ endif()


# === Find ASIO ===
-if (BOOST_INCLUDE_PATH AND BOOST_LIBRARY_PATH)
+find_package(OpenSSL CONFIG REQUIRED)
+find_package(Boost CONFIG)
+if (Boost_FOUND)
# If Boost path (both include and library) is given,
# use Boost's ASIO.
- message(STATUS "Boost include path: " ${BOOST_INCLUDE_PATH})
- message(STATUS "Boost library path: " ${BOOST_LIBRARY_PATH})
-
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DUSE_BOOST_ASIO")
-
- set(ASIO_INCLUDE_DIR ${BOOST_INCLUDE_PATH})
- set(LIBBOOST_SYSTEM "${BOOST_LIBRARY_PATH}/libboost_system.a")
-
+ set(ASIO_DEP boost::boost)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this was replaced below 🤔

Should we not use the asio target?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is? If Boost_FOUND tests false I find the asio package and set ASIO_DEP to asio::asio and use ${ASIO_DEP} as the target_link_library. Am I missing the point?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought there was Boost::asio but it's not official now that I double check https://cmake.org/cmake/help/latest/module/FindBoost.html#imported-targets

else ()
# If not, ASIO standalone mode.
- FIND_PATH(ASIO_INCLUDE_DIR
- NAME asio.hpp
- HINTS ${PROJECT_SOURCE_DIR}/asio/asio/include
- $ENV{HOME}/local/include
- /opt/local/include
- /usr/local/include
- /usr/include)
-
+ find_package(Asio CONFIG REQUIRED)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DASIO_STANDALONE")
-
-endif ()
-
-if (NOT ASIO_INCLUDE_DIR)
- message(FATAL_ERROR "Can't find ASIO header files")
-else ()
- message(STATUS "ASIO include path: " ${ASIO_INCLUDE_DIR})
+ set(ASIO_DEP asio::asio)
endif ()


# === Includes ===
include_directories(BEFORE ./)
-include_directories(BEFORE ${ASIO_INCLUDE_DIR})
include_directories(BEFORE ${PROJECT_SOURCE_DIR}/include)
include_directories(BEFORE ${PROJECT_SOURCE_DIR}/include/libnuraft)
include_directories(BEFORE ${PROJECT_SOURCE_DIR}/examples)
@@ -81,20 +64,10 @@ endif()
if (NOT WIN32)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-pessimizing-move")
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -g")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fPIC")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be managed by Conan

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I let it, then the shared library option does not include PIC and I end up with:

/usr/bin/ld: CMakeFiles/RAFT_CORE_OBJ.dir/src/handle_commit.cxx.o: relocation R_X86_64_TPOFF32 against `_ZZN6nuraft11raft_server11reconfigureERKSt10shared_ptrINS_14cluster_configEEE8temp_buf' can not be used when making a shared object; recompile with -fPIC

So it seems fPIC is needed regardless which conan is not managing correctly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conan does not pass -fPIC, but the CMAKE_POSITION_INDEPENDENT_CODE definition

https://docs.conan.io/en/latest/reference/conanfile/tools/cmake/cmaketoolchain.html#cmaketoolchain

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, and it seems to not be present with the shared option enabled (I’ll double check), so I had to leave the original author’s -fPIC flag in the cmake rules.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😕 by default CMake does the right thing https://cmake.org/cmake/help/latest/prop_tgt/POSITION_INDEPENDENT_CODE.html#prop_tgt:POSITION_INDEPENDENT_CODE

So there must be some hackery in the build script tripping up the sensible default 🙈

- if (APPLE)
- include_directories(BEFORE /usr/local/opt/openssl/include)
- link_directories(/usr/local/opt/openssl/lib)
- else()
- set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -pthread")
- endif ()
-
else ()
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /wd5045 /wd4571 /wd4774 /wd4820 /wd5039 /wd4626 /wd4625 /wd5026 /wd5027 /wd4623 /wd4996 /wd4530 /wd4267 /wd4244 /W3")
message(STATUS "---- WIN32 ----")
- set(DISABLE_SSL 1)
endif ()

# === Disable SSL ===
@@ -250,6 +223,7 @@ set(RAFT_CORE
${ROOT_SRC}/stat_mgr.cxx
)
add_library(RAFT_CORE_OBJ OBJECT ${RAFT_CORE})
+target_link_libraries(RAFT_CORE_OBJ ${ASIO_DEP} openssl::openssl)

set(STATIC_LIB_SRC
$<TARGET_OBJECTS:RAFT_CORE_OBJ>)
@@ -257,54 +231,11 @@ set(STATIC_LIB_SRC
# === Executables ===
set(LIBRARY_NAME "nuraft")

-add_library(static_lib ${STATIC_LIB_SRC})
-set_target_properties(static_lib PROPERTIES OUTPUT_NAME ${LIBRARY_NAME} CLEAN_DIRECT_OUTPUT 1)
-
-add_library(shared_lib SHARED ${STATIC_LIB_SRC})
-set_target_properties(shared_lib PROPERTIES OUTPUT_NAME ${LIBRARY_NAME} CLEAN_DIRECT_OUTPUT 1)
-if (APPLE)
- target_link_libraries(shared_lib ${LIBRARIES})
-endif ()
-
-if (WIN32)
- set(LIBRARY_OUTPUT_NAME "${LIBRARY_NAME}.lib")
-else ()
- set(LIBRARY_OUTPUT_NAME "lib${LIBRARY_NAME}.a")
-endif ()
-message(STATUS "Output library file name: ${LIBRARY_OUTPUT_NAME}")
-
-# === Examples ===
-add_subdirectory("${PROJECT_SOURCE_DIR}/examples")
+add_library(nuraft ${STATIC_LIB_SRC})
+target_link_libraries(nuraft ${ASIO_DEP} openssl::openssl)


# === Tests ===
-add_subdirectory("${PROJECT_SOURCE_DIR}/tests")
-
-
-if (CODE_COVERAGE GREATER 0)
- set(CODE_COVERAGE_DEPS
- raft_server_test
- failure_test
- asio_service_test
- buffer_test
- serialization_test
- timer_test
- strfmt_test
- stat_mgr_test
- )
-
- # lcov
- SETUP_TARGET_FOR_COVERAGE(
- NAME raft_cov
- EXECUTABLE ./runtests.sh
- DEPENDENCIES ${CODE_COVERAGE_DEPS}
- )
-endif()
-
-
-# === Install Targets ===
-install(TARGETS static_lib ARCHIVE DESTINATION lib)
-install(TARGETS shared_lib LIBRARY DESTINATION lib)
+install(TARGETS nuraft ARCHIVE DESTINATION lib)
+install(TARGETS nuraft LIBRARY DESTINATION lib)
install(DIRECTORY ${PROJECT_SOURCE_DIR}/include/libnuraft DESTINATION include)
-
-
12 changes: 12 additions & 0 deletions recipes/nuraft/all/test_package/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
cmake_minimum_required(VERSION 3.8)
project(test_package LANGUAGES CXX)

include(${CMAKE_BINARY_DIR}/conanbuildinfo.cmake)
conan_basic_setup(TARGETS)

find_package(nuraft CONFIG REQUIRED)

add_executable(${PROJECT_NAME})
target_sources(${PROJECT_NAME} PRIVATE test_package.cpp)
target_link_libraries(${PROJECT_NAME} nuraft::nuraft)
target_compile_features(${PROJECT_NAME} PUBLIC cxx_std_11)
18 changes: 18 additions & 0 deletions recipes/nuraft/all/test_package/conanfile.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import os
from conan import ConanFile
from conans import CMake
from conan.tools.build import cross_building

class TestPackageConan(ConanFile):
settings = "os", "compiler", "build_type", "arch"
generators = "cmake", "cmake_find_package_multi"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be the test for the new generators and this can be moved to test_v1_package/


def build(self):
cmake = CMake(self)
cmake.configure()
cmake.build()

def test(self):
if not cross_building(self):
bin_path = os.path.join("bin", "test_package")
self.run(bin_path, run_environment=True)
61 changes: 61 additions & 0 deletions recipes/nuraft/all/test_package/test_package.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
#include <iostream>
#include <libnuraft/nuraft.hxx>

struct ex_logger : ::nuraft::logger {
void set_level(int) override {}
void debug(const std::string&) override {}
void info(const std::string&) override {}
void warn(const std::string&) override {}
void err(const std::string& log_line) override {
std::cout << log_line << std::endl;
}

void put_details(int, const char*, const char*, size_t,
const std::string&) override {}
};

class echo_state_machine : public nuraft::state_machine {
nuraft::ulong last_commit_idx_;

public:
echo_state_machine() : last_commit_idx_(0) {}

nuraft::ptr<nuraft::buffer> commit(const nuraft::ulong log_idx,
nuraft::buffer&) override {
last_commit_idx_ = log_idx;
return nullptr;
}
nuraft::ptr<nuraft::buffer> pre_commit(const nuraft::ulong,
nuraft::buffer&) override {
return nullptr;
}
void rollback(const nuraft::ulong, nuraft::buffer&) override {}
void save_snapshot_data(nuraft::snapshot&, const nuraft::ulong,
nuraft::buffer&) override {}
bool apply_snapshot(nuraft::snapshot&) override { return true; }
int read_snapshot_data(nuraft::snapshot&, const nuraft::ulong,
nuraft::buffer&) override {
return 0;
}
nuraft::ptr<nuraft::snapshot> last_snapshot() override { return nullptr; }
void create_snapshot(nuraft::snapshot&,
nuraft::async_result<bool>::handler_type&) override {}
nuraft::ulong last_commit_index() override { return last_commit_idx_; }
};

int main(int argc, char** argv) {
// State machine.
nuraft::ptr<nuraft::state_machine> smachine(
nuraft::cs_new<echo_state_machine>());

// Parameters.
nuraft::raft_params params;

// ASIO service.
nuraft::ptr<nuraft::logger> l = nuraft::cs_new<ex_logger>();
nuraft::ptr<nuraft::asio_service> asio_svc_ =
nuraft::cs_new<nuraft::asio_service>();
nuraft::ptr<nuraft::rpc_listener> listener(
asio_svc_->create_rpc_listener((ushort)(9001), l));
return 0;
}
3 changes: 3 additions & 0 deletions recipes/nuraft/config.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
versions:
"2.0.0":
folder: all