From 8358563717516557a244be98f67baf18aeed21bf Mon Sep 17 00:00:00 2001 From: ISP akm Date: Thu, 27 Jun 2024 15:29:23 +0900 Subject: [PATCH] feat: add cppcheck into ci (#291) * get cppcheck suppressions file Signed-off-by: ISP akm * change NOLINT->cppcheck-suppress Signed-off-by: ISP akm * NOLINT->cppcheck-suppress Signed-off-by: ISP akm * add cppcheck-all-files and remove clang-tidy-differential Signed-off-by: ISP akm * add NOLINT Signed-off-by: ISP akm * move suppression comments Signed-off-by: ISP akm * move suppression comments Signed-off-by: ISP akm * move suppression comments Signed-off-by: ISP akm * move suppression comments Signed-off-by: ISP akm * move suppression comments Signed-off-by: ISP akm * move suppression comments Signed-off-by: ISP akm * Update cppcheck-differential.yaml * Update trace_node.cpp * temporary change Signed-off-by: ISP akm * change .cppcheck-suppressions download timing Signed-off-by: ISP akm * add LF to build-and-test-differential.yaml Signed-off-by: ISP akm * ci(pre-commit): autofix * modify cppcheck-differential Signed-off-by: ISP akm * formatting cppcheck-differential Signed-off-by: ISP akm * modify cppcheck-differential Signed-off-by: ISP akm * add install snapd Signed-off-by: ISP akm * fix workflow version Signed-off-by: ISP akm * cppcheck is installed via apt-get Signed-off-by: ISP akm * remove snapd and ros2 container Signed-off-by: ISP akm * fix files variable setting Signed-off-by: ISP akm * fix files variable setting Signed-off-by: ISP akm * restore clang-tidy-differential Signed-off-by: ISP akm --------- Signed-off-by: ISP akm Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- .github/workflows/cppcheck-all-files.yaml | 63 ++++++++++++++ .github/workflows/cppcheck-differential.yaml | 83 +++++++++++++++++++ .../include/caret_trace/data_container.hpp | 6 +- .../include/caret_trace/lttng_session.hpp | 6 +- .../include/caret_trace/trace_node.hpp | 2 +- CARET_trace/src/trace_node.cpp | 2 + CARET_trace/test/test_scenario.cpp | 6 ++ 7 files changed, 161 insertions(+), 7 deletions(-) create mode 100644 .github/workflows/cppcheck-all-files.yaml create mode 100644 .github/workflows/cppcheck-differential.yaml diff --git a/.github/workflows/cppcheck-all-files.yaml b/.github/workflows/cppcheck-all-files.yaml new file mode 100644 index 00000000..0ae5c4a4 --- /dev/null +++ b/.github/workflows/cppcheck-all-files.yaml @@ -0,0 +1,63 @@ +name: cppcheck-all-files + +on: + pull_request: + workflow_dispatch: + +jobs: + cppcheck-all-files: + runs-on: ubuntu-latest + steps: + - name: Check out repository + uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Install dependencies + run: | + sudo apt-get update + sudo apt-get install -y wget + + # cppcheck from apt does not yet support --check-level args, and thus install from snap + - name: Install Cppcheck from snap + run: | + sudo snap install cppcheck + + # Download the cppcheck suppression file + - name: Download cppcheck suppression file + run: | + wget https://raw.githubusercontent.com/autowarefoundation/autoware.universe/main/.cppcheck_suppressions -O .cppcheck_suppressions + + - name: Get all C++ files + id: all-files + run: | + find . -type f \( -name "*.cpp" -o -name "*.hpp" \) > all_files.txt + cat all_files.txt + + - name: Run Cppcheck on all files + continue-on-error: true + id: cppcheck + run: | + files=$(cat all_files.txt) + if [ -n "$files" ]; then + echo "Running Cppcheck on all files: $files" + cppcheck --inline-suppr --enable=all --inconclusive --check-level=exhaustive --error-exitcode=1 --suppressions-list=.cppcheck_suppressions $files 2> cppcheck-report.txt + else + echo "No C++ files found." + touch cppcheck-report.txt + fi + shell: bash + + - name: Show cppcheck-report result + run: | + cat cppcheck-report.txt + + - name: Upload Cppcheck report + uses: actions/upload-artifact@v4 + with: + name: cppcheck-report + path: cppcheck-report.txt + + - name: Fail the job if Cppcheck failed + if: ${{ steps.cppcheck.outcome == 'failure' }} + run: exit 1 diff --git a/.github/workflows/cppcheck-differential.yaml b/.github/workflows/cppcheck-differential.yaml new file mode 100644 index 00000000..84c2425e --- /dev/null +++ b/.github/workflows/cppcheck-differential.yaml @@ -0,0 +1,83 @@ +name: cppcheck-differential + +on: + pull_request: + +jobs: + cppcheck-differential: + runs-on: ubuntu-latest + steps: + - name: Check out repository + uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Install dependencies + run: | + sudo apt-get update + sudo apt-get install -y wget + + # cppcheck from apt does not yet support --check-level args, and thus install from snap + - name: Install Cppcheck from snap + run: | + sudo snap install cppcheck + + # Download the cppcheck suppression file + - name: Download cppcheck suppression file + run: | + wget https://raw.githubusercontent.com/autowarefoundation/autoware.universe/main/.cppcheck_suppressions -O .cppcheck_suppressions + + - name: Remove exec_depend + uses: autowarefoundation/autoware-github-actions/remove-exec-depend@v1 + + - name: Get modified packages + id: get-modified-packages + uses: autowarefoundation/autoware-github-actions/get-modified-packages@v1 + + # Display the modified packages + - name: Show modified packages + run: | + echo "Modified packages:" + echo "${{ steps.get-modified-packages.outputs.modified_packages }}" + + - name: Get modified files + id: get-modified-files + uses: tj-actions/changed-files@v44 + with: + files: | + **/*.cpp + **/*.hpp + + # Display the modified files + - name: Show modified files + run: | + echo "Modified files:" + echo "${{ steps.get-modified-files.outputs.all_changed_files }}" + + - name: Run Cppcheck on changed files + continue-on-error: true + id: cppcheck + run: | + files=$(echo ${{ steps.get-modified-files.outputs.all_changed_files }}) + if [ -n "$files" ]; then + echo "Running Cppcheck on changed files: $files" + cppcheck --inline-suppr --enable=all --inconclusive --check-level=exhaustive --error-exitcode=1 --suppressions-list=.cppcheck_suppressions $files 2> cppcheck-report.txt + else + echo "No C++ files changed." + touch cppcheck-report.txt + fi + shell: bash + + - name: Show cppcheck-report result + run: | + cat cppcheck-report.txt + + - name: Upload Cppcheck report + uses: actions/upload-artifact@v4 + with: + name: cppcheck-report + path: cppcheck-report.txt + + - name: Fail the job if Cppcheck failed + if: steps.cppcheck.outcome == 'failure' + run: exit 1 diff --git a/CARET_trace/include/caret_trace/data_container.hpp b/CARET_trace/include/caret_trace/data_container.hpp index be51efe8..dc1902e5 100644 --- a/CARET_trace/include/caret_trace/data_container.hpp +++ b/CARET_trace/include/caret_trace/data_container.hpp @@ -186,11 +186,11 @@ class DataContainer : public DataContainerInterface std::shared_ptr rclcpp_ipb_to_subscription, std::shared_ptr rmw_implementation); - bool record(uint64_t loop_count = 1); + bool record(uint64_t loop_count = 1) override; - void start_recording(); + void start_recording() override; - void reset(); + void reset() override; /// @brief Store data for add_callback_group trace points. /// @tparam ...Args Data types to be stored. diff --git a/CARET_trace/include/caret_trace/lttng_session.hpp b/CARET_trace/include/caret_trace/lttng_session.hpp index 4372f76e..9c3cb0d7 100644 --- a/CARET_trace/include/caret_trace/lttng_session.hpp +++ b/CARET_trace/include/caret_trace/lttng_session.hpp @@ -46,10 +46,10 @@ class LttngSessionImpl : public LttngSession public: LttngSessionImpl() : started_session_running_(is_session_running()) {} - ~LttngSessionImpl() {} + ~LttngSessionImpl() override {} - bool is_session_running() const; - bool started_session_running() const; + bool is_session_running() const override; + bool started_session_running() const override; private: mutable std::mutex mtx_; diff --git a/CARET_trace/include/caret_trace/trace_node.hpp b/CARET_trace/include/caret_trace/trace_node.hpp index 949c6596..0e03eef8 100644 --- a/CARET_trace/include/caret_trace/trace_node.hpp +++ b/CARET_trace/include/caret_trace/trace_node.hpp @@ -78,7 +78,7 @@ class TraceNode : public rclcpp::Node, public TraceNodeInterface rclcpp::Logger::Level level = rclcpp::Logger::Level::Info, bool use_log = false, bool execute_timer_on_run = true); - ~TraceNode(); + ~TraceNode() override; /// @brief Check whther current status allows recording. /// @return True if recording is allowed, false otherwise. diff --git a/CARET_trace/src/trace_node.cpp b/CARET_trace/src/trace_node.cpp index d1900fae..eef89d0c 100644 --- a/CARET_trace/src/trace_node.cpp +++ b/CARET_trace/src/trace_node.cpp @@ -166,12 +166,14 @@ bool TraceNode::is_recording_allowed() const bool TraceNode::is_recording_allowed_init() const { return true; +#if 0 // ignore following std::shared_lock lock(mutex_); // NOTE: Since PREPARE to RECORD is a continuous state transition // with a return value of TRUE, no mutex is required. // On the other hand, the transition to the PREPARE state is a boundary, so a mutex is required. return status_ == TRACE_STATUS::RECORD || status_ == TRACE_STATUS::PREPARE; +#endif } const TRACE_STATUS & TraceNode::get_status() const diff --git a/CARET_trace/test/test_scenario.cpp b/CARET_trace/test/test_scenario.cpp index 2383a0db..3660f67d 100644 --- a/CARET_trace/test/test_scenario.cpp +++ b/CARET_trace/test/test_scenario.cpp @@ -38,11 +38,13 @@ using rclcpp::Logger; void add_data(DataContainer & container, int loop) { + // cppcheck-suppress-begin nullPointerArithmetic char * ptr = nullptr; for (auto i = 0; i < loop; i++) { ptr++; container.store_rcl_init(ptr, 0); } + // cppcheck-suppress-end nullPointerArithmetic } void record_data(DataContainer & container, int loop) @@ -66,11 +68,13 @@ TEST(ScenarioTest, TestSingleThread) auto loop = 1000; + // cppcheck-suppress-begin nullPointerArithmetic char * ptr = nullptr; for (auto i = 0; i < loop; i++) { ptr++; EXPECT_CALL(rcl_init_mock, Call(ptr, 0)).Times(1); } + // cppcheck-suppress-end nullPointerArithmetic add_data(container, loop); container.record(loop); @@ -92,11 +96,13 @@ TEST(ScenarioTest, TestMultiThread) auto loop = 1000; + // cppcheck-suppress-begin nullPointerArithmetic char * ptr = nullptr; for (auto i = 0; i < loop; i++) { ptr++; EXPECT_CALL(rcl_init_mock, Call(ptr, 0)).WillRepeatedly(Return()); } + // cppcheck-suppress-end nullPointerArithmetic // NOTE: Ensure recording data. Avoid container.record() before add_data() is called. add_data(container, 1);