-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
(#12358): NuRaft recipe #12360
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 243c5adnuraft/cci.20220801
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple small comments and questions
Overall it's super close
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") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🙈
- | ||
- set(ASIO_INCLUDE_DIR ${BOOST_INCLUDE_PATH}) | ||
- set(LIBBOOST_SYSTEM "${BOOST_LIBRARY_PATH}/libboost_system.a") | ||
- | ||
-else () | ||
- # If not, ASIO standalone mode. | ||
+ set(ASIO_DEP boost::boost) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
This comment has been minimized.
This comment has been minimized.
raise ConanInvalidConfiguration("Building Shared Object for {} unsupported".format(self.info.settings.os)) | ||
if self.info.settings.compiler.cppstd: | ||
check_min_cppstd(self, 11) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def layout(self): | |
cmake_layout(self, src_folder="src") |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from conan.tools.cmake import CMake, CMakeDeps, CMakeToolchain | |
from conan.tools.cmake import CMake, CMakeDeps, CMakeToolchain, cmake_layout |
|
||
def requirements(self): | ||
if self.options.asio == "boost": | ||
self.requires("boost/1.79.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.requires("boost/1.79.0") | |
self.requires("boost/1.80.0") |
We have Boost 1.80.0 available on Conan Center.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
|
||
class TestPackageConan(ConanFile): | ||
settings = "os", "compiler", "build_type", "arch" | ||
generators = "cmake", "cmake_find_package_multi" |
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs a new test package https://github.com/conan-io/conan-center-index/pull/12360/files#r975553072
Conan v1 pipelineAll green in build 31 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although there are still some comments to be resolved, I think there is a reasonable amount of work here to move the PR forward. So let's do it and keep the details for another one 👊
Specify library name and version: nuraft/cci.20220801
A robust RAFT implementation in C++11 used throughout eBay storage services.
closes #12358