eBay / NuRaft

C++ implementation of Raft core logic as a replication library
Apache License 2.0
1.02k stars 241 forks source link

Use find_package for OpenSSL #358

Closed ppande closed 1 year ago

ppande commented 2 years ago
greensky00 commented 2 years ago

Before diving into the review, what is the purpose of this commit? Building NuRaft has no problem with openssl, and if you want to define a custom path, you can simply do

cmake -DDEPS_PREFIX=<path>

And FYI, this PR causes an error on Mac as follows, even though openssl is already installed in /usr/local/opt/openssl:

-- deps prefix is not given
CMake Error at /usr/local/Cellar/cmake/3.15.4/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:137 (message):
  Could NOT find OpenSSL, try to set the path to OpenSSL root folder in the
  system variable OPENSSL_ROOT_DIR (missing: OPENSSL_CRYPTO_LIBRARY
  OPENSSL_INCLUDE_DIR)
Call Stack (most recent call first):
  /usr/local/Cellar/cmake/3.15.4/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:378 (_FPHSA_FAILURE_MESSAGE)
  /usr/local/Cellar/cmake/3.15.4/share/cmake/Modules/FindOpenSSL.cmake:413 (find_package_handle_standard_args)
  CMakeLists.txt:89 (find_package)

-- Configuring incomplete, errors occurred!
ppande commented 2 years ago

Before diving into the review, what is the purpose of this commit? Building NuRaft has no problem with openssl, and if you want to define a custom path, you can simply do

The purpose is to replace any hardcoded paths or custom variables with built-in support provided by cmake for OpenSSL. The problem arises when it is one of the various dependencies in a project.

And FYI, this PR causes an error on Mac as follows, ..

As the message reported by cmake describes, setting OPENSSL_ROOT_DIR to /usr/local/opt/openssl would get you past it

greensky00 commented 2 years ago

There are two problems that must be fixed:

Once those are fixed, let me start the review of the CMake script itself. Thanks.

greensky00 commented 2 years ago
ppande commented 2 years ago

Thanks @greensky00. I will re-work these changes to incorporate your suggestions and update the PR

JosiahWI commented 1 year ago

Thank you for starting these changes, @ppande. There wasn't any progress on this for over a year, so I picked up the work and #451 has now landed. I think this PR can be closed.