From fb53f71a87f8b357cf3dd4a59b788b6b7681de1c Mon Sep 17 00:00:00 2001 From: Yunze Xu Date: Fri, 14 Jul 2023 00:33:03 +0800 Subject: [PATCH] Fix the build failure with C++20 standard ### Motivation When building the project with the `-DCMAKE_CXX_STANDARD=20` option and GCC 11.3, it failed. There are two main reasons. One is the `ObjectPool.h`, see http://eel.is/c++draft/diff.cpp17.class#2 In short, see the code below: ```c++ template struct A { // A() {} // error: simple-template-id not allowed for constructor A() {} // OK, injected-class-name used }; ``` The other reason is deeply hidden and OS-specific. When building the target for the unit test, the `lib/` directory is added into the include directories. So for `#include "Semaphore.h"`, the `Semaphore.h` header will be looked up first in the `lib/` directory. However, C++20 introduced a `` header, which finds the POSIX semaphore header `semaphore.h` in the system path. For example, the include order in `ubuntu:22.04` arm64 container is: - `$PROJECT_DIR/lib/` (where `Semaphore.h` is) - ... - `/usr/lib/gcc/aarch64-linux-gnu/11/include` (where `semaphore.h` is) The C++ header is case insensitive so the `lib/Semaphore.h` will be included by the `` header, which is implicitly included by ``. Our own `Semaphore.h` does not have the POSIX semaphore struct definitions so the build failed. ### Modifications - Fix the semantics error in `ObjectPool.h` - Remove the `lib/` directory from the included directories of the unit test and include `lib/xxx.h` for header in `lib/` directory. - Add a workflow to verify now it can be built with C++20 --- .github/workflows/ci-pr-validation.yaml | 28 +++++++++++++++++++++++-- lib/ObjectPool.h | 4 ++-- lib/ProducerImpl.cc | 1 + lib/ProducerImpl.h | 2 +- tests/CMakeLists.txt | 2 +- tests/InterceptorsTest.cc | 2 +- tests/KeyValueImplTest.cc | 3 ++- tests/LookupServiceTest.cc | 8 +++---- tests/MessageChunkingTest.cc | 2 +- tests/MessageTest.cc | 2 +- tests/SchemaTest.cc | 2 +- tests/TableViewTest.cc | 4 ++-- tests/c/c_SeekTest.cc | 3 ++- 13 files changed, 45 insertions(+), 18 deletions(-) diff --git a/.github/workflows/ci-pr-validation.yaml b/.github/workflows/ci-pr-validation.yaml index d6188174..0d85c6d7 100644 --- a/.github/workflows/ci-pr-validation.yaml +++ b/.github/workflows/ci-pr-validation.yaml @@ -90,11 +90,35 @@ jobs: run: make check-format - name: Build - run: make -j8 + run: | + # Build the libraries first to avoid possible link failures + cmake --build . -j8 --target pulsarShared pulsarStatic + cmake --build . -j8 - name: Run unit tests run: RETRY_FAILED=3 ./run-unit-tests.sh + cpp20-build: + name: Build with the C++20 standard + runs-on: ubuntu-22.04 + timeout-minutes: 60 + + steps: + - name: checkout + uses: actions/checkout@v3 + - name: Install deps + run: | + sudo apt-get update -y + sudo apt-get install -y libcurl4-openssl-dev libssl-dev \ + protobuf-compiler libprotobuf-dev libboost-dev \ + libboost-dev libboost-program-options-dev \ + libzstd-dev libsnappy-dev libgmock-dev libgtest-dev + - name: CMake + run: cmake -B build -DBUILD_PERF_TOOLS=ON -DCMAKE_CXX_STANDARD=20 + - name: Build + run: | + cmake --build build -j8 --target pulsarShared pulsarStatic + cmake --build build -j8 cpp-build-windows: timeout-minutes: 120 @@ -281,7 +305,7 @@ jobs: check-completion: name: Check Completion runs-on: ubuntu-latest - needs: [wireshark-dissector-build, unit-tests, cpp-build-windows, package, cpp-build-macos] + needs: [wireshark-dissector-build, unit-tests, cpp20-build, cpp-build-windows, package, cpp-build-macos] steps: - run: true diff --git a/lib/ObjectPool.h b/lib/ObjectPool.h index 883e0809..6fcb771f 100644 --- a/lib/ObjectPool.h +++ b/lib/ObjectPool.h @@ -223,8 +223,8 @@ class ObjectPool { } private: - ObjectPool(const ObjectPool&); - ObjectPool& operator=(const ObjectPool&); + ObjectPool(const ObjectPool&); + ObjectPool& operator=(const ObjectPool&); }; } // namespace pulsar #endif /* LIB_OBJECTPOOL_H_ */ diff --git a/lib/ProducerImpl.cc b/lib/ProducerImpl.cc index 597bfd37..3b831666 100644 --- a/lib/ProducerImpl.cc +++ b/lib/ProducerImpl.cc @@ -36,6 +36,7 @@ #include "OpSendMsg.h" #include "ProducerConfigurationImpl.h" #include "PulsarApi.pb.h" +#include "Semaphore.h" #include "TimeUtils.h" #include "TopicName.h" #include "stats/ProducerStatsDisabled.h" diff --git a/lib/ProducerImpl.h b/lib/ProducerImpl.h index b041f473..afc6346b 100644 --- a/lib/ProducerImpl.h +++ b/lib/ProducerImpl.h @@ -31,7 +31,6 @@ #include "PendingFailures.h" #include "PeriodicTask.h" #include "ProducerImplBase.h" -#include "Semaphore.h" namespace pulsar { @@ -53,6 +52,7 @@ class PulsarFriend; class Producer; class MemoryLimitController; +class Semaphore; class TopicName; struct OpSendMsg; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index f5c86629..6b464251 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -56,7 +56,7 @@ file(GLOB TEST_SOURCES *.cc c/*.cc) add_executable(pulsar-tests ${TEST_SOURCES} ${PROTO_SOURCES}) -target_include_directories(pulsar-tests PRIVATE ${PROJECT_SOURCE_DIR}/lib ${AUTOGEN_DIR}/lib) +target_include_directories(pulsar-tests PRIVATE ${AUTOGEN_DIR}/lib) target_link_libraries(pulsar-tests ${CLIENT_LIBS} pulsarStatic $<$:${GMOCKD_LIBRARY_PATH}> $<$:${GTESTD_LIBRARY_PATH}> $<$>:${GMOCK_LIBRARY_PATH}> $<$>:${GTEST_LIBRARY_PATH}>) diff --git a/tests/InterceptorsTest.cc b/tests/InterceptorsTest.cc index b643dbbb..db87d8f4 100644 --- a/tests/InterceptorsTest.cc +++ b/tests/InterceptorsTest.cc @@ -25,7 +25,7 @@ #include #include "HttpHelper.h" -#include "Latch.h" +#include "lib/Latch.h" #include "lib/LogUtils.h" DECLARE_LOG_OBJECT() diff --git a/tests/KeyValueImplTest.cc b/tests/KeyValueImplTest.cc index 89770c41..c2cce006 100644 --- a/tests/KeyValueImplTest.cc +++ b/tests/KeyValueImplTest.cc @@ -16,9 +16,10 @@ * specific language governing permissions and limitations * under the License. */ -#include #include +#include "lib/KeyValueImpl.h" + using namespace pulsar; TEST(KeyValueTest, testEncodeAndDeCode) { diff --git a/tests/LookupServiceTest.cc b/tests/LookupServiceTest.cc index aff8409b..bc6ea2be 100644 --- a/tests/LookupServiceTest.cc +++ b/tests/LookupServiceTest.cc @@ -16,10 +16,6 @@ * specific language governing permissions and limitations * under the License. */ -#include -#include -#include -#include #include #include #include @@ -31,11 +27,15 @@ #include "HttpHelper.h" #include "PulsarFriend.h" +#include "lib/BinaryProtoLookupService.h" #include "lib/ClientConnection.h" #include "lib/ConnectionPool.h" +#include "lib/Future.h" +#include "lib/HTTPLookupService.h" #include "lib/LogUtils.h" #include "lib/RetryableLookupService.h" #include "lib/TimeUtils.h" +#include "lib/Utils.h" DECLARE_LOG_OBJECT() diff --git a/tests/MessageChunkingTest.cc b/tests/MessageChunkingTest.cc index 9a65fbc6..6d54a69f 100644 --- a/tests/MessageChunkingTest.cc +++ b/tests/MessageChunkingTest.cc @@ -23,9 +23,9 @@ #include #include -#include "ChunkMessageIdImpl.h" #include "PulsarFriend.h" #include "WaitUtils.h" +#include "lib/ChunkMessageIdImpl.h" #include "lib/LogUtils.h" DECLARE_LOG_OBJECT() diff --git a/tests/MessageTest.cc b/tests/MessageTest.cc index 3dfe2217..189d40e6 100644 --- a/tests/MessageTest.cc +++ b/tests/MessageTest.cc @@ -22,7 +22,7 @@ #include -#include "MessageImpl.h" +#include "lib/MessageImpl.h" using namespace pulsar; TEST(MessageTest, testMessageContents) { diff --git a/tests/SchemaTest.cc b/tests/SchemaTest.cc index 34e224e6..1fad081c 100644 --- a/tests/SchemaTest.cc +++ b/tests/SchemaTest.cc @@ -20,7 +20,7 @@ #include #include "PulsarFriend.h" -#include "SharedBuffer.h" +#include "lib/SharedBuffer.h" using namespace pulsar; diff --git a/tests/TableViewTest.cc b/tests/TableViewTest.cc index a645b9a0..2515818d 100644 --- a/tests/TableViewTest.cc +++ b/tests/TableViewTest.cc @@ -23,10 +23,10 @@ #include #include "HttpHelper.h" -#include "LogUtils.h" #include "PulsarFriend.h" -#include "TopicName.h" #include "WaitUtils.h" +#include "lib/LogUtils.h" +#include "lib/TopicName.h" using namespace pulsar; diff --git a/tests/c/c_SeekTest.cc b/tests/c/c_SeekTest.cc index cfa8a188..7295c849 100644 --- a/tests/c/c_SeekTest.cc +++ b/tests/c/c_SeekTest.cc @@ -17,12 +17,13 @@ * under the License. */ -#include #include #include #include +#include "lib/TimeUtils.h" + struct seek_ctx { std::promise *promise; };