feat: S21.01 build-time tunables mechanism via SOLIDSYSLOG_MAX_MESSAGE_SIZE#348
Conversation
…E_SIZE Introduce the build-time tunables override mechanism — defaults header + central header + optional user-config file + CMake INTERFACE target + BDD configure_file bridge. SOLIDSYSLOG_MAX_MESSAGE_SIZE is the first tunable; default 2048 unchanged, so no behavioural change on main. mbedTLS / FreeRTOS-Kernel / lwIP pattern: integrators set SOLIDSYSLOG_USER_TUNABLES_FILE to the absolute path of their own header containing #define overrides. The SolidSyslogTunables INTERFACE target propagates the macro to the library; the library #ifndef-guards each default so the user's value wins. Compile-time floor (#if/#error) rejects sub-floor values. Replaces the hardcoded "remember to bump this" SOLIDSYSLOG_MAX_MESSAGE_SIZE mirror in Bdd/features/steps/syslog_steps.py: CMake parses the defaults header (and the user-config file if set) at configure time and writes Bdd/features/steps/solidsyslog_tunables.py (gitignored) from a .in template. Single source of truth — drift impossible. New tunable-override-debug preset + build-linux-tunable-override CI job prove the override path flows end-to-end (static_assert in Tests/SolidSyslogTunablesTest.cpp fails the build if it doesn't). Fixes two latent test-infra bugs surfaced by the override: - Tests/SolidSyslogUdpSenderTest.cpp hardcoded TEST_MAX_MESSAGE_SIZE to 1024 despite its name claiming "max size"; now tracks the macro. - Tests/Support/SocketFake.c reserved a null-terminator byte inside a buffer sized exactly to MAX, silently clipping the last byte of a true max-size message; buffer is now MAX + 1. Closes #347. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR implements build-time tunables for SOLIDSYSLOG_MAX_MESSAGE_SIZE: new tunables headers and defaults, CMake INTERFACE propagation and presets (including an override preset), generation of a BDD Python mirror, test coverage for override propagation, source/test include migrations, and CI artifact/job wiring. ChangesBuild-time tunables mechanism for SOLIDSYSLOG_MAX_MESSAGE_SIZE
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1112 passed, 🙈 3 skipped) 🚧 Error MessagesCreated by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 817: The Quality Monitor config is missing an entry to parse results from
the new build-linux-tunable-override job; add a tests.tools array entry for
junit-build-linux-tunable-override in the quality-monitor section so its JUnit
artifact is parsed and shown in the Quality Summary. Locate the quality-monitor
config's tests.tools array (referencing the existing entries for
junit-build-linux-gcc/junit-build-linux-clang) and add a similar tool object for
junit-build-linux-tunable-override that points to the corresponding artifact
name and format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: daf94358-cfde-48f0-9f27-2d1c651e503f
📒 Files selected for processing (36)
.github/workflows/ci.yml.gitignoreBdd/Targets/Common/BddTargetInteractive.cBdd/Targets/FreeRtos/main.cBdd/Targets/Linux/main.cBdd/features/steps/solidsyslog_tunables.py.inBdd/features/steps/syslog_steps.pyCMakeLists.txtCMakePresets.jsonCore/Interface/SolidSyslog.hCore/Interface/SolidSyslogBlockStore.hCore/Interface/SolidSyslogCircularBuffer.hCore/Interface/SolidSyslogTunables.hCore/Interface/SolidSyslogTunablesDefaults.hCore/Source/CMakeLists.txtCore/Source/RecordStore.cCore/Source/RecordStore.hCore/Source/SolidSyslog.cCore/Source/SolidSyslogBlockStore.cCore/Source/SolidSyslogCircularBuffer.cDEVLOG.mdTests/Bdd/Targets/BddTargetServiceThreadTest.cppTests/CMakeLists.txtTests/FileFake.cTests/Fixtures/SmallMessageSizeTunables.hTests/SenderFake.cTests/SolidSyslogBlockStorePosixTest.cppTests/SolidSyslogBlockStoreTest.cppTests/SolidSyslogCircularBufferTest.cppTests/SolidSyslogCrc16Test.cppTests/SolidSyslogPosixMessageQueueBufferTest.cppTests/SolidSyslogTest.cppTests/SolidSyslogTunablesTest.cppTests/SolidSyslogUdpSenderTest.cppTests/Support/SocketFake.cTests/Support/WinsockFake.c
💤 Files with no reviewable changes (1)
- Core/Interface/SolidSyslog.h
…D artefact
Two follow-up fixes from CI on the S21.01 PR:
- analyze-iwyu was steering callers to include SolidSyslogTunablesDefaults.h
directly, which would bypass the optional SOLIDSYSLOG_USER_TUNABLES_FILE
override. Add `// IWYU pragma: private, include "SolidSyslogTunables.h"`
on the defaults header and `// IWYU pragma: export` on the include from
the umbrella, the canonical mbedTLS-shaped fix for the umbrella-header
case.
- The BDD jobs (linux-syslog-ng, windows-otel, freertos-qemu) only check
out the repo + download the BDD target binary; they don't run cmake, so
the gitignored Bdd/features/steps/solidsyslog_tunables.py wasn't present
and behave failed with `ModuleNotFoundError: No module named
'solidsyslog_tunables'`. Upload the generated file as a per-target
artefact from each build job and download it into Bdd/features/steps/ in
the matching BDD job (bdd-tunables-{linux,windows,freertos}). Per-target
artefacts so S21.02's FreeRTOS preset override flows independently.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
903-954:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd missing Quality Monitor test parser entry for tunable-override job.
Line 856 adds
build-linux-tunable-overridetosummary.needs, but Lines 903-954 do not parsejunit-build-linux-tunable-override, so this lane’s results won’t appear in Quality Summary.📊 Proposed fix
{ "id": "junit", "name": "build-linux-clang", "pattern": "**/junit-build-linux-clang/cpputest_*.xml" + }, + { + "id": "junit", + "name": "build-linux-tunable-override", + "pattern": "**/junit-build-linux-tunable-override/cpputest_*.xml" }, { "id": "junit", "name": "sanitize-linux-gcc", "pattern": "**/junit-sanitize-linux-gcc/cpputest_*.xml"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 903 - 954, The Quality Monitor tools array is missing a parser entry for the build-linux-tunable-override lane referenced in summary.needs; add a new object to the "tools" list with id "junit", name "build-linux-tunable-override", and pattern "**/junit-build-linux-tunable-override/cpputest_*.xml" so the junit reports from junit-build-linux-tunable-override are picked up in the Quality Summary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 903-954: The Quality Monitor tools array is missing a parser entry
for the build-linux-tunable-override lane referenced in summary.needs; add a new
object to the "tools" list with id "junit", name "build-linux-tunable-override",
and pattern "**/junit-build-linux-tunable-override/cpputest_*.xml" so the junit
reports from junit-build-linux-tunable-override are picked up in the Quality
Summary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8244b372-2a2a-43cd-8cf4-7f4570fce0ab
📒 Files selected for processing (3)
.github/workflows/ci.ymlCore/Interface/SolidSyslogTunables.hCore/Interface/SolidSyslogTunablesDefaults.h
✅ Files skipped from review due to trivial changes (1)
- Core/Interface/SolidSyslogTunables.h
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/Interface/SolidSyslogTunablesDefaults.h
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1112 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
…de to Quality Monitor - IWYU caught that Core/Interface/SolidSyslogBlockStore.h's #include of SolidSyslog.h is only referenced inside a documentation comment about NullBuffer recursion semantics, not by any code. My earlier audit grepped for SolidSyslogMessage/SolidSyslog_Log/SolidSyslog_Service symbols and counted the comment-mentions as hits — false positive. Remove the include. - CodeRabbit caught that build-linux-tunable-override was added to the summary gate's needs list but missing from the Quality Monitor config's tests.tools array, so its JUnit results would have been uploaded but not displayed in the Quality Summary panel. Add the matching tool entry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1112 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
1 participant
Purpose
First slice of #217 (E21 Port-Time Configurability). Introduce the mechanism for integrators to override library defaults at build time without forking the tree or editing files inside it. Default value of
SOLIDSYSLOG_MAX_MESSAGE_SIZEis unchanged at 2048, so no behavioural change onmain.Closes #347.
Change Description
C-side single source of truth.
Core/Interface/SolidSyslogTunablesDefaults.hholds every tunable as an#ifndef-guarded#definewith rich doc-comments.Core/Interface/SolidSyslogTunables.hoptionally#includes a user-supplied override file (controlled by theSOLIDSYSLOG_USER_TUNABLES_FILEmacro), then the defaults. mbedTLS / FreeRTOS-Kernel / lwIP convention — familiar to embedded developers.CMake
SolidSyslogTunablesINTERFACE target carries the override macro into the library and onto consumers viatarget_link_libraries(SolidSyslog PUBLIC SolidSyslogTunables). Per Ben Boeckel's CMake-Discourse guidance — usages that morph the library itself break CMake's identity model; an INTERFACE target the consumer can link against does not.Compile-time floor guard.
#if SOLIDSYSLOG_MAX_MESSAGE_SIZE < 64/#erroroutside the#ifndefblock evaluates whatever value is in effect (user override or default). Floor rationale: a legal all-NILVALUE RFC 5424 message is 16 bytes; 64 leaves real headroom for PRI, hostname, MSGID, short payload.BDD-Python bridge via
configure_filereplaces the hardcodedSOLIDSYSLOG_MAX_MESSAGE_SIZE = 2048mirror inBdd/features/steps/syslog_steps.py(with its "remember to bump this" comment — a known maintenance burden). CMake parses the defaults header and the user-config file (if set) at configure time and writesBdd/features/steps/solidsyslog_tunables.pyfrom a.intemplate. Gitignored. Drift now structurally impossible.tunable-override-debugpreset + newbuild-linux-tunable-overrideCI job prove the override flows end-to-end. Astatic_assertinTests/SolidSyslogTunablesTest.cppfails the build if the user-config file didn't reach the compiler. Job is wired into thesummarygate'sneeds:list; required-check status onmainis a separate GitHub-side step.Test Evidence
Followed strict red-green-refactor in three phases:
MaxMessageSizeIsReachableViaTunablesHeaderfails to compile (header missing). Green: create the two headers + register the test inTests/CMakeLists.txt.tunable-override-debugpreset →static_assert (2048 == 512)fires because the INTERFACE target isn't wired yet. Green: declareSolidSyslogTunablesINTERFACE in top-level CMake, link fromCore/Source. Build passes; all 1106 unit tests pass at MAX = 512.python3 -c 'import solidsyslog_tunables'→ModuleNotFoundError. Green: addconfigure_fileblock parsing the defaults header; reconfigure; import works, value mirrors configured MAX (2048 default / 512 override).Audit of existing tests for hardcoded message-size literals — fixed two latent bugs surfaced by the override:
Tests/SolidSyslogUdpSenderTest.cpp:28—TEST_MAX_MESSAGE_SIZE = 1024despite the test name claiming "max size transmitted without truncation". Now tracksSOLIDSYSLOG_MAX_MESSAGE_SIZE.Tests/Support/SocketFake.c— null-terminator slot inside a buffer sized toSOLIDSYSLOG_MAX_MESSAGE_SIZEsilently clipped the last byte of a true max-size message. Buffer is nowMAX + 1.Local pre-PR gates all pass:
build-linux-gcc(debug) — 1106 unit + 48 BDD-target testsbuild-linux-clang(clang-debug) — 1106 unit testssanitize-linux-gcc— 1106 tests under ASan + UBSancoverage-linux-gcc— 99.5% line coverage (above 90% gate)analyze-tidy— clang-tidy clean (one new NOLINTNEXTLINE on the macro#definewith documented rationale; matches existing precedent inSolidSyslogCircularBuffer.h)analyze-cppcheck— clean (one inline suppression on the#include SOLIDSYSLOG_USER_TUNABLES_FILEline — cppcheck evaluates both branches of#if definedand complains about empty include)analyze-format— clang-format cleanbuild-linux-tunable-override— 1106 tests pass at MAX = 512 with the user-config overrideAreas Affected
Core/Interface/,Core/Source/) — two new public headers (SolidSyslogTunables.h,SolidSyslogTunablesDefaults.h); enumSOLIDSYSLOG_MAX_MESSAGE_SIZE = 2048inSolidSyslog.hremoved (macro now); IWYU-clean migration of 12 internal#includelines.Bdd/Targets/*/main.c,Bdd/features/steps/) —#includeadjustments + replaced hardcoded Python mirror with generated module import.SolidSyslogTunablesTest.cpp+Tests/Fixtures/SmallMessageSizeTunables.h; two pre-existing test/fake files fixed.SolidSyslogTunablesINTERFACE target +configure_fileblock +tunable-override-debugpreset.build-linux-tunable-overridejob in.github/workflows/ci.yml, wired into thesummarygate.SolidSyslog.hget for free (still includesSolidSyslog.hif needed, plus nowSolidSyslogTunables.hexplicitly).Sets the pattern for further tunables (
SEND_TIMEOUT_*, buffer capacities, SD limits) under E21 — S21.03+ extend the defaults header one#ifndef-guarded#defineat a time. S21.02 will actually tune thefreertos-crosspreset down + audit BDD scenarios for hardcoded message-size assumptions.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
BDD / Tests
Chores
Documentation