[BUG] Fix conditional linking of RTCX embed dependencies & Allow null fragment names - #22898
Conversation
📝 WalkthroughWalkthrough
Changeszstd mandatory dependency and logging robustness
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 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)
Comment |
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 `@cpp/librtcx/embed.cmake`:
- Around line 8-10: The error message in the message(FATAL_ERROR...) call within
the `if(NOT TARGET zstd)` block is misleading because it references "when
COMPRESSION is not none", but this check executes unconditionally at module load
time before ARG_COMPRESSION is set by any embed() call. Since zstd is now always
required unconditionally per this PR, update the error message to reflect that
zstd target is required unconditionally, removing the reference to the
COMPRESSION conditional logic.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 97c5c6ce-73bd-4eeb-ae56-d3aa83f55d67
📒 Files selected for processing (1)
cpp/librtcx/embed.cmake
|
/merge |
Description
Fixes conditional compilation bug introduced in #22856.
The
Zstdfunctions are always referenced regardless of the specified compression.The dependency is always available regardless of the compression type, and it should be linked into the embed runner executable.
It also makes a fix to allow
nullptrin fragment names.Checklist