Skip to content

Fix clang-tidy and sanitizer CI config blocking score/time_daemon checks - #216

Open
MaciejSalwa543 wants to merge 9 commits into
eclipse-score:mainfrom
qorix-group:fix-sca-sanitizer-ci-config-77
Open

MaciejSalwa543 wants to merge 9 commits into
eclipse-score:mainfrom
qorix-group:fix-sca-sanitizer-ci-config-77

Conversation

@MaciejSalwa543

@MaciejSalwa543 MaciejSalwa543 commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Description

Starting on #77, found that QA / Clang-Tidy and QA / Sanitizers show green but aren't
actually checking anything.

Clang-Tidy: under --config=clang-tidy, most targets fail to build before the aspect
runs (clang: error: argument unused during compilation: '-stdlib=libc++' [-Werror,-Wunused-command-line-argument]). MODULE.bazel defines two LLVM toolchains; the
one used here never sets stdlib, unlike the newer llvm_toolchain_coverage one. Fix:
mirror stdlib = {"": "stdc++"} onto it.

Sanitizers: under --config=asan_ubsan_lsan, every test is silently skipped
(Executed 0 out of 22 tests: 22 were skipped). .bazelrc set
--@score_cpp_policies//sanitizers/flags:sanitizer=asan_ubsan_lsan, a flag removed in the
score_cpp_policies commit this repo pins — real API is three bool flags (:asan, :ubsan,
:lsan). Since none were set, the sanitizer run_under wrapper is incompatible and every
test gets skipped. Fix: set the three bool flags.

Both look like they've been broken since #102 (2026-07-03), not a recent regression —
continue-on-error: true on both jobs hid it.

Turns out that wasn't the whole story. Once the build actually worked, digging further
showed clang-tidy was still only running its own narrow built-in default (clang-diagnostic-*,
clang-analyzer-*) instead of the S-CORE baseline — the baseline lives in
score_cpp_policies, but external/ is a sibling of score/, not an ancestor, so clang-tidy's
own directory-walk config discovery could never reach it. Mirrored the baseline into
//:.clang-tidy and wired it via local_configs. That surfaced 552 real findings in
score/time_daemon/ that had never actually been checked — fixed what's genuinely fixable
(missing includes, redundant/missing default member inits, magic numbers, pass-by-value+move,
const-correctness, trailing return types, a duplicated global turned into a member) and added
specific justification comments for the suppressions that are the right call.

Results

  • bazel test --config=clang-tidy //score/time_daemon/... → 22/22 pass, 0 clang-tidy findings
    (full S-CORE baseline, not just the built-in default).
  • bazel test --config=asan_ubsan_lsan --config=x86_64-linux //score/time_daemon/...
    22/22 pass, zero skipped, zero sanitizer failures.
  • bazel test //:format.check → 5/5 pass.

No open findings, no unjustified suppressions — #77's acceptance criteria are met.

Scope note

These two potential bugs are repo-wide (MODULE.bazel/.bazelrc), not time_daemon-specific —
also unblocks #76/#78/#79 and is a prerequisite for #111/#112. Open to splitting into its own
issue/PR if preferred.

Related ticket

closes #77

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run //:license-check

Status: ⚠️ Needs Review

Click to expand output
[License Check Output]
Extracting Bazel installation...
Starting local Bazel server (8.6.0) and connecting to it...
INFO: Invocation ID: 3693887b-b122-4284-a6d7-eaa65f695b06
Computing main repo mapping: 
Computing main repo mapping: 
WARNING: For repository 'platforms', the root module requires module version platforms@1.0.0, but got platforms@1.1.0 in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
WARNING: For repository 'score_platform', the root module requires module version score_platform@0.7.1, but got score_platform@0.7.2 in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
WARNING: For repository 'rules_oci', the root module requires module version rules_oci@2.2.7, but got rules_oci@2.3.0 in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Loading: 
Loading: 3 packages loaded
WARNING: Target pattern parsing failed.
ERROR: Skipping '//:license-check': no such target '//:license-check': target 'license-check' not declared in package '' defined by /home/runner/work/time/time/BUILD
ERROR: no such target '//:license-check': target 'license-check' not declared in package '' defined by /home/runner/work/time/time/BUILD
INFO: Elapsed time: 14.328s
INFO: 0 processes.
ERROR: Build did NOT complete successfully
ERROR: Build failed. Not running target

@MaciejSalwa543

Copy link
Copy Markdown
Contributor Author

sanitizers.yml's matrix only runs asan_ubsan_lsan (line 39) — TSan was never added,
despite #102 implementing #43, titled "Add sanitizer CI (ASAN/TSAN/UBSAN)". Doesn't seem
to be intentional — I'd expect TSan to be in the CI job from the title/scope of the mentioned
ticket & PR.

Ran TSan+UBSan manually against score/time_daemon/... out of curiosity (not in CI, not part
of this PR): 2 failures, both ThreadSanitizer: data race.

  • ptp_control_flow_divider_test — in EventDrivenMachine's stop_token wait path
    (event_driven_machine.cpp:28). Self-pipe-wakeup-vs-teardown races are a common real bug
    shape here.
  • job_runner_test — in score_baselibs' Meyer-singleton logging init
    (GetSingletonWithFence) — looks more like a known TSan false-positive on fence-based
    lock-free init.

Looks like something I want to fix and add TSan to CI, would you agree?

@MaciejSalwa543
MaciejSalwa543 marked this pull request as ready for review September 10, 2026 06:02
@pawelrutkaq

Copy link
Copy Markdown
Contributor

@lavrovvalera ?

@github-project-automation github-project-automation Bot moved this from In Progress to Done in TIM - Time FT Sep 10, 2026
@MaciejSalwa543

Copy link
Copy Markdown
Contributor Author

Closed due to missing clang-tidy config file

@pawelrutkaq pawelrutkaq left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to tue this return type as auto or at least confirm it

Comment thread .github/workflows/clang-tidy.yml
Comment thread score/time_daemon/src/application/job_runner/job_runner.cpp Outdated
Comment thread score/time_daemon/src/common/machines/base_machine.h Outdated
Comment thread score/time_daemon/src/ptp_machine/stub/details/stub_ptp_engine.h Outdated
@MaciejSalwa543
MaciejSalwa543 force-pushed the fix-sca-sanitizer-ci-config-77 branch from 873aa0b to 96907e6 Compare September 17, 2026 18:49
@MaciejSalwa543
MaciejSalwa543 deployed to workflow-approval September 17, 2026 18:49 — with GitHub Actions Active
@MaciejSalwa543
MaciejSalwa543 deployed to workflow-approval September 17, 2026 18:49 — with GitHub Actions Active

@pawelrutkaq pawelrutkaq left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beside two small thing, now looks good to me

Comment thread score/time_daemon/src/ptp_machine/stub/details/stub_ptp_engine.h Outdated
Comment thread .github/workflows/clang-tidy.yml Outdated

This branch is waiting to be deployed

1 waiting deployment
workflow-approval f7fbb977 Waiting Sep 18, 2026 by MaciejSalwa543 via qnx-build (arm64-qnx) / approval #750
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Fix SCA and sanitizer findings: score/time_daemon

3 participants