feat: add quality.yml CI workflow with sanitizers and decoupled clang-tidy#348
feat: add quality.yml CI workflow with sanitizers and decoupled clang-tidy#348
Conversation
Clang-tidy and format linters move to quality.yml workflow.
There was a problem hiding this comment.
Pull request overview
Adds a dedicated “Quality” CI workflow to run Jazzy-only formatting/linting, incremental clang-tidy, and sanitizer (ASan/UBSan + TSan) unit-test runs, while simplifying the existing CI workflow by removing Jazzy clang-tidy/lint steps.
Changes:
- Introduces
.github/workflows/quality.ymlwith parallel jobs for format-lint, clang-tidy (with ctcache), ASan+UBSan, and TSan. - Adds
ROS2MedkitSanitizers.cmakeand wires it into several packages to enable-DSANITIZER=...builds. - Removes Jazzy clang-tidy/lint coupling from
.github/workflows/ci.ymland adds a TSan suppressions file.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tsan_suppressions.txt |
Adds CI-referenced TSan suppressions for known ROS 2/DDS false positives. |
src/ros2_medkit_serialization/CMakeLists.txt |
Includes new sanitizer module for per-package sanitizer flag injection. |
src/ros2_medkit_gateway/CMakeLists.txt |
Includes new sanitizer module for gateway builds/tests. |
src/ros2_medkit_fault_reporter/CMakeLists.txt |
Includes new sanitizer module for sanitizer-enabled builds/tests. |
src/ros2_medkit_fault_manager/CMakeLists.txt |
Includes new sanitizer module for sanitizer-enabled builds/tests. |
src/ros2_medkit_discovery_plugins/ros2_medkit_linux_introspection/CMakeLists.txt |
Includes new sanitizer module for sanitizer-enabled builds/tests. |
src/ros2_medkit_diagnostic_bridge/CMakeLists.txt |
Includes new sanitizer module for sanitizer-enabled builds/tests. |
src/ros2_medkit_cmake/CMakeLists.txt |
Installs the new sanitizer CMake module. |
src/ros2_medkit_cmake/cmake/ROS2MedkitSanitizers.cmake |
Implements -DSANITIZER=... parsing/validation and applies sanitizer flags. |
src/ros2_medkit_cmake/cmake/ros2_medkit_cmake-extras.cmake |
Updates documentation comment to mention the sanitizer module. |
.github/workflows/quality.yml |
New quality workflow running format-lint, clang-tidy, ASan+UBSan, and TSan jobs. |
.github/workflows/ci.yml |
Removes clang-tidy from Jazzy build and deletes the Jazzy lint job; updates gating dependencies. |
| # Full: all files (filter to our source only) | ||
| run-clang-tidy -p build -clang-tidy-binary clang-tidy-cache \ | ||
| -header-filter='.*ros2_medkit.*' \ | ||
| 'src/ros2_medkit_.*/src/.*\.cpp$' |
There was a problem hiding this comment.
In the “Full” path, run-clang-tidy is passed a regex-like string as a positional argument. run-clang-tidy typically expects either explicit file paths or a -regex selector; passing a regex as a file argument will likely analyze nothing or fail. Consider switching this to the appropriate -regex option (or enumerating files with find and passing the list) so pushes to main actually run a full analysis.
| 'src/ros2_medkit_.*/src/.*\.cpp$' | |
| -regex='src/ros2_medkit_.*/src/.*\.cpp$' |
| endforeach() | ||
| list(JOIN _FSANITIZE_FLAGS "," _FSANITIZE_VALUE) | ||
|
|
||
| add_compile_options(-fsanitize=${_FSANITIZE_VALUE} -fno-omit-frame-pointer -O1) |
There was a problem hiding this comment.
This module unconditionally adds -O1 when sanitizers are enabled. That can override the chosen CMAKE_BUILD_TYPE (e.g., Debug’s -O0) and makes sanitizer builds less predictable. Consider not forcing an optimization level here (or making it configurable / conditional) and leave optimization control to the build type/toolchain.
| add_compile_options(-fsanitize=${_FSANITIZE_VALUE} -fno-omit-frame-pointer -O1) | |
| add_compile_options(-fsanitize=${_FSANITIZE_VALUE} -fno-omit-frame-pointer) |
| add_link_options(-fsanitize=${_FSANITIZE_VALUE}) | ||
|
|
There was a problem hiding this comment.
add_link_options() requires relatively recent CMake (introduced after older 3.x releases). Since the packages declare cmake_minimum_required(VERSION 3.8), this can break users/build agents running older CMake even if they otherwise satisfy the declared minimum. Either bump the project-wide minimum CMake version accordingly, or add a compatibility fallback (e.g., using target_link_options where possible, or appending to linker flags for older CMake).
| add_link_options(-fsanitize=${_FSANITIZE_VALUE}) | |
| if(COMMAND add_link_options) | |
| add_link_options(-fsanitize=${_FSANITIZE_VALUE}) | |
| else() | |
| foreach(_LINKER_FLAG_VAR | |
| CMAKE_EXE_LINKER_FLAGS | |
| CMAKE_SHARED_LINKER_FLAGS | |
| CMAKE_MODULE_LINKER_FLAGS) | |
| set(${_LINKER_FLAG_VAR} "${${_LINKER_FLAG_VAR}} -fsanitize=${_FSANITIZE_VALUE}") | |
| endforeach() | |
| endif() |
…containers - PyPI package is 'ctcache', not 'clang-tidy-cache' - Use shell expansion for TSAN_OPTIONS suppressions path to resolve correctly inside GitHub Actions containers (github.workspace gives host path, not container path)
Pull Request
Summary
quality.ymlworkflow with 4 parallel Jazzy-only jobs: format-lint, incremental clang-tidy (with ctcache), ASan+UBSan, and TSanROS2MedkitSanitizers.cmakemodule for-DSANITIZER=asan,ubsan/-DSANITIZER=tsanIssue
Type
Testing
-fsanitize=address,undefinedflags confirmed in build outputnew_delete_type_mismatch=0to suppress known ROS 2/DDS allocator mismatchesChecklist