Fix clang-tidy and sanitizer CI config blocking score/time_daemon checks by MaciejSalwa543 · Pull Request #216 · eclipse-score/time

MaciejSalwa543 · 2026-09-09T18:32:23Z

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 · 2026-09-09T18:33:15Z

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: 0d76aba2-1141-454b-b4c3-1081e5af2ce0
Computing main repo mapping: 
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: 
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: 15.153s
INFO: 0 processes.
ERROR: Build did NOT complete successfully
ERROR: Build failed. Not running target

MaciejSalwa543 · 2026-09-09T18:36:56Z

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?

pawelrutkaq · 2026-09-10T06:25:00Z

@lavrovvalera ?

MaciejSalwa543 · 2026-09-10T07:05:09Z

Closed due to missing clang-tidy config file

pawelrutkaq

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

pawelrutkaq · 2026-09-16T08:10:35Z

          if [ "$COUNT" -gt "$BASELINE" ]; then
-            echo "::error::Clang-tidy violations grew: $COUNT > baseline $BASELINE. Fix new violations before merging."
+            echo "::error::Clang-tidy violations in score/time_daemon/ grew: $COUNT > baseline $BASELINE. Fix new violations before merging."
            exit 1


we shall scope TimeDaemon to 0 and leave rest as it was ;)

pawelrutkaq · 2026-09-16T08:12:24Z

 }

-JobRunner::Result JobRunner::GetResult() const
+auto JobRunner::GetResult() const -> JobRunner::Result


This rule is strange imho and will cause troubles with misra. @lavrovvalera what do you think ?

I can imagine, it is an advantage, when we can help compiler to deriuve the proper type when it is going about the template, type elicittion and so on.
but here (and everywhere), we can specify the exact return type. why do we need to return auto instead and anyway specify the return value.
for me it doesn't make sense and we need indeed to do somethign with the rule itself

@MaciejSalwa543 can you indetify which rule is causing this ?

"modernize-use-trailing-return-type" - included with "modernize*" in .clang-tidy

Then lets disable this one

pawelrutkaq · 2026-09-16T08:14:15Z

    virtual ~BaseMachine() = default;

-    inline std::string GetName() const noexcept
+    [[nodiscard]] auto GetName() const noexcept -> std::string


nodiscrd makse no sense or ?

https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-nodiscard.html
also "modernize-*"

Ye but in this API it makse no sense imho

after looking second times, yeah I think you're right here

pawelrutkaq · 2026-09-16T08:18:13Z

    /// \return true - deinitialize success, otherwise false
    ///
-    bool Deinitialize() const;
+    [[nodiscard]] auto Deinitialize() const -> bool;


nodiscrd maybe not needed here

the same as above

good catch - always return true

pawelrutkaq · 2026-09-16T08:19:19Z


+// NOLINTBEGIN(bugprone-easily-swappable-parameters) — debouncing_clock and max_time_jump_allowed
+// have unrelated, non-convertible types (ReferenceClock vs. chrono::nanoseconds), so swapping the
+// call-site arguments would fail to compile rather than silently misbehave.


but those two:
std::chrono::nanoseconds max_time_jump_allowed,
std::chrono::nanoseconds sync_debounce_threshold,

Are easily swappable. Maybe the order of args could be changed

Signed-off-by: Maciej Salwa <maciej.salwa.ext@qorix.ai>

github-project-automation Bot added this to TIM - Time FT Sep 9, 2026

github-project-automation Bot moved this to In Progress in TIM - Time FT Sep 9, 2026

MaciejSalwa543 deployed to workflow-approval September 9, 2026 18:32 — with GitHub Actions Active

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

pawelrutkaq requested a review from lavrovvalera September 10, 2026 06:24

MaciejSalwa543 closed this Sep 10, 2026

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

MaciejSalwa543 reopened this Sep 15, 2026

MaciejSalwa543 had a problem deploying to workflow-approval September 15, 2026 06:23 — with GitHub Actions Error

MaciejSalwa543 marked this pull request as draft September 15, 2026 06:40

MaciejSalwa543 had a problem deploying to workflow-approval September 15, 2026 07:13 — with GitHub Actions Error

MaciejSalwa543 force-pushed the fix-sca-sanitizer-ci-config-77 branch from 172e39e to 8926082 Compare September 15, 2026 07:29

MaciejSalwa543 deployed to workflow-approval September 15, 2026 07:29 — with GitHub Actions Active

MaciejSalwa543 marked this pull request as ready for review September 15, 2026 10:29

pawelrutkaq requested changes Sep 16, 2026

View reviewed changes

MaciejSalwa543 had a problem deploying to workflow-approval September 17, 2026 17:39 — with GitHub Actions Error

MaciejSalwa543 added 5 commits September 17, 2026 20:41

Fix clang-tidy and sanitizer CI config blocking time_daemon checks

f75f295

Signed-off-by: Maciej Salwa <maciej.salwa.ext@qorix.ai>

Enable S-CORE clang-tidy baseline and fix time_daemon findings

5cd42de

Signed-off-by: Maciej Salwa <maciej.salwa.ext@qorix.ai>

fix clang-tidy for yamlfmt

c825f16

Add clang-tidy path to collect only time_deamon findings

73a1f0d

Signed-off-by: Maciej Salwa <maciej.salwa.ext@qorix.ai>

Fix swappable params in TimeJumpsValidator ctor

3b97a1c

MaciejSalwa543 added 3 commits September 17, 2026 20:41

Drop unjustified nodiscard on GetName/Deinitialize

9749fc3

Revert modernize-use-trailing-return-type for time_daemon

259719f

Add frozen baseline for clang-tidy findings outside time_daemon

96907e6

MaciejSalwa543 force-pushed the fix-sca-sanitizer-ci-config-77 branch from 873aa0b to 96907e6 Compare September 17, 2026 18:49

MaciejSalwa543 requested a deployment to workflow-approval September 17, 2026 18:49 — with GitHub Actions Waiting

Conversation

MaciejSalwa543 commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Results

Scope note

Related ticket

Uh oh!

github-actions Bot commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

License Check Results

Uh oh!

MaciejSalwa543 commented Sep 9, 2026

Uh oh!

pawelrutkaq commented Sep 10, 2026

Uh oh!

MaciejSalwa543 commented Sep 10, 2026

Uh oh!

pawelrutkaq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaciejSalwa543 Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MaciejSalwa543 commented Sep 9, 2026 •

edited

Loading

github-actions Bot commented Sep 9, 2026 •

edited

Loading

MaciejSalwa543 Sep 16, 2026 •

edited

Loading