fix: correct catch ordering and migrate to spdlog#99
fix: correct catch ordering and migrate to spdlog#99siddimore wants to merge 2 commits intolivekit:mainfrom
Conversation
…uard - Fix exception catch ordering in 5 RPC methods in livekit_bridge.cpp: catch(std::exception) was listed before catch(livekit::RpcError), making the RpcError handler unreachable dead code. Since RpcError extends std::runtime_error (which extends std::exception), the most-derived type must be caught first. Reorder to: catch(RpcError) -> catch(std::exception), and remove the redundant catch(std::runtime_error) clause entirely. - RpcError catches now log code() and message() instead of just what(), preserving the structured error information (error code, message, data). - Replace all std::cerr usage across bridge/ with LK_LOG_ERROR and LK_LOG_WARN macros (spdlog), consistent with the rest of the SDK. Remove <iostream> includes that are no longer needed. - Add missing #pragma once include guard to src/track_proto_converter.h, which had no include guard at all (every other header in the project has one).
siddimore
changed the title
spdlog::spdlog is linked PRIVATE to the livekit target, so its headers do not propagate to livekit_bridge. Add an explicit PRIVATE link to spdlog::spdlog and set SPDLOG_ACTIVE_LEVEL so the bridge can use LK_LOG_* macros from lk_log.h.
|
Hi @stephen-derosa and @alan-george-lk , could you please review this PR ? |
alan-george-lk
left a comment
alan-george-lk
left a comment
There was a problem hiding this comment.
Changes LGTM -- I will however hold off approving until @stephen-derosa takes a look. AFAIK the bridge is deprecated, so we may be minimizing changes to it. @siddimore are you actively using it?
| return std::nullopt; | ||
| } catch (const livekit::RpcError &e) { | ||
| std::cerr << "[LiveKitBridge] RPC error: " << e.what() << "\n"; | ||
| LK_LOG_ERROR("performRpc RPC error (code {}): {}", e.code(), e.message()); |
There was a problem hiding this comment.
Should we preserve the [LiveKitBridge] prefix here?
[LiveKitBridge] performRpc ...
stephen-derosa
left a comment
stephen-derosa
left a comment
There was a problem hiding this comment.
@siddimore are you actively using the bridge? FYI the bridge has been deperecated since its functionality has been moved to the core cpp sdk. Are you able to migrate to the base sdk?
| * limitations under the License. | ||
| */ | ||
|
|
||
| #pragma once |
@alan-george-lk not actively using just trying it out. |
@siddimore id prefer that we dont touch the bridge, especially since this is sort of feature/maintenance work. We are really excited for your contributions though! |
4 participants
RpcError catches now log code() and message() instead of just what(), preserving the structured error information (error code, message, data).
Replace all std::cerr usage across bridge/ with LK_LOG_ERROR and LK_LOG_WARN macros (spdlog), consistent with the rest of the SDK. Remove includes that are no longer needed.
Add missing #pragma once include guard to src/track_proto_converter.h, which had no include guard at all (every other header in the project has one).