You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/10/09 06:33:59 UTC
[GitHub] [pulsar-client-cpp] BewareMyPower opened a new pull request, #28: [fix] Force static libraries to be linked when LINK_STATIC is enabled
BewareMyPower opened a new pull request, #28:
URL: https://github.com/apache/pulsar-client-cpp/pull/28
### Motivation
Currently, even with `LINK_STATIC=ON` option, dynamic libraries could still be linked. For example, on macOS, the following libcurl library will be found:
```
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.3.sdk/usr/lib/libcurl.tbd
```
Then, the `libpulsar.dylib` will link to `/usr/lib/libcurl.4.dylib`, which is not a dynamic library.
### Modifications
When `LINK_STATIC` is enabled, change `CMAKE_FIND_LIBRARY_SUFFIXES` to `.a` for non-MSVC compilers so that only static libraries could be found except OpenSSL, which should not be linked statically.
In addition, use `find_package` as much as possible instead of raw usages of `find_path` and `find_library`.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] merlimat commented on pull request #28: [fix] Force static libraries to be linked when LINK_STATIC is enabled
Posted by GitBox <gi...@apache.org>.
merlimat commented on PR #28:
URL: https://github.com/apache/pulsar-client-cpp/pull/28#issuecomment-1272571537
@BewareMyPower CMake is now failing to find OpenSSL on Mac (installed with brew):
```
CMake Error at /opt/homebrew/Cellar/cmake/3.23.0/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:230 (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):
/opt/homebrew/Cellar/cmake/3.23.0/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:594 (_FPHSA_FAILURE_MESSAGE)
/opt/homebrew/Cellar/cmake/3.23.0/share/cmake/Modules/FindOpenSSL.cmake:578 (find_package_handle_standard_args)
CMakeLists.txt:127 (find_package)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] merlimat merged pull request #28: [fix] Force static libraries to be linked when LINK_STATIC is enabled
Posted by GitBox <gi...@apache.org>.
merlimat merged PR #28:
URL: https://github.com/apache/pulsar-client-cpp/pull/28
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on pull request #28: [fix] Force static libraries to be linked when LINK_STATIC is enabled
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #28:
URL: https://github.com/apache/pulsar-client-cpp/pull/28#issuecomment-1272580848
> Additionally, we should still use static lib for OpenSSL too
It is exactly what I am confused. From https://pulsar.apache.org/docs/client-libraries-cpp#install-dependencies
> The libpulsarwithdeps.a does not include library openssl related libraries libssl and libcrypto, because these two libraries are related to security. It is more reasonable and easier to use the versions provided by the local system to handle security issues and upgrade libraries.
I also found the original PR (https://github.com/apache/pulsar/pull/6458) from @jiazhai.
> Because if there is a security problem in libssl, all the machines can just update their own libssl library without rebuilding libpulsar.a.
Should we revert these complicated logic to find and link OpenSSL dependency?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on pull request #28: [fix] Force static libraries to be linked when LINK_STATIC is enabled
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #28:
URL: https://github.com/apache/pulsar-client-cpp/pull/28#issuecomment-1272578365
@merlimat I just tested again in my local env.
Here is my command (just test the default `LINK_STATIC` option because it does not affect OpenSSL):
```bash
cmake -Wno-dev \
-DCMAKE_PREFIX_PATH="/usr/local" \
-DBUILD_TESTS=OFF -DBUILD_PERF_TOOLS=ON \
-B build
```
Here are my outputs after this PR:
```
-- Found OpenSSL: /opt/local/lib/libcrypto.dylib (found version "3.0.4")
-- Found OpenSSL: /opt/local/lib/libcrypto.dylib (found version "3.0.4")
RECORD_OPENSSL_SSL_LIBRARY: /opt/local/lib/libssl.dylib
RECORD_OPENSSL_CRYPTO_LIBRARY: /opt/local/lib/libcrypto.dylib
-- Found OpenSSL: /usr/local/opt/openssl/lib/libcrypto.dylib (found version "3.0.4")
OPENSSL_INCLUDE_DIR: /usr/local/opt/openssl/include//opt/homebrew/opt/openssl/include
OPENSSL_SSL_LIBRARY: /usr/local/opt/openssl/lib/libssl.dylib
OPENSSL_CRYPTO_LIBRARY: /usr/local/opt/openssl/lib/libcrypto.dylib
```
Here are my outputs before this PR (just added the same logs):
```
-- Found OpenSSL: /usr/local/opt/openssl/lib/libcrypto.dylib
RECORD_OPENSSL_SSL_LIBRARY: /usr/local/opt/openssl/lib/libssl.dylib
RECORD_OPENSSL_CRYPTO_LIBRARY: /usr/local/opt/openssl/lib/libcrypto.dylib
OPENSSL_INCLUDE_DIR: /usr/local/opt/openssl/include//opt/homebrew/opt/openssl/include
OPENSSL_SSL_LIBRARY: /usr/local/opt/openssl/lib/libssl.dylib
OPENSSL_CRYPTO_LIBRARY: /usr/local/opt/openssl/lib/libcrypto.dylib
```
The patch to add logs:
```diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index d3876f8..5952576 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -135,6 +135,8 @@ set(OPENSSL_USE_STATIC_LIBS FALSE)
find_package(OpenSSL REQUIRED)
set(RECORD_OPENSSL_SSL_LIBRARY ${OPENSSL_SSL_LIBRARY})
set(RECORD_OPENSSL_CRYPTO_LIBRARY ${OPENSSL_CRYPTO_LIBRARY})
+message("RECORD_OPENSSL_SSL_LIBRARY: " ${RECORD_OPENSSL_SSL_LIBRARY})
+message("RECORD_OPENSSL_CRYPTO_LIBRARY: " ${RECORD_OPENSSL_CRYPTO_LIBRARY})
unset(OPENSSL_FOUND CACHE)
unset(OPENSSL_INCLUDE_DIR CACHE)
@@ -270,6 +272,9 @@ endif()
find_package(Boost REQUIRED COMPONENTS ${BOOST_COMPONENTS})
find_package(OpenSSL REQUIRED)
+message("OPENSSL_INCLUDE_DIR: " ${OPENSSL_INCLUDE_DIR})
+message("OPENSSL_SSL_LIBRARY: " ${OPENSSL_SSL_LIBRARY})
+message("OPENSSL_CRYPTO_LIBRARY: " ${OPENSSL_CRYPTO_LIBRARY})
if (BUILD_TESTS)
find_path(GTEST_INCLUDE_PATH gtest/gtest.h)
```
It looks like this PR brings some unexpected changes, I will investigate and fix it soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on pull request #28: [fix] Force static libraries to be linked when LINK_STATIC is enabled
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #28:
URL: https://github.com/apache/pulsar-client-cpp/pull/28#issuecomment-1272588627
> if we don't do that, there can be linking problems between different OpenSSL version (eg: 1.1.1 vs 3.0.4)
Yeah, I agree. The ABI compatibility changed a lot, see https://abi-laboratory.pro/?view=timeline&l=openssl
In addition,
> libpulsar.so is a shared library, containing statically linked boost and openssl.
>
> libpulsarnossl.so is a shared library, similar to libpulsar.so except that the libraries openssl and crypto are dynamically linked.
I think we should remove `libpulsarnossl.so` and link statically to openssl in `libpulsarwithdeps.a`, right? @merlimat
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] merlimat commented on pull request #28: [fix] Force static libraries to be linked when LINK_STATIC is enabled
Posted by GitBox <gi...@apache.org>.
merlimat commented on PR #28:
URL: https://github.com/apache/pulsar-client-cpp/pull/28#issuecomment-1272586615
I think that if we don't do that, there can be linking problems between different OpenSSL version (eg: 1.1.1 vs 3.0.4)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] merlimat commented on pull request #28: [fix] Force static libraries to be linked when LINK_STATIC is enabled
Posted by GitBox <gi...@apache.org>.
merlimat commented on PR #28:
URL: https://github.com/apache/pulsar-client-cpp/pull/28#issuecomment-1272579642
I think there’s one extra find_package for OpenSSL at the beginning (3 calls in total). It should be removed.
Additionally, we should still use static lib for OpenSSL too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-cpp] BewareMyPower commented on pull request #28: [fix] Force static libraries to be linked when LINK_STATIC is enabled
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #28:
URL: https://github.com/apache/pulsar-client-cpp/pull/28#issuecomment-1272507139
@merlimat @Demogorgon314 @RobertIndie Now all tests passed, PTAL.
Currently I'm working on fixing the Windows build when `LINK_STATIC` is `ON`, which might be based on this PR. Because when `LINK_STATIC` is `ON`, no Windows dependencies could be found because the suffix is not `.a`. This PR accidentally fixes some issues of finding dependencies. Regarding other issues to build statically linked Windows library, will push another PR later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org