Merge branch 'main' into houliston/priority-changes

TrentHouliston · web-flow · commit 2c65b6cf40c9 · 2025-03-06T14:48:21.000+11:00
diff --git a/.github/workflows/sonarcloud.yaml b/.github/workflows/sonarcloud.yaml
@@ -19,18 +19,20 @@ jobs:
   sonarcloud:
     name: SonarCloud
     runs-on: ubuntu-24.04
+    container: silkeh/clang:18
     env:
       BUILD_WRAPPER_OUT_DIR: build_wrapper_output_directory # Directory where build-wrapper output will be placed
     steps:
       - uses: actions/checkout@v4
         with:
           fetch-depth: 0 # Shallow clones should be disabled for a better relevancy of analysis
 
-      - name: Install gcovr
-        run: pip install gcovr==8.2
-
-      - name: Install sonar-scanner and build-wrapper
-        uses: SonarSource/sonarcloud-github-c-cpp@v3
+      - name: Install Prerequisites
+        run: |
+          apt-get update
+          apt-get install -y \
+            ccache \
+            unzip
 
       - name: Install CMake
         uses: lukka/get-cmake@v3.30.5
@@ -53,66 +55,54 @@ jobs:
             -GNinja \
             -DCMAKE_C_COMPILER_LAUNCHER=ccache \
             -DCMAKE_CXX_COMPILER_LAUNCHER=ccache \
-            -DCMAKE_BUILD_TYPE=Debug \
+            -DCMAKE_C_COMPILER=clang \
+            -DCMAKE_CXX_COMPILER=clang++ \
+            -DCMAKE_BUILD_TYPE=Release \
             -DBUILD_TESTS=ON \
             -DCI_BUILD=ON \
             -DENABLE_COVERAGE=ON \
             -DENABLE_CLANG_TIDY=OFF
 
-      - name: Build the code in debug mode
+      - name: Build the code
         timeout-minutes: 30
-        run: cmake --build build/ --config Debug
+        run: cmake --build build/ --config Release
 
       - name: Add capabilities for scheduler
         run: find build/tests -type f -executable -exec sudo setcap cap_sys_nice=eip {} \; || true
 
       - name: Run tests to generate coverage statistics
         timeout-minutes: 10
         working-directory: build
-        run: ninja run_all_tests -j1 -k 0
+        run: ninja coverage -k 0
 
       - name: Test Summary
         if: ${{ !cancelled() }}
         uses: test-summary/action@v2
         with:
           paths: "build/reports/tests/*.junit.xml"
 
-      - name: Collect coverage into one XML report
-        if: ${{ !cancelled() }}
-        run: |
-          gcovr \
-            --root . \
-            --object-directory build \
-            --force-color \
-            --no-markers \
-            --decisions \
-            --calls \
-            --exclude-noncode-lines \
-            --gcov-ignore-parse-errors negative_hits.warn \
-            --sonarqube "coverage.xml"
-
       - name: Upload coverage report
         if: ${{ !cancelled() }}
         uses: actions/upload-artifact@v4
         with:
-          name: coverage-sonar
-          path: coverage.xml
+          name: coverage
+          path: build/reports/coverage.txt
           retention-days: 1 # This sets the artifact TTL to 1 day (minimum is 1 day)
           overwrite: true
 
-      - name: Run sonar-scanner
+      - name: SonarQube Scan
+        uses: SonarSource/sonarqube-scan-action@v5
         if: ${{ !cancelled() }}
         env:
-          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
           SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}
-        run: |
-          sonar-scanner \
-            --define sonar.projectKey=Fastcode_NUClear \
-            --define sonar.organization=fastcode \
-            --define sonar.sources=src \
-            --define sonar.tests=tests \
-            --define sonar.cfamily.compile-commands=build/compile_commands.json \
-            --define sonar.coverageReportPaths=coverage.xml
+        with:
+          args: >
+            --define sonar.projectKey=Fastcode_NUClear
+            --define sonar.organization=fastcode
+            --define sonar.sources=src
+            --define sonar.tests=tests
+            --define sonar.cfamily.compile-commands=build/compile_commands.json
+            --define sonar.cfamily.llvm-cov.reportPath=build/reports/coverage.txt
 
       - name: Upload Traces
         if: ${{ !cancelled() }}
diff --git a/cmake/TestRunner.cmake b/cmake/TestRunner.cmake
@@ -36,25 +36,68 @@ get_all_catch_test_targets(all_targets ${PROJECT_SOURCE_DIR})
 
 # Create a custom command for each test target to run it
 # Make sure that coverage data is written with paths relative to the source directory
-unset(reports)
+unset(report_outputs)
+unset(coverage_reports)
 foreach(target ${all_targets})
 
-  set(sonarqube_report_file "${PROJECT_BINARY_DIR}/reports/tests/${target}.sonarqube.xml")
+  unset(command_prefix)
+  if(ENABLE_COVERAGE AND CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
+    # Extra output files for the profile data
+    set(raw_coverage "${PROJECT_BINARY_DIR}/reports/tests/${target}.profraw")
+    set(indexed_coverage "${PROJECT_BINARY_DIR}/reports/tests/${target}.profdata")
+    set(coverage_report "${PROJECT_BINARY_DIR}/reports/tests/${target}.txt")
+    set(command_prefix "cmake" "-E" "env" "LLVM_PROFILE_FILE=${raw_coverage}")
+    list(APPEND coverage_reports ${coverage_report})
+  endif()
+
   set(junit_report_file "${PROJECT_BINARY_DIR}/reports/tests/${target}.junit.xml")
-  list(APPEND reports ${sonarqube_report_file} ${junit_report_file})
+  list(APPEND report_outputs ${junit_report_file})
   add_custom_command(
-    OUTPUT ${sonarqube_report_file} ${junit_report_file}
-    COMMAND $<TARGET_FILE:${target}> --reporter console --reporter SonarQube::out=${sonarqube_report_file} --reporter
-            JUnit::out=${junit_report_file}
+    OUTPUT ${junit_report_file} ${raw_coverage}
+    COMMAND ${command_prefix} $<TARGET_FILE:${target}> --reporter console --reporter JUnit::out=${junit_report_file}
     WORKING_DIRECTORY ${PROJECT_BINARY_DIR}
+    DEPENDS ${target}
     USES_TERMINAL
     COMMENT "Running test ${target}"
   )
+
+  if(ENABLE_COVERAGE AND CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
+    add_custom_command(
+      OUTPUT ${indexed_coverage}
+      COMMAND llvm-profdata merge -sparse ${raw_coverage} -o ${indexed_coverage}
+      DEPENDS ${raw_coverage}
+    )
+
+    add_custom_command(
+      OUTPUT ${coverage_report}
+      COMMAND llvm-cov show --show-branches=count -Xdemangler c++filt -instr-profile=${indexed_coverage}
+              $<TARGET_FILE:${target}> > ${coverage_report}
+      DEPENDS ${indexed_coverage}
+    )
+  endif()
+
 endforeach()
 
 # Create a custom target that depends on all test targets
 add_custom_target(
   run_all_tests
-  DEPENDS ${reports}
+  DEPENDS ${report_outputs} ${all_targets}
   COMMENT "Running all Catch2 tests"
 )
+
+# Concatenate all the coverage reports together
+if(ENABLE_COVERAGE AND CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
+
+  add_custom_command(
+    OUTPUT ${PROJECT_BINARY_DIR}/reports/coverage.txt
+    COMMAND cat ${coverage_reports} > ${PROJECT_BINARY_DIR}/reports/coverage.txt
+    DEPENDS ${coverage_reports}
+  )
+
+  add_custom_target(
+    coverage
+    DEPENDS ${PROJECT_BINARY_DIR}/reports/coverage.txt
+    COMMENT "Running all Catch2 tests with coverage"
+  )
+
+endif()
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
@@ -39,11 +39,18 @@ target_compile_features(nuclear PUBLIC cxx_std_14)
 
 option(ENABLE_COVERAGE "Compile with coverage support enabled.")
 if(ENABLE_COVERAGE)
-  if(NOT CMAKE_BUILD_TYPE STREQUAL "Debug")
-    message(WARNING "Coverage is enabled but not build in debug mode. Coverage results may be misleading.")
+  if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
+    if(NOT CMAKE_BUILD_TYPE STREQUAL "Debug")
+      message(WARNING "Coverage is enabled but not build in debug mode. Coverage results may be misleading.")
+    endif()
+    target_compile_options(nuclear PUBLIC -fprofile-arcs -ftest-coverage -fprofile-abs-path -fprofile-update=atomic)
+    target_link_options(nuclear PUBLIC -fprofile-arcs)
+  elseif(CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
+    target_compile_options(nuclear PUBLIC -fprofile-instr-generate -fcoverage-mapping)
+    target_link_options(nuclear PUBLIC -fprofile-instr-generate)
+  else()
+    message(FATAL_ERROR "Coverage is not supported for the current compiler.")
   endif()
-  target_compile_options(nuclear PUBLIC -fprofile-arcs -ftest-coverage -fprofile-abs-path -fprofile-update=atomic)
-  target_link_options(nuclear PUBLIC -fprofile-arcs)
 endif(ENABLE_COVERAGE)
 
 # Enable warnings, and all warnings are errors
diff --git a/src/dsl/word/Network.hpp b/src/dsl/word/Network.hpp
@@ -91,8 +91,8 @@ namespace dsl {
             template <typename DSL>
             static std::tuple<std::shared_ptr<NetworkSource>, NetworkData<T>> get(threading::ReactionTask& /*task*/) {
 
-                auto* data   = store::ThreadStore<std::vector<uint8_t>>::value;
-                auto* source = store::ThreadStore<NetworkSource>::value;
+                const auto* data   = store::ThreadStore<const std::vector<uint8_t>>::value;
+                const auto* source = store::ThreadStore<const NetworkSource>::value;
 
                 if (data && source) {
 
diff --git a/src/dsl/word/TCP.hpp b/src/dsl/word/TCP.hpp
@@ -66,17 +66,10 @@ namespace dsl {
 
             struct Connection {
 
-                struct Target {
-                    /// The address of the connection
-                    std::string address;
-                    /// The port of the connection
-                    uint16_t port;
-                };
-
                 /// The local address of the connection
-                Target local;
+                util::network::sock_t local;
                 /// The remote address of the connection
-                Target remote;
+                util::network::sock_t remote;
 
                 /// The file descriptor for the connection
                 fd_t fd;
@@ -187,10 +180,7 @@ namespace dsl {
                     return Connection{};
                 }
 
-                auto local_s  = local.address();
-                auto remote_s = remote.address();
-
-                return Connection{{local_s.first, local_s.second}, {remote_s.first, remote_s.second}, fd.release()};
+                return Connection{local, remote, fd.release()};
             }
         };
 
diff --git a/src/dsl/word/UDP.hpp b/src/dsl/word/UDP.hpp
@@ -102,20 +102,10 @@ namespace dsl {
                 /// If the packet is valid (it contains data)
                 bool valid{false};
 
-                struct Target {
-                    Target() = default;
-                    Target(std::string address, const uint16_t& port) : address(std::move(address)), port(port) {}
-
-                    /// The address of the target
-                    std::string address;
-                    /// The port of the target
-                    uint16_t port{0};
-                };
-
                 /// The information about this packets destination
-                Target local;
+                util::network::sock_t local{};
                 /// The information about this packets source
-                Target remote;
+                util::network::sock_t remote{};
 
                 /// The data to be sent in the packet
                 std::vector<uint8_t> payload;
@@ -427,12 +417,10 @@ namespace dsl {
                 RecvResult result = read<DSL>(task);
 
                 Packet p{};
-                p.valid       = result.valid;
-                p.payload     = std::move(result.payload);
-                auto local_s  = result.local.address();
-                auto remote_s = result.remote.address();
-                p.local       = Packet::Target{local_s.first, local_s.second};
-                p.remote      = Packet::Target{remote_s.first, remote_s.second};
+                p.valid   = result.valid;
+                p.payload = std::move(result.payload);
+                p.local   = result.local;
+                p.remote  = result.remote;
 
                 // Confirm that this packet was sent to one of our local addresses
                 for (const auto& iface : util::network::get_interfaces()) {
@@ -475,12 +463,10 @@ namespace dsl {
                     if (result.local.sock.sa_family == AF_INET) {
 
                         Packet p{};
-                        p.valid       = result.valid;
-                        p.payload     = std::move(result.payload);
-                        auto local_s  = result.local.address();
-                        auto remote_s = result.remote.address();
-                        p.local       = Packet::Target{local_s.first, local_s.second};
-                        p.remote      = Packet::Target{remote_s.first, remote_s.second};
+                        p.valid   = result.valid;
+                        p.payload = std::move(result.payload);
+                        p.local   = result.local;
+                        p.remote  = result.remote;
 
                         // 255.255.255.255 is always a valid broadcast address
                         if (result.local.ipv4.sin_addr.s_addr == htonl(INADDR_BROADCAST)) {
@@ -526,12 +512,10 @@ namespace dsl {
                     // Only return multicast packets
                     if (multicast) {
                         Packet p{};
-                        p.valid       = result.valid;
-                        p.payload     = std::move(result.payload);
-                        auto local_s  = result.local.address();
-                        auto remote_s = result.remote.address();
-                        p.local       = Packet::Target{local_s.first, local_s.second};
-                        p.remote      = Packet::Target{remote_s.first, remote_s.second};
+                        p.valid   = result.valid;
+                        p.payload = std::move(result.payload);
+                        p.local   = result.local;
+                        p.remote  = result.remote;
                         return p;
                     }
 
diff --git a/src/extension/NetworkController.cpp b/src/extension/NetworkController.cpp
@@ -57,14 +57,10 @@ namespace extension {
                                            const bool& reliable,
                                            std::vector<uint8_t>&& payload) {
             // Construct our NetworkSource information
-            dsl::word::NetworkSource src{remote.name, remote.target, reliable};
+            const dsl::word::NetworkSource src{remote.name, remote.target, reliable};
 
             // Move the payload in as we are stealing it
-            std::vector<uint8_t> p(std::move(payload));
-
-            // Store in our thread local cache
-            dsl::store::ThreadStore<std::vector<uint8_t>>::value     = &p;
-            dsl::store::ThreadStore<dsl::word::NetworkSource>::value = &src;
+            const std::vector<uint8_t> p(std::move(payload));
 
             /* Mutex Scope */ {
                 // Lock our reaction mutex
@@ -75,13 +71,17 @@ namespace extension {
 
                 // Execute on our interested reactions
                 for (auto it = rs.first; it != rs.second; ++it) {
+                    // Store in our thread local cache
+                    dsl::store::ThreadStore<const std::vector<uint8_t>>::value     = &p;
+                    dsl::store::ThreadStore<const dsl::word::NetworkSource>::value = &src;
+
                     powerplant.submit(it->second->get_task());
                 }
-            }
 
-            // Clear our cache
-            dsl::store::ThreadStore<std::vector<uint8_t>>::value     = nullptr;
-            dsl::store::ThreadStore<dsl::word::NetworkSource>::value = nullptr;
+                // Clear our cache
+                dsl::store::ThreadStore<const std::vector<uint8_t>>::value     = nullptr;
+                dsl::store::ThreadStore<const dsl::word::NetworkSource>::value = nullptr;
+            }
         });
 
         // Set our join callback
diff --git a/src/util/network/sock_t.hpp b/src/util/network/sock_t.hpp
diff --git a/tests/tests/dsl/UDP.cpp b/tests/tests/dsl/UDP.cpp
