You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "BewareMyPower (via GitHub)" <gi...@apache.org> on 2023/05/15 23:41:50 UTC
[GitHub] [pulsar-client-cpp] BewareMyPower opened a new pull request, #269: Support specifying the C++ standard for some higher dependencies
BewareMyPower opened a new pull request, #269:
URL: https://github.com/apache/pulsar-client-cpp/pull/269
Fixes https://github.com/apache/pulsar-client-cpp/issues/268
### Motivation
When users build from source with a dependency that requires a higher C++ version than 11, it will fail because currently the C++ standard is pinned to 11. For example, the latest GTest requires C++14 now. So currently there is no way to build the tests except modifying the CMakeLists.txt file.
### Modifications
Make `CMAKE_CXX_STANDARD` configurable and test it for the macOS workflow because the `gtest` dependency installed from `brew` requires C++14. Add the description into the README for it.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [ ] `doc-not-needed`
(Please explain why)
- [x] `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] BewareMyPower commented on pull request #269: Support specifying the C++ standard for some higher dependencies
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #269:
URL: https://github.com/apache/pulsar-client-cpp/pull/269#issuecomment-1548897805
> another way is pinning the version of gtest
Yes. But for most C++ developers, it's still much easier to install the binary dependencies via the default package manager on Linux and macOS
The `FetchContent` feature of CMake just downloads the source code. It still needs to build and install the dependencies. And as you can see we still need to pay some extra efforts to it: https://github.com/apache/incubator-kvrocks/blob/75db3e42df8a715339abaf02d640f3c62d43304a/cmake/utils.cmake
Actually, we can achieve this goal via Vcpkg as well. Vcpkg can specify dependency versions. See example here: https://github.com/apache/pulsar-client-python/pull/23/files#diff-e62a7794d36dac6a2bbd197522dfe4d80d8d77f2e54e4772271e14e0103829e1
But still, for development, installing binaries directly will be much easier unless the dependencies are depended by source.
--
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] tisonkun commented on a diff in pull request #269: Support specifying the C++ standard for some higher dependencies
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #269:
URL: https://github.com/apache/pulsar-client-cpp/pull/269#discussion_r1194533381
##########
CMakeLists.txt:
##########
@@ -88,7 +88,9 @@ find_package(Threads REQUIRED)
MESSAGE(STATUS "Threads library: " ${CMAKE_THREAD_LIBS_INIT})
set(Boost_NO_BOOST_CMAKE ON)
-set(CMAKE_CXX_STANDARD 11)
+if (NOT CMAKE_CXX_STANDARD)
+ set(CMAKE_CXX_STANDARD 11)
+endif ()
set(CMAKE_C_STANDARD 11)
Review Comment:
For options alignment, perhaps perform the same change for `CMAKE_C_STANDARD`?
--
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 a diff in pull request #269: Support specifying the C++ standard for some higher dependencies
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #269:
URL: https://github.com/apache/pulsar-client-cpp/pull/269#discussion_r1194539664
##########
CMakeLists.txt:
##########
@@ -88,7 +88,9 @@ find_package(Threads REQUIRED)
MESSAGE(STATUS "Threads library: " ${CMAKE_THREAD_LIBS_INIT})
set(Boost_NO_BOOST_CMAKE ON)
-set(CMAKE_CXX_STANDARD 11)
+if (NOT CMAKE_CXX_STANDARD)
+ set(CMAKE_CXX_STANDARD 11)
+endif ()
set(CMAKE_C_STANDARD 11)
Review Comment:
No. The C standard is only applied to
- The C headers (`*.h` files) under https://github.com/apache/pulsar-client-cpp/tree/main/include/pulsar/c
- The C examples, i.e. the `*.c` files under https://github.com/apache/pulsar-client-cpp/tree/main/examples
They are never affected by any external dependency.
We should still stick to the C++11 and C11 standard for this project, but we cannot avoid the minimum standard version requirements from external C++ dependencies.
--
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] tisonkun commented on pull request #269: Support specifying the C++ standard for some higher dependencies
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #269:
URL: https://github.com/apache/pulsar-client-cpp/pull/269#issuecomment-1548886456
For reference - another way is pinning the version of gtest, like [gtest.cmake in Kvrocks](https://github.com/apache/incubator-kvrocks/blob/75db3e42df8a715339abaf02d640f3c62d43304a/cmake/gtest.cmake).
--
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] tisonkun commented on a diff in pull request #269: Support specifying the C++ standard for some higher dependencies
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #269:
URL: https://github.com/apache/pulsar-client-cpp/pull/269#discussion_r1194540048
##########
CMakeLists.txt:
##########
@@ -88,7 +88,9 @@ find_package(Threads REQUIRED)
MESSAGE(STATUS "Threads library: " ${CMAKE_THREAD_LIBS_INIT})
set(Boost_NO_BOOST_CMAKE ON)
-set(CMAKE_CXX_STANDARD 11)
+if (NOT CMAKE_CXX_STANDARD)
+ set(CMAKE_CXX_STANDARD 11)
+endif ()
set(CMAKE_C_STANDARD 11)
Review Comment:
Thanks for your explanation! That sounds good to me.
--
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 merged pull request #269: Support specifying the C++ standard for some higher dependencies
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower merged PR #269:
URL: https://github.com/apache/pulsar-client-cpp/pull/269
--
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