feat: S30.03 generate the integration manifest from CMake (anti-drift)#572
Conversation
Add cmake/Manifest.cmake: a `manifest` target that emits the non-CMake file/include/-D integration manifest for the active configuration, reading the .c lists straight from the build (Core SOURCES + each selected pack's INTERFACE_SOURCES) so they can't drift. Selection via SOLIDSYSLOG_MANIFEST_PACKS (empty = all defined packs); Core is always included; the host Pattern-A adapters baked into Core are filtered out; include dirs are de-duplicated. Generation runs at configure time; the target prints via a -P helper (3.16-floor safe). Commit the generated beta-stack manifest at docs/generated/beta-stack-manifest.txt and add the gating `verify-manifest` CI job that regenerates it and fails on drift (git diff --exit-code). getting-started.md now consumes that generated file (hand-listed .c blocks removed) and documents the regenerate command. Note: verify-manifest must be added to branch-protection required checks (manual) to block merges on drift. Co-Authored-By: Claude Opus 4.8 (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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a CMake manifest generator and manifest print target, integrates generation into top-level CMake, adds a CI verify-manifest job that fails when regenerated manifest diverges from the committed beta manifest, and updates documentation and DEVLOG to reflect the new generated source of truth. ChangesManifest Generation & Drift Verification
Sequence DiagramsequenceDiagram
participant Developer
participant CMakeConfig as CMake Configure
participant ManifestModule as Manifest.cmake
participant ManifestTarget as manifest Target
participant PrintScript as PrintManifest.cmake
participant CIJob as verify-manifest Job
participant Stdout
Developer->>CMakeConfig: cmake -DSOLIDSYSLOG_MANIFEST_PACKS=...
CMakeConfig->>ManifestModule: solidsyslog_generate_manifest()
ManifestModule->>ManifestModule: read INTERFACE_SOURCES from targets
ManifestModule->>ManifestModule: collect INTERFACE_INCLUDE_DIRECTORIES deduped
ManifestModule->>ManifestModule: emit config headers + conditional defines
ManifestModule->>ManifestModule: write solidsyslog-manifest.txt
Developer->>ManifestTarget: cmake --build . --target manifest
ManifestTarget->>PrintScript: -DSOLIDSYSLOG_MANIFEST_FILE=...
PrintScript->>Stdout: read and emit manifest
CIJob->>CMakeConfig: regenerate manifest for beta pack set
CMakeConfig->>ManifestModule: solidsyslog_generate_manifest()
ManifestModule->>CIJob: produces file
CIJob->>CIJob: git diff --exit-code -> fail on drift
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/getting-started.md (1)
182-223:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDNS define guidance contradicts the selected beta stack.
Line 219 says no
LWIP_DNSis required, but Line 182 includes DNS in the target stack and the generated manifest referenced on Line 190 includes-DLWIP_DNS=1. This can lead integrators to a broken config.Suggested fix
### 2. Defines ```text -DSOLIDSYSLOG_USER_TUNABLES_FILE="my_tunables.h" # your tunable overrides +-DLWIP_DNS=1 # required by SolidSyslog::LwipRawDnsResolver-> No
LWIP_DNSis required for this stack (numeric resolver). The header-configured
+>LWIP_DNS=1is required for this stack because it includes the lwIP DNS resolver.
+> The header-configuredupstreams take their settings from your config headers, not from
-Ds:
lwipopts.h(incl.NO_SYS,LWIP_RAW/UDP/TCP),mbedtls_config.h,
FreeRTOSConfig.h(withconfigSUPPORT_STATIC_ALLOCATION=1for the mutex),
ffconf.h.</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/getting-started.mdaround lines 182 - 223, The docs contradict
themselves about LWIP_DNS: the beta stack includes LwipRawDnsResolver (manifest
references the pack and adds -DLWIP_DNS=1) but the text claims no LWIP_DNS is
required; update the getting-started.md guidance to require LWIP_DNS for the
numeric resolver by adding a note that-DLWIP_DNS=1is required when using
SolidSyslog::LwipRawDnsResolver (and the generated manifest in
docs/generated/beta-stack-manifest.txt reflects this), and adjust the paragraph
that currently says “NoLWIP_DNSis required” to instead state that
LWIP_DNS=1is required for this stack so integrators enable it in their
build/tunables.</details> <!-- cr-comment:v1:2bd6c06fff6f2eaa02156b93 --> </blockquote></details> </blockquote></details>🤖 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. Outside diff comments: In `@docs/getting-started.md`: - Around line 182-223: The docs contradict themselves about LWIP_DNS: the beta stack includes LwipRawDnsResolver (manifest references the pack and adds -DLWIP_DNS=1) but the text claims no LWIP_DNS is required; update the getting-started.md guidance to require LWIP_DNS for the numeric resolver by adding a note that `-DLWIP_DNS=1` is required when using SolidSyslog::LwipRawDnsResolver (and the generated manifest in docs/generated/beta-stack-manifest.txt reflects this), and adjust the paragraph that currently says “No `LWIP_DNS` is required” to instead state that `LWIP_DNS=1` is required for this stack so integrators enable it in their build/tunables.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID:
30db0f02-f063-499a-87b4-489f72b9f342⛔ Files ignored due to path filters (1)
docs/generated/beta-stack-manifest.txtis excluded by!**/generated/**📒 Files selected for processing (6)
.github/workflows/ci.ymlCMakeLists.txtDEVLOG.mdcmake/Manifest.cmakecmake/PrintManifest.cmakedocs/getting-started.md
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1521 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
The beta sample stack includes SolidSyslog::LwipRawDnsResolver (the generated manifest lists -DLWIP_DNS=1), but the Defines note still claimed no LWIP_DNS was required — a contradiction that could lead integrators to a broken config. Align the note with the stack and point at the generated manifest as authoritative. (CodeRabbit, PR #572.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1521 passed) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
1 participant
Purpose
Final E30 (#566) story. Generate the non-CMake integration manifest from
CMake so the file/include/
-Dlist can't drift from the build, and replace thehand-authored lists in
getting-started.md(S30.01) with generated output.Closes #569
Change Description
cmake/Manifest.cmake— amanifesttarget that emits the integrationmanifest for the active configuration. The
.clists are read straight from thebuild (the Core lib's
SOURCES+ each selected pack'sINTERFACE_SOURCES,populated in S30.02), so they can't drift. Stable per-pack knowledge (the config
header each pack needs) is a small curated map.
SOLIDSYSLOG_MANIFEST_PACKS(;-list of pack short-names;empty = every defined
SolidSyslog::<Pack>). Core always included.(POSIX/Windows/OpenSSL/C11-atomics) are filtered out — they're host-build
conveniences, not part of an embedded manifest. Include dirs de-duplicated.
${BINARY_DIR}/solidsyslog-manifest.txt,plus
SOLIDSYSLOG_MANIFEST_OUTPUTwhen set). Themanifesttarget prints itvia a
-Phelper (cmake/PrintManifest.cmake) so it works on the project'sCMake 3.16 floor (
cmake -E catis 3.18+).docs/generated/beta-stack-manifest.txt;the new
verify-manifestCI job regenerates it in the cpputest-freertos imageand
git diff --exit-codes (fails on drift). The beta sample includes the DNSresolver (matching the FreeRtosLwip BDD target), so it demonstrates the opt-in
component +
-DLWIP_DNS=1.getting-started.mdnow consumes the generated file — the hand-listed.cblocks are gone, replaced by a pointer to the committed manifest + the
regenerate command. Bare code fences in the touched file tagged (
text).Test Evidence
CMake/docs only (no
.c/.hchanged). Verified locally in the cpputest-freertosimage:
manifesttarget emits a correct manifest; Core list has noPlatform/leak; include dirs de-duplicated.
git diff --exit-codeis clean (the gate passes and would catch real drift).
cmake --preset debug, all 7 packs) configures and the fulljunitbuild passes.Areas Affected
cmake/Manifest.cmake(new),cmake/PrintManifest.cmake(new),CMakeLists.txt(include + call),
docs/generated/beta-stack-manifest.txt(new committed sample),docs/getting-started.md,.github/workflows/ci.yml(newverify-manifestjob),
DEVLOG.md.Summary by CodeRabbit
New Features
Documentation
Chores