fix(rag): RAC_ENABLE_PROTOBUF=OFF cannot compile the RAG backend - #824
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe RAG protobuf ABI source now includes ChangesConditional protobuf include
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to The targeted build fix is complete with no remaining merge-blocking risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
a4b002b to
2251bef
Compare
rac_rag_proto_abi.cpp includes features/llm/llm_thinking_tags_internal.h above the RAC_HAVE_PROTOBUF guard, but the commons src/ root only joins rac_backend_rag's include path inside the protobuf branch of core/src/features/rag/CMakeLists.txt. With RAC_ENABLE_PROTOBUF=OFF the header is unreachable and the build stops on a fatal include error. Its only use is inside the guard, so move the include next to the two src-relative siblings that are already there.
2251bef to
49349cf
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
sanchitmonga22
left a comment
There was a problem hiding this comment.
Thanks a lot for this, @ayaangazali! This fixes the RAG backend failing to compile with RAC_ENABLE_PROTOBUF=OFF — it moves the features/llm/llm_thinking_tags_internal.h include inside the existing RAC_HAVE_PROTOBUF guard, right next to its only caller in this file.
Checked: CodeRabbit reviewed the latest commit · CI green · built and linted locally merged into main (C++ commons RAG backend, both protobuf ON and OFF) · two independent code reviews.
Follow-ups, not blocking: #902 (CI never exercises the protobuf-OFF RAG build) — you're welcome to pick it up.
Merging now — really appreciate the contribution!
Reviewed with help from Claude Code and Codex.
Description
cmake -B build -DRAC_ENABLE_PROTOBUF=OFFdoes not build. It stops here:RAC_ENABLE_PROTOBUFis a real supported option (core/CMakeLists.txt:1417) with its own documentedelse()branch — "RAC_ENABLE_PROTOBUF=OFF — exported proto ABI functions use unavailable stubs" — and it is the EMSCRIPTEN default.RAC_BACKEND_RAGdefaultsON, so the flag on its own is enough to reproduce.The cause is an include on the wrong side of a guard.
rac_rag_proto_abi.cpppulls in three commons-internal headers by a path relative to thesrc/root, but only two of them are inside theRAC_HAVE_PROTOBUFguard:That matters because the
src/root only joins the target's include path inside the protobuf branch ofcore/src/features/rag/CMakeLists.txt:120:With protobuf off, that directory is never added and the unguarded include cannot resolve. The other two never had the problem because they sit where the include path exists.
The header's only use is
model_thinking_tags_from_registryat line 497, which is itself inside the guard, so the include belongs there too. This moves that one line down beside its siblings — no new include directory, no CMake change, and nothing new compiled in either configuration.Why CI is green today: no workflow configures
RAC_ENABLE_PROTOBUF=OFF, andbindings/web/wasm/scripts/build.sh:416passes-DRAC_ENABLE_PROTOBUF=ONexplicitly, so the WASM build overrides the EMSCRIPTEN default and never hits it.Type of Change
Testing
No test added: this is a build-configuration failure, so the build in each configuration is the check, and there is no runtime behaviour to assert. Both directions verified.
Protobuf off, which is the failure this fixes:
Protobuf on, unchanged:
On lint: left unchecked rather than imply more than I checked.
core/scripts/lint-cpp.shwants the repo's pinnedclang-formatand only Appleclang-format 21is available here, which reports pre-existing diffs in this file at lines 343/551/575/601/605/609/668 — none of them my two lines (29 and 48). I did not reformat anything.No platform boxes ticked: commons-only, verified through the two CMake configurations and the C++ suite on macOS.
Labels
SDKs:
Commons- Changes to shared native code (core)Checklist
Summary by CodeRabbit
CI note: the red
centralizationis not from this diff. Its only failing step is "Swift distribution repo (runanywhere-swift) is cut at this release" — that repo is tagged0.20.30while this one has publishedv0.20.31, whichscripts/release/sync-versions.sh:616documents as failing every PR until the tag is cut. Every other step in that job, including the C++ gates, passed. Nothing to push here; the remedy is cutting the distribution repo.