From eeb1bb47f9365365167fd4de4ca2aff18e6dea6f Mon Sep 17 00:00:00 2001 From: Jeff Ithier Date: Thu, 19 Dec 2024 17:37:16 +1100 Subject: [PATCH 1/4] [#15] Generalize CI for clang build --- .github/workflows/build-test.yml | 183 ++++++++++++++++--------------- 1 file changed, 96 insertions(+), 87 deletions(-) diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml index 28eb053..448a582 100644 --- a/.github/workflows/build-test.yml +++ b/.github/workflows/build-test.yml @@ -71,95 +71,104 @@ jobs: # git fetch origin main # ./scripts/check-clang-tidy.sh warning-as-error diff-to-main - colcon-build-test-clang: + colcon-build-test: needs: [preflight-check, static-code-analysis] + strategy: + matrix: + compiler: [clang] + include: + - compiler: clang + cc: clang + cxx: clang++ runs-on: ubuntu-latest container: image: ros:rolling steps: - - name: Install ros build dependencies - run: | - apt-get update - apt-get install -y \ - python3-colcon-common-extensions \ - python3-colcon-mixin \ - python3-vcstool \ - python3-rosdep \ - libacl1-dev \ - software-properties-common \ - curl \ - wget - - - name: Install rust toolchain - uses: dtolnay/rust-toolchain@stable - - - name: Create workspace - run: | - mkdir -p $WORKSPACE_DIR/src/rmw_iceoryx2 - mkdir -p $WORKSPACE_DIR/src/test_interface_files - - - name: Checkout test_interface_files - uses: actions/checkout@v4 - with: - repository: ros2/test_interface_files - path: src/test_interface_files - ref: rolling - - - name: Checkout current ref - uses: actions/checkout@v4 - with: - path: src/rmw_iceoryx2 - - - name: Install clang toolchain - uses: ./src/rmw_iceoryx2/.github/actions/install-clang - - - name: Import dependencies with VCS - run: | - cd $WORKSPACE_DIR - sed 's|git@github.com:|https://github.com/|g' src/rmw_iceoryx2/iceoryx.repos > _deps.repos - vcs import src < _deps.repos - - - name: Build with colcon - run: | - cd $WORKSPACE_DIR - . /opt/ros/rolling/setup.sh - colcon build \ - --cmake-args \ - -DCMAKE_C_COMPILER=clang \ - -DCMAKE_CXX_COMPILER=clang++ \ - -DCMAKE_BUILD_TYPE=RelWithDebInfo \ - -DBUILD_TESTING=On \ - --symlink-install \ - --packages-select \ - test_interface_files \ - iceoryx_platform\ - iceoryx_hoofs \ - iceoryx2_cxx \ - rmw_iceoryx2_cxx_test_msgs \ - rmw_iceoryx2_cxx - - - name: Run tests - run: | - cd $WORKSPACE_DIR - . /opt/ros/rolling/setup.sh - . install/setup.sh - colcon test \ - --packages-select \ - rmw_iceoryx2_cxx \ - --event-handlers console_direct+ \ - --return-code-on-test-failure - - - name: Generate test reports - if: always() - run: | - cd $WORKSPACE_DIR - colcon test-result --verbose - - - name: Upload test results - if: always() - uses: actions/upload-artifact@v4 - with: - name: test-results - path: | - ${{ env.WORKSPACE_DIR }}/build/*/test_results/*/*.xml - ${{ env.WORKSPACE_DIR }}/log/**/test*.log + - name: Install common build dependencies + run: | + apt-get update + apt-get install -y \ + python3-colcon-common-extensions \ + python3-colcon-mixin \ + python3-vcstool \ + python3-rosdep \ + libacl1-dev \ + software-properties-common \ + curl \ + wget + + - name: Install rust toolchain + uses: dtolnay/rust-toolchain@stable + + - name: Setup workspace + run: | + mkdir -p $WORKSPACE_DIR/src/rmw_iceoryx2 + mkdir -p $WORKSPACE_DIR/src/test_interface_files + + - name: Checkout test message interfaces + uses: actions/checkout@v4 + with: + repository: ros2/test_interface_files + path: src/test_interface_files + ref: rolling + + - name: Checkout current ref + uses: actions/checkout@v4 + with: + path: src/rmw_iceoryx2 + + - name: Install compiler toolchain + if: matrix.compiler == 'clang' + uses: ./src/rmw_iceoryx2/.github/actions/install-clang + + - name: Checkout iceoryx + run: | + cd $WORKSPACE_DIR + + # need to switch to https in ci + sed 's|git@github.com:|https://github.com/|g' src/rmw_iceoryx2/iceoryx.repos > _deps.repos + vcs import src < _deps.repos + + - name: Build workspace + run: | + cd $WORKSPACE_DIR + . /opt/ros/rolling/setup.sh + colcon build \ + --cmake-args \ + -DCMAKE_C_COMPILER=${{ matrix.cc }} \ + -DCMAKE_CXX_COMPILER=${{ matrix.cxx }} \ + -DCMAKE_BUILD_TYPE=RelWithDebInfo \ + -DBUILD_TESTING=On \ + --symlink-install \ + --packages-select \ + test_interface_files \ + iceoryx_platform \ + iceoryx_hoofs \ + iceoryx2_cxx \ + rmw_iceoryx2_cxx_test_msgs \ + rmw_iceoryx2_cxx + + - name: Run tests + run: | + cd $WORKSPACE_DIR + . /opt/ros/rolling/setup.sh + . install/setup.sh + colcon test \ + --packages-select rmw_iceoryx2_cxx \ + --event-handlers console_direct+ \ + --return-code-on-test-failure + + - name: Generate test reports + if: always() + run: | + cd $WORKSPACE_DIR + colcon test-result --verbose + + - name: Upload test results + if: always() + uses: actions/upload-artifact@v4 + with: + name: test-results-${{ matrix.compiler }} + path: | + ${{ env.WORKSPACE_DIR }}/build/*/test_results/*/*.xml + ${{ env.WORKSPACE_DIR }}/log/**/test*.log From b0577b59f4ffe27d16c35a59a2f100a1b13a630d Mon Sep 17 00:00:00 2001 From: Jeff Ithier Date: Thu, 19 Dec 2024 17:45:21 +1100 Subject: [PATCH 2/4] [#15] Add GCC build to CI --- .github/workflows/build-test.yml | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml index 448a582..7abd365 100644 --- a/.github/workflows/build-test.yml +++ b/.github/workflows/build-test.yml @@ -74,12 +74,16 @@ jobs: colcon-build-test: needs: [preflight-check, static-code-analysis] strategy: + fail-fast: false matrix: - compiler: [clang] + compiler: [clang, gcc] include: - compiler: clang cc: clang cxx: clang++ + - compiler: gcc + cc: gcc + cxx: g++ runs-on: ubuntu-latest container: image: ros:rolling @@ -117,8 +121,8 @@ jobs: with: path: src/rmw_iceoryx2 + # this is also required for all builds as iceoryx2 depends on libclang - name: Install compiler toolchain - if: matrix.compiler == 'clang' uses: ./src/rmw_iceoryx2/.github/actions/install-clang - name: Checkout iceoryx From 0195fd541bff084351e16387c69da2e36f6c6dcf Mon Sep 17 00:00:00 2001 From: Jeff Ithier Date: Thu, 19 Dec 2024 18:26:55 +1100 Subject: [PATCH 3/4] [#15] Fix GCC build --- .github/actions/install-clang/action.yml | 4 +- .github/actions/install-gcc/action.yml | 12 ++++++ .github/workflows/build-test.yml | 26 ++++++++---- .../rmw_iceoryx2_cxx/iox2/iceoryx2.hpp | 8 ++-- rmw_iceoryx2_cxx/src/rmw/graph.cpp | 4 +- rmw_iceoryx2_cxx/test/test_rmw_allocator.cpp | 42 ++++++++++--------- .../test/test_rmw_guard_condition.cpp | 2 +- rmw_iceoryx2_cxx/test/test_rmw_waitset.cpp | 8 ++-- 8 files changed, 66 insertions(+), 40 deletions(-) create mode 100644 .github/actions/install-gcc/action.yml diff --git a/.github/actions/install-clang/action.yml b/.github/actions/install-clang/action.yml index 1d875aa..7106015 100644 --- a/.github/actions/install-clang/action.yml +++ b/.github/actions/install-clang/action.yml @@ -1,8 +1,8 @@ -name: 'Install clang toolchain' +name: 'Install clang' runs: using: "composite" steps: - - name: Install iceoryx dependencies and clang-tidy + - name: Install clang shell: bash run: | sudo wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key | sudo apt-key add - diff --git a/.github/actions/install-gcc/action.yml b/.github/actions/install-gcc/action.yml new file mode 100644 index 0000000..7c79413 --- /dev/null +++ b/.github/actions/install-gcc/action.yml @@ -0,0 +1,12 @@ +name: 'Install GCC' +runs: + using: "composite" + steps: + - name: Install GCC + shell: bash + run: | + apt-get update + apt-get install -y software-properties-common + add-apt-repository -y ppa:ubuntu-toolchain-r/test + apt-get update + apt-get install -y gcc-13 g++-13 diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml index 7abd365..927783d 100644 --- a/.github/workflows/build-test.yml +++ b/.github/workflows/build-test.yml @@ -18,7 +18,7 @@ jobs: source-code: ${{ steps.filter.outputs.source-code }} markdown: ${{ steps.filter.outputs.markdown }} steps: - - name: Checkout sources + - name: Check out sources uses: actions/checkout@v4 - name: Check for changed file types @@ -37,7 +37,7 @@ jobs: timeout-minutes: 10 runs-on: ubuntu-latest steps: - - name: Checkout sources + - name: Check out sources uses: actions/checkout@v4 - name: Check format of all commit messages @@ -52,7 +52,7 @@ jobs: timeout-minutes: 10 runs-on: ubuntu-latest steps: - - name: Checkout sources + - name: Check out sources uses: actions/checkout@v4 - name: Install clang toolchain @@ -109,23 +109,27 @@ jobs: mkdir -p $WORKSPACE_DIR/src/rmw_iceoryx2 mkdir -p $WORKSPACE_DIR/src/test_interface_files - - name: Checkout test message interfaces + - name: Check out test message interfaces uses: actions/checkout@v4 with: repository: ros2/test_interface_files path: src/test_interface_files ref: rolling - - name: Checkout current ref + - name: Check out current ref uses: actions/checkout@v4 with: path: src/rmw_iceoryx2 # this is also required for all builds as iceoryx2 depends on libclang - - name: Install compiler toolchain + - name: Install clang toolchain uses: ./src/rmw_iceoryx2/.github/actions/install-clang - - name: Checkout iceoryx + - name: Install gcc toolchain + if: matrix.compiler == 'gcc' + uses: ./src/rmw_iceoryx2/.github/actions/install-gcc + + - name: Import iceoryx run: | cd $WORKSPACE_DIR @@ -133,6 +137,13 @@ jobs: sed 's|git@github.com:|https://github.com/|g' src/rmw_iceoryx2/iceoryx.repos > _deps.repos vcs import src < _deps.repos + - name: Check compiler versions + run: | + echo "C compiler version:" + ${{ matrix.cc }} --version + echo "C++ compiler version:" + ${{ matrix.cxx }} --version + - name: Build workspace run: | cd $WORKSPACE_DIR @@ -159,7 +170,6 @@ jobs: . install/setup.sh colcon test \ --packages-select rmw_iceoryx2_cxx \ - --event-handlers console_direct+ \ --return-code-on-test-failure - name: Generate test reports diff --git a/rmw_iceoryx2_cxx/include/rmw_iceoryx2_cxx/iox2/iceoryx2.hpp b/rmw_iceoryx2_cxx/include/rmw_iceoryx2_cxx/iox2/iceoryx2.hpp index 0f718db..4ccd6c5 100644 --- a/rmw_iceoryx2_cxx/include/rmw_iceoryx2_cxx/iox2/iceoryx2.hpp +++ b/rmw_iceoryx2_cxx/include/rmw_iceoryx2_cxx/iox2/iceoryx2.hpp @@ -142,15 +142,15 @@ class RMW_PUBLIC Iceoryx2 // =================================================================================================================== -template <::iox2::ServiceType ServiceType> -auto Iceoryx2::service_builder(const std::string& service_name) -> ::iox2::ServiceBuilder { +template <::iox2::ServiceType S> +auto Iceoryx2::service_builder(const std::string& service_name) -> ::iox2::ServiceBuilder { auto name = ::iox2::ServiceName::create(service_name.c_str()); if (name.has_error()) { // TODO: propagate error } - if constexpr (ServiceType == ::iox2::ServiceType::Local) { + if constexpr (S == ::iox2::ServiceType::Local) { return local().service_builder(name.value()); - } else if constexpr (ServiceType == ::iox2::ServiceType::Ipc) { + } else if constexpr (S == ::iox2::ServiceType::Ipc) { return ipc().service_builder(name.value()); } else { static_assert(std::false_type::value, "Attempted to build a service of unknown type"); diff --git a/rmw_iceoryx2_cxx/src/rmw/graph.cpp b/rmw_iceoryx2_cxx/src/rmw/graph.cpp index 616581c..787a5f9 100644 --- a/rmw_iceoryx2_cxx/src/rmw/graph.cpp +++ b/rmw_iceoryx2_cxx/src/rmw/graph.cpp @@ -352,7 +352,7 @@ rmw_ret_t rmw_get_publishers_info_by_topic(const rmw_node_t* node, if (!rcutils_allocator_is_valid(allocator)) { return RMW_RET_INVALID_ARGUMENT; } - if (auto result = rmw_topic_endpoint_info_array_check_zero(publishers_info) != RMW_RET_OK) { + if (rmw_topic_endpoint_info_array_check_zero(publishers_info) != RMW_RET_OK) { return RMW_RET_INVALID_ARGUMENT; } @@ -443,7 +443,7 @@ rmw_ret_t rmw_get_subscriptions_info_by_topic(const rmw_node_t* node, if (!rcutils_allocator_is_valid(allocator)) { return RMW_RET_INVALID_ARGUMENT; } - if (auto result = rmw_topic_endpoint_info_array_check_zero(subscriptions_info) != RMW_RET_OK) { + if (rmw_topic_endpoint_info_array_check_zero(subscriptions_info) != RMW_RET_OK) { return RMW_RET_INVALID_ARGUMENT; } diff --git a/rmw_iceoryx2_cxx/test/test_rmw_allocator.cpp b/rmw_iceoryx2_cxx/test/test_rmw_allocator.cpp index 8bba5ef..c284d0a 100644 --- a/rmw_iceoryx2_cxx/test/test_rmw_allocator.cpp +++ b/rmw_iceoryx2_cxx/test/test_rmw_allocator.cpp @@ -17,18 +17,6 @@ namespace using namespace rmw::iox2::testing; -class DummyClass -{ -public: - DummyClass() - : value(42) { - } - ~DummyClass() { - value = 0; - } - int value; -}; - class AllocatorHelpersTest : public TestBase { protected: @@ -113,21 +101,37 @@ TEST_F(AllocatorHelpersTest, deallocate_nullptr) { ASSERT_NO_THROW(rmw::iox2::deallocate(ptr)); } +class DummyClass +{ +public: + DummyClass() { + ++constructor_calls; + } + ~DummyClass() { + ++destructor_calls; + } + + static size_t constructor_calls; + static size_t destructor_calls; +}; +size_t DummyClass::constructor_calls = 0; +size_t DummyClass::destructor_calls = 0; + TEST_F(AllocatorHelpersTest, construct_and_destruct) { auto allocation = rmw::iox2::allocate(); ASSERT_TRUE(allocation.has_value()); auto ptr = allocation.value(); auto constructed = rmw::iox2::construct(ptr); - ASSERT_TRUE(constructed.has_value()); + ASSERT_FALSE(constructed.has_error()); + ASSERT_EQ(DummyClass::constructor_calls, 1u); + ASSERT_EQ(DummyClass::destructor_calls, 0u); - auto obj = constructed.value(); - ASSERT_EQ(obj->value, 42); + rmw::iox2::destruct(ptr); + ASSERT_EQ(DummyClass::constructor_calls, 1u); + ASSERT_EQ(DummyClass::destructor_calls, 1u); - rmw::iox2::destruct(obj); - ASSERT_EQ(obj->value, 0); - - rmw::iox2::deallocate(obj); + rmw::iox2::deallocate(ptr); } TEST_F(AllocatorHelpersTest, destruct_nullptr) { diff --git a/rmw_iceoryx2_cxx/test/test_rmw_guard_condition.cpp b/rmw_iceoryx2_cxx/test/test_rmw_guard_condition.cpp index 77f10c3..445321d 100644 --- a/rmw_iceoryx2_cxx/test/test_rmw_guard_condition.cpp +++ b/rmw_iceoryx2_cxx/test/test_rmw_guard_condition.cpp @@ -59,7 +59,7 @@ class RmwGuardConditionTest : public TestBase auto listener = service.listener_builder().create().expect("failed to create test listener"); return listener; - }; + } private: iox::optional<::rmw::iox2::Iceoryx2> m_iox2; diff --git a/rmw_iceoryx2_cxx/test/test_rmw_waitset.cpp b/rmw_iceoryx2_cxx/test/test_rmw_waitset.cpp index 12fbe00..f694638 100644 --- a/rmw_iceoryx2_cxx/test/test_rmw_waitset.cpp +++ b/rmw_iceoryx2_cxx/test/test_rmw_waitset.cpp @@ -56,14 +56,14 @@ class WaitSetTestContext ~WaitSetTestContext() { for (size_t i = 0; i < m_subscriptions.size(); i++) { - (void)rmw_destroy_publisher(m_rmw_node, m_publishers.at(i).publisher); - (void)rmw_destroy_subscription(m_rmw_node, m_subscriptions.at(i).subscriber); + EXPECT_RMW_OK(rmw_destroy_publisher(m_rmw_node, m_publishers.at(i).publisher)); + EXPECT_RMW_OK(rmw_destroy_subscription(m_rmw_node, m_subscriptions.at(i).subscriber)); } for (size_t i = 0; i < m_guard_conditions.size(); i++) { - (void)rmw_destroy_guard_condition(m_guard_conditions.at(i)); + EXPECT_RMW_OK(rmw_destroy_guard_condition(m_guard_conditions.at(i))); } if (m_waitset) { - (void)rmw_destroy_wait_set(m_waitset); + EXPECT_RMW_OK(rmw_destroy_wait_set(m_waitset)); } } From 0cf82c02e214844d04744aec52dee1bbf7e0b98b Mon Sep 17 00:00:00 2001 From: Jeff Ithier Date: Sat, 21 Dec 2024 14:23:53 +1100 Subject: [PATCH 4/4] [#15] Add release notes --- doc/release-notes/unreleased.md | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/doc/release-notes/unreleased.md b/doc/release-notes/unreleased.md index 72c4d6e..c5a179e 100644 --- a/doc/release-notes/unreleased.md +++ b/doc/release-notes/unreleased.md @@ -11,8 +11,6 @@ conflicts when merging. --> -* Example text [#1](https://github.com/ekxide/rmw_iceoryx2/issues/1) - ### Bugfixes -* Example text [#1](https://github.com/ekxide/rmw_iceoryx2/issues/1) +* Fix failing `gcc` build [#15](https://github.com/ekxide/rmw_iceoryx2/issues/15) ### Refactoring @@ -29,8 +27,6 @@ conflicts when merging. --> -* Example text [#1](https://github.com/ekxide/rmw_iceoryx2/issues/1) - ### Workflow -* Use VCS to manage dependencies [#13](https://github.com/ekxide/rmw_iceoryx2/issues/13) -* Add CI workflow for building and testing [#12](https://github.com/ekxide/rmw_iceoryx2/issues/12) +* Use `vcs` to manage dependencies [#13](https://github.com/ekxide/rmw_iceoryx2/issues/13) +* Add CI for building and testing with `clang` [#12](https://github.com/ekxide/rmw_iceoryx2/issues/12) +* Add CI for building and testing with `gcc` [#15](https://github.com/ekxide/rmw_iceoryx2/issues/15) ### New API features