From 0111a26653144d4e9057699ce58e86e00e824c1b Mon Sep 17 00:00:00 2001 From: Alan George Date: Tue, 19 May 2026 12:11:42 -0700 Subject: [PATCH 1/5] Treat warnings as errors, move third-party stuff out of warning checks --- CMakeLists.txt | 49 +++++++- PRE_RELEASE_AUDIT.md | 266 +++++++++++++++++++++++++++++++++++++++ src/tests/CMakeLists.txt | 25 +--- src/video_source.cpp | 2 +- 4 files changed, 313 insertions(+), 29 deletions(-) create mode 100644 PRE_RELEASE_AUDIT.md diff --git a/CMakeLists.txt b/CMakeLists.txt index 895284b1..6924211d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -52,6 +52,26 @@ else() set(LIVEKIT_IS_TOPLEVEL FALSE) endif() +function(livekit_enable_strict_compile_warnings target_name) + if(MSVC) + target_compile_options(${target_name} PRIVATE /permissive- /Zc:__cplusplus /W4 /WX) + else() + target_compile_options(${target_name} PRIVATE + -Wall + -Wextra + -Wpedantic + -Werror + $<$:-Wno-gnu-zero-variadic-macro-arguments> + ) + endif() +endfunction() + +function(livekit_add_system_include_dirs target_name) + if(ARGN) + target_include_directories(${target_name} SYSTEM PRIVATE ${ARGN}) + endif() +endfunction() + if(LIVEKIT_IS_TOPLEVEL) # Since we use separate build directories (build-debug/build-release), # we don't need additional platform/config subdirectories @@ -113,6 +133,27 @@ if(TARGET protobuf::libprotobuf) else() message(FATAL_ERROR "No protobuf library target found (expected protobuf::libprotobuf)") endif() + +set(LIVEKIT_EXTERNAL_SYSTEM_INCLUDE_DIRS + "${PROTO_BINARY_DIR}" + ${Protobuf_INCLUDE_DIRS} +) +foreach(_livekit_system_include_target + protobuf::libprotobuf + absl::base + spdlog::spdlog + spdlog) + if(TARGET ${_livekit_system_include_target}) + get_target_property(_livekit_system_includes + ${_livekit_system_include_target} + INTERFACE_INCLUDE_DIRECTORIES) + if(_livekit_system_includes) + list(APPEND LIVEKIT_EXTERNAL_SYSTEM_INCLUDE_DIRS ${_livekit_system_includes}) + endif() + endif() +endforeach() +list(REMOVE_DUPLICATES LIVEKIT_EXTERNAL_SYSTEM_INCLUDE_DIRS) + target_include_directories(livekit_proto PRIVATE "${PROTO_BINARY_DIR}" ${Protobuf_INCLUDE_DIRS} @@ -433,8 +474,8 @@ target_include_directories(livekit ${LIVEKIT_ROOT_DIR}/src ${LIVEKIT_ROOT_DIR}/src/trace ${RUST_ROOT}/livekit-ffi/include - ${PROTO_BINARY_DIR} ) +livekit_add_system_include_dirs(livekit ${LIVEKIT_EXTERNAL_SYSTEM_INCLUDE_DIRS}) target_link_libraries(livekit PRIVATE @@ -600,11 +641,7 @@ if(UNIX AND NOT APPLE) target_link_libraries(livekit PRIVATE OpenSSL::SSL OpenSSL::Crypto) endif() -if(MSVC) - target_compile_options(livekit PRIVATE /permissive- /Zc:__cplusplus /W4) -else() - target_compile_options(livekit PRIVATE -Wall -Wextra -Wpedantic) -endif() +livekit_enable_strict_compile_warnings(livekit) # -------------------- Install / Package (SDK bundle) -------------------- include(GNUInstallDirs) diff --git a/PRE_RELEASE_AUDIT.md b/PRE_RELEASE_AUDIT.md new file mode 100644 index 00000000..7ab7fc39 --- /dev/null +++ b/PRE_RELEASE_AUDIT.md @@ -0,0 +1,266 @@ +# Pre-Release Low-Hanging Fruit Audit + +> **Context:** Audit performed on `feature/unified_method_style_tmp_participant` +> branch in preparation for a major release. Breaking changes are acceptable. +> Constraint: **do not remove deprecated code in this PR** — that is scheduled +> for a separate follow-up. +> +> Date generated: Friday, May 15, 2026 + +--- + +## Category A — Quick TODO resolutions (≤ a few lines each) + +### A1. `src/ffi_client.cpp:257` — silent integer truncation in FFI callback + +```cpp +event.ParseFromArray(buf, + static_cast(len)); // TODO: this fixes for now, what if len exceeds int? +``` + +`size_t → int` cast can wrap for buffers > 2 GB. Cheap fix: bounds check + +`LK_LOG_ERROR` + early return. ~3 lines. + +### A2. `src/audio_stream.cpp:145, 174` — dead-code TODOs + +```cpp +// TODO, sample_rate and num_channels are not useful in AudioStream, remove it from FFI. +// new_audio_stream->set_sample_rate(options_.sample_rate); +// new_audio_stream->set_num_channels(options.num_channels); +``` + +Two duplicate TODOs with commented-out FFI fields. The Rust side ignores them. +Either delete both comment blocks (resolution: "won't do") or actually drop the +fields from the proto. Cheapest path is just remove the dead `//` lines — +they've been dead long enough that we know we don't need them. + +### A3. `src/room_proto_converter.cpp:103` — `toDisconnectReason` is a stub + +```cpp +DisconnectReason toDisconnectReason(proto::DisconnectReason /*in*/) { + // TODO: map each proto::DisconnectReason to your DisconnectReason enum + return DisconnectReason::Unknown; +} +``` + +This is **functionally broken** — every `DisconnectReason` event delivered to +consumers comes through as `Unknown`. Real fix is a switch over the proto enum +(~20 lines, mechanical). High-impact, low-effort. **Strong candidate for +pre-release fix.** + +### A4. `src/video_stream.cpp:151` — stale TODO + +```cpp +// TODO, do we need to cache the metadata from stream.info ? +``` + +`AudioStream` doesn't cache it either, and no caller asks for it. Resolve by +deleting the comment. + +### A5. `src/local_participant.cpp:91` — `publishDtmf` ignores destinations + +```cpp +// TODO, should we take destination as inputs? +const std::vector destination_identities; +``` + +This is a public-API design decision. For a major release where breaks are OK, +adding an optional `destinations` parameter to `publishDtmf` is the right time. +~5 lines + 1 doc-comment update. + +### A6. `src/room.cpp:1087` — `kConnectionStateChanged` doesn't update `connection_state_` + +```cpp +// TODO, maybe we should update our |connection_state_| correspoindingly, +// but the this kConnectionStateChanged event is never triggered in my local test. +``` + +This is a real correctness gap (typo "correspoindingly" too). Either: (a) write +the assignment under the lock (already held), or (b) verify the event is +actually unused and delete the case. Both are tiny. + +### A7. `src/room_proto_converter.cpp:287, 295` — unmapped reader handles + +```cpp +ByteStreamOpenedEvent fromProto(const proto::ByteStreamOpened& in) { + // TODO: map reader handle once OwnedByteStreamReader is known +``` + +The `reader_handle` field is being silently dropped from +`ByteStreamOpenedEvent` / `TextStreamOpenedEvent`. Worth flagging for product +owner: is the reader handle exposed through the C++ API? If not, drop the +comments and the unused field on the event struct. If it is, wire it up. + +### A8. `src/tests/integration/test_data_track.cpp:490` — flaky test + +```cpp +// TODO(BOT-347): this sometimes fails with a timeout. +``` + +Has a tracking ticket. Not low-hanging in the code sense, but worth keeping on +the radar for the release sign-off. + +### A9. `bridge/README.md:78` — TODO from another author + +``` +TODO(sderosa): add instructions on how to use the bridge in your own CMake project. +``` + +Bridge is being deleted on 06/01/2026 anyway (see B1). Safe to drop the TODO. + +### A10. `.github/workflows/make-release.yml:247` — Windows fix-up + +``` +# TODO(sxian): fix it after getting access to a windows machine +``` + +Probably blocked on hardware. Leave but worth confirming if it's been resolved. + +--- + +## Category B — Approaching deprecation deadlines + +### B1. Two deprecation deadlines fall on `06/01/2026` (~16 days from audit date) + +`README.md:624-625` and supporting deprecation messages in +`include/livekit/room.h` and `include/livekit/subscription_thread_dispatcher.h`: + +- `bridge/` (`livekit_bridge`) folder removal +- `setOnAudioFrameCallback(TrackSource)` / `setOnVideoFrameCallback(TrackSource)` + overloads removal + +Action: **either** push the date out to align with the major release window, +**or** keep the date and queue a follow-up PR to actually remove them on +06/01/2026. Right now the dates will silently slip with no enforcement. Just +flag — don't change without a product call. + +### B2. Internal callers of soon-to-be-removed callback overloads + +`src/tests/unit/test_room_callbacks.cpp` and +`src/tests/unit/test_subscription_thread_dispatcher.cpp` call +`setOnAudioFrameCallback(participant, TrackSource, …)` ~30+ times. When the +overload is removed, those tests break. They need migration to the track-name +overload before B1's deadline. **Not** low-hanging — substantial test refactor +— but worth scheduling now so it isn't a blocker on deletion day. + +--- + +## Category C — Stale doc comments / minor hygiene + +### C1. `include/livekit/participant.h:63-99` — references to no-longer-existent siblings + +Each deprecated `set_*` carries the comment +`// Deprecated - see setName() (also deprecated; see notes above).` But +`setName()` etc. on `Participant` were removed in this branch (the WIP that's +currently uncommitted), and there are no "notes above" anymore (the long +comment block was deleted too). Now reads as a dangling cross-reference. +~7 single-line comments to update or remove. Trivial. + +### C2. `include/livekit/track.h:91-94` — TODO-style code comment masquerading as documentation + +```cpp +// std::string can actually throw, suppressing for now to maintain API +// compatibility +// NOLINTNEXTLINE(bugprone-exception-escape) +std::optional mimeType() const noexcept { return mime_type_; } +``` + +This is the kind of `noexcept` lie that shows up as undefined behavior in the +wild. For a major release where breaks are OK, the right move is to drop +`noexcept` from `mimeType()` (and `mime_type()`). It's a one-token API change; +only effect on consumers is they can no longer use it in `noexcept`-required +contexts (rare). Worth raising for the breaking-change list. + +### C3. `src/ffi_client.h:23` — unused `` + +```cpp +#include +``` + +No `std::cout` / `std::cerr` in the file. Drop the include. Tiny. + +### C4. Per-AGENTS.md include conventions — clean + +Public headers all use `"livekit/foo.h"` (good); internal headers use bare +names (good). No issues found. + +--- + +## Category D — NOLINT suppressions worth a second look + +Most suppressions are intentional and well-scoped. A handful that could be +tightened: + +### D1. `src/ffi_client.cpp:151` — `// NOLINTNEXTLINE(modernize-use-equals-default)` + +Worth checking if the destructor body still needs to be non-trivial. If it's +just suppressing `~FfiClient() {}` vs `~FfiClient() = default;`, the lint is +right and the fix is `= default`. + +### D2. `src/data_track_stream.cpp:63` — `// NOLINT(bugprone-unchecked-optional-access)` + +Suppression is correct (the `cv_.wait` predicate plus the `if` above prove +`frame_.has_value()`), but a one-line `assert(frame_.has_value());` or +`[[assume]]` makes the invariant self-documenting. Optional polish. + +### D3. NOLINT span in `src/subscription_thread_dispatcher.cpp:511-537, 574-602` + +`bugprone-lambda-function-name` and `bugprone-exception-escape` suppressed +across ~30-line lambdas. These are real (the lambdas catch-all and log via +`__func__`). Worth a brief comment near the `NOLINTBEGIN` explaining *why*, +not just *what*. Tiny but improves auditability. + +### D4. `src/trace/trace_event.h` and `src/trace/event_tracer.h` — file-wide NOLINT + +Both wrap the entire file in `// NOLINTBEGIN ... // NOLINTEND` (no specific +check listed). This globally disables clang-tidy. If these were ported in from +Chromium and we're not the upstream, that's defensible — note it inline. +Otherwise scope to specific checks. + +The remaining `readability-identifier-naming` suppressions are all on the +deprecated snake_case wrappers — those have to stay as long as the deprecated +symbols stay. + +--- + +## Category E — Things to flag, not fix in this pass + +These need product/architecture sign-off before changing: + +### E1. Public-API integers using bare `int` + +`AudioFrame`, `AudioSource`, `VideoSource` ctors use bare `int` for +`sample_rate`, `num_channels`, `samples_per_channel`. AGENTS.md prefers +fixed-width but explicitly says don't change existing public APIs without a +compat review. Flag for the breaking-changes list. + +### E2. `Participant::Participant(...)` ctor takes 8 positional args + +`include/livekit/participant.h:34-43`. Easy to misorder. A builder/options +struct would be more durable. Major-release scope. + +### E3. `local_participant.h:117` Python-idiom doc comment + +References `LocalParticipant.set_name` as the Python idiom — accurate but might +confuse C++ readers since our API is `setName`. Worth a brief parenthetical. + +### E4. `bridge/README.md:5` removal date + +Warns the folder is being removed on 06/01. Sync with B1 — same date. + +--- + +## Recommended quick-win batch (a single small PR) + +If you want a single low-risk PR that just clears the easy stuff before the +release branch cuts: + +- **A1** — FFI buffer length bounds-check +- **A3** — `toDisconnectReason` real mapping +- **A4, A9** — delete two stale TODO comments +- **C1** — clean up dangling `// see setName()` references in `participant.h` +- **C3** — drop unused `` +- **D1** — `= default` the `FfiClient` dtor if applicable + +That gets visibly cleaner code, fixes one real correctness bug (A3), and +leaves all the deprecated API surface untouched per the constraint. diff --git a/src/tests/CMakeLists.txt b/src/tests/CMakeLists.txt index 2bd0c09e..6bcbad59 100644 --- a/src/tests/CMakeLists.txt +++ b/src/tests/CMakeLists.txt @@ -72,18 +72,9 @@ if(UNIT_TEST_SOURCES) ${LIVEKIT_ROOT_DIR}/include ${LIVEKIT_ROOT_DIR}/src ${LIVEKIT_ROOT_DIR}/src/trace - ${LIVEKIT_BINARY_DIR}/generated ${CMAKE_CURRENT_SOURCE_DIR}/benchmark - ${Protobuf_INCLUDE_DIRS} ) - if(TARGET absl::base) - get_target_property(_livekit_unit_test_absl_inc absl::base INTERFACE_INCLUDE_DIRECTORIES) - if(_livekit_unit_test_absl_inc) - target_include_directories(livekit_unit_tests PRIVATE - ${_livekit_unit_test_absl_inc} - ) - endif() - endif() + livekit_add_system_include_dirs(livekit_unit_tests ${LIVEKIT_EXTERNAL_SYSTEM_INCLUDE_DIRS}) target_compile_definitions(livekit_unit_tests PRIVATE @@ -170,18 +161,9 @@ if(INTEGRATION_TEST_SOURCES) PRIVATE ${LIVEKIT_ROOT_DIR}/include ${LIVEKIT_ROOT_DIR}/src - ${LIVEKIT_BINARY_DIR}/generated ${CMAKE_CURRENT_SOURCE_DIR}/benchmark - ${Protobuf_INCLUDE_DIRS} ) - if(TARGET absl::base) - get_target_property(_livekit_test_absl_inc absl::base INTERFACE_INCLUDE_DIRECTORIES) - if(_livekit_test_absl_inc) - target_include_directories(livekit_integration_tests PRIVATE - ${_livekit_test_absl_inc} - ) - endif() - endif() + livekit_add_system_include_dirs(livekit_integration_tests ${LIVEKIT_EXTERNAL_SYSTEM_INCLUDE_DIRS}) target_compile_definitions(livekit_integration_tests PRIVATE @@ -258,9 +240,8 @@ if(STRESS_TEST_SOURCES) PRIVATE ${LIVEKIT_ROOT_DIR}/include ${LIVEKIT_ROOT_DIR}/src - ${LIVEKIT_BINARY_DIR}/generated - ${Protobuf_INCLUDE_DIRS} ) + livekit_add_system_include_dirs(livekit_stress_tests ${LIVEKIT_EXTERNAL_SYSTEM_INCLUDE_DIRS}) target_compile_definitions(livekit_stress_tests PRIVATE diff --git a/src/video_source.cpp b/src/video_source.cpp index 468b900d..e173fe7b 100644 --- a/src/video_source.cpp +++ b/src/video_source.cpp @@ -63,7 +63,7 @@ void VideoSource::captureFrame(const VideoFrame& frame, const VideoCaptureOption } void VideoSource::captureFrame(const VideoFrame& frame, std::int64_t timestamp_us, VideoRotation rotation) { - captureFrame(frame, VideoCaptureOptions{timestamp_us, rotation}); + captureFrame(frame, VideoCaptureOptions{timestamp_us, rotation, {}}); } } // namespace livekit From 69b61175080f4af7bbf27569a88879fd40c8c702 Mon Sep 17 00:00:00 2001 From: Alan George Date: Tue, 19 May 2026 12:15:57 -0700 Subject: [PATCH 2/5] Remove accidental file --- PRE_RELEASE_AUDIT.md | 266 ------------------------------------------- 1 file changed, 266 deletions(-) delete mode 100644 PRE_RELEASE_AUDIT.md diff --git a/PRE_RELEASE_AUDIT.md b/PRE_RELEASE_AUDIT.md deleted file mode 100644 index 7ab7fc39..00000000 --- a/PRE_RELEASE_AUDIT.md +++ /dev/null @@ -1,266 +0,0 @@ -# Pre-Release Low-Hanging Fruit Audit - -> **Context:** Audit performed on `feature/unified_method_style_tmp_participant` -> branch in preparation for a major release. Breaking changes are acceptable. -> Constraint: **do not remove deprecated code in this PR** — that is scheduled -> for a separate follow-up. -> -> Date generated: Friday, May 15, 2026 - ---- - -## Category A — Quick TODO resolutions (≤ a few lines each) - -### A1. `src/ffi_client.cpp:257` — silent integer truncation in FFI callback - -```cpp -event.ParseFromArray(buf, - static_cast(len)); // TODO: this fixes for now, what if len exceeds int? -``` - -`size_t → int` cast can wrap for buffers > 2 GB. Cheap fix: bounds check + -`LK_LOG_ERROR` + early return. ~3 lines. - -### A2. `src/audio_stream.cpp:145, 174` — dead-code TODOs - -```cpp -// TODO, sample_rate and num_channels are not useful in AudioStream, remove it from FFI. -// new_audio_stream->set_sample_rate(options_.sample_rate); -// new_audio_stream->set_num_channels(options.num_channels); -``` - -Two duplicate TODOs with commented-out FFI fields. The Rust side ignores them. -Either delete both comment blocks (resolution: "won't do") or actually drop the -fields from the proto. Cheapest path is just remove the dead `//` lines — -they've been dead long enough that we know we don't need them. - -### A3. `src/room_proto_converter.cpp:103` — `toDisconnectReason` is a stub - -```cpp -DisconnectReason toDisconnectReason(proto::DisconnectReason /*in*/) { - // TODO: map each proto::DisconnectReason to your DisconnectReason enum - return DisconnectReason::Unknown; -} -``` - -This is **functionally broken** — every `DisconnectReason` event delivered to -consumers comes through as `Unknown`. Real fix is a switch over the proto enum -(~20 lines, mechanical). High-impact, low-effort. **Strong candidate for -pre-release fix.** - -### A4. `src/video_stream.cpp:151` — stale TODO - -```cpp -// TODO, do we need to cache the metadata from stream.info ? -``` - -`AudioStream` doesn't cache it either, and no caller asks for it. Resolve by -deleting the comment. - -### A5. `src/local_participant.cpp:91` — `publishDtmf` ignores destinations - -```cpp -// TODO, should we take destination as inputs? -const std::vector destination_identities; -``` - -This is a public-API design decision. For a major release where breaks are OK, -adding an optional `destinations` parameter to `publishDtmf` is the right time. -~5 lines + 1 doc-comment update. - -### A6. `src/room.cpp:1087` — `kConnectionStateChanged` doesn't update `connection_state_` - -```cpp -// TODO, maybe we should update our |connection_state_| correspoindingly, -// but the this kConnectionStateChanged event is never triggered in my local test. -``` - -This is a real correctness gap (typo "correspoindingly" too). Either: (a) write -the assignment under the lock (already held), or (b) verify the event is -actually unused and delete the case. Both are tiny. - -### A7. `src/room_proto_converter.cpp:287, 295` — unmapped reader handles - -```cpp -ByteStreamOpenedEvent fromProto(const proto::ByteStreamOpened& in) { - // TODO: map reader handle once OwnedByteStreamReader is known -``` - -The `reader_handle` field is being silently dropped from -`ByteStreamOpenedEvent` / `TextStreamOpenedEvent`. Worth flagging for product -owner: is the reader handle exposed through the C++ API? If not, drop the -comments and the unused field on the event struct. If it is, wire it up. - -### A8. `src/tests/integration/test_data_track.cpp:490` — flaky test - -```cpp -// TODO(BOT-347): this sometimes fails with a timeout. -``` - -Has a tracking ticket. Not low-hanging in the code sense, but worth keeping on -the radar for the release sign-off. - -### A9. `bridge/README.md:78` — TODO from another author - -``` -TODO(sderosa): add instructions on how to use the bridge in your own CMake project. -``` - -Bridge is being deleted on 06/01/2026 anyway (see B1). Safe to drop the TODO. - -### A10. `.github/workflows/make-release.yml:247` — Windows fix-up - -``` -# TODO(sxian): fix it after getting access to a windows machine -``` - -Probably blocked on hardware. Leave but worth confirming if it's been resolved. - ---- - -## Category B — Approaching deprecation deadlines - -### B1. Two deprecation deadlines fall on `06/01/2026` (~16 days from audit date) - -`README.md:624-625` and supporting deprecation messages in -`include/livekit/room.h` and `include/livekit/subscription_thread_dispatcher.h`: - -- `bridge/` (`livekit_bridge`) folder removal -- `setOnAudioFrameCallback(TrackSource)` / `setOnVideoFrameCallback(TrackSource)` - overloads removal - -Action: **either** push the date out to align with the major release window, -**or** keep the date and queue a follow-up PR to actually remove them on -06/01/2026. Right now the dates will silently slip with no enforcement. Just -flag — don't change without a product call. - -### B2. Internal callers of soon-to-be-removed callback overloads - -`src/tests/unit/test_room_callbacks.cpp` and -`src/tests/unit/test_subscription_thread_dispatcher.cpp` call -`setOnAudioFrameCallback(participant, TrackSource, …)` ~30+ times. When the -overload is removed, those tests break. They need migration to the track-name -overload before B1's deadline. **Not** low-hanging — substantial test refactor -— but worth scheduling now so it isn't a blocker on deletion day. - ---- - -## Category C — Stale doc comments / minor hygiene - -### C1. `include/livekit/participant.h:63-99` — references to no-longer-existent siblings - -Each deprecated `set_*` carries the comment -`// Deprecated - see setName() (also deprecated; see notes above).` But -`setName()` etc. on `Participant` were removed in this branch (the WIP that's -currently uncommitted), and there are no "notes above" anymore (the long -comment block was deleted too). Now reads as a dangling cross-reference. -~7 single-line comments to update or remove. Trivial. - -### C2. `include/livekit/track.h:91-94` — TODO-style code comment masquerading as documentation - -```cpp -// std::string can actually throw, suppressing for now to maintain API -// compatibility -// NOLINTNEXTLINE(bugprone-exception-escape) -std::optional mimeType() const noexcept { return mime_type_; } -``` - -This is the kind of `noexcept` lie that shows up as undefined behavior in the -wild. For a major release where breaks are OK, the right move is to drop -`noexcept` from `mimeType()` (and `mime_type()`). It's a one-token API change; -only effect on consumers is they can no longer use it in `noexcept`-required -contexts (rare). Worth raising for the breaking-change list. - -### C3. `src/ffi_client.h:23` — unused `` - -```cpp -#include -``` - -No `std::cout` / `std::cerr` in the file. Drop the include. Tiny. - -### C4. Per-AGENTS.md include conventions — clean - -Public headers all use `"livekit/foo.h"` (good); internal headers use bare -names (good). No issues found. - ---- - -## Category D — NOLINT suppressions worth a second look - -Most suppressions are intentional and well-scoped. A handful that could be -tightened: - -### D1. `src/ffi_client.cpp:151` — `// NOLINTNEXTLINE(modernize-use-equals-default)` - -Worth checking if the destructor body still needs to be non-trivial. If it's -just suppressing `~FfiClient() {}` vs `~FfiClient() = default;`, the lint is -right and the fix is `= default`. - -### D2. `src/data_track_stream.cpp:63` — `// NOLINT(bugprone-unchecked-optional-access)` - -Suppression is correct (the `cv_.wait` predicate plus the `if` above prove -`frame_.has_value()`), but a one-line `assert(frame_.has_value());` or -`[[assume]]` makes the invariant self-documenting. Optional polish. - -### D3. NOLINT span in `src/subscription_thread_dispatcher.cpp:511-537, 574-602` - -`bugprone-lambda-function-name` and `bugprone-exception-escape` suppressed -across ~30-line lambdas. These are real (the lambdas catch-all and log via -`__func__`). Worth a brief comment near the `NOLINTBEGIN` explaining *why*, -not just *what*. Tiny but improves auditability. - -### D4. `src/trace/trace_event.h` and `src/trace/event_tracer.h` — file-wide NOLINT - -Both wrap the entire file in `// NOLINTBEGIN ... // NOLINTEND` (no specific -check listed). This globally disables clang-tidy. If these were ported in from -Chromium and we're not the upstream, that's defensible — note it inline. -Otherwise scope to specific checks. - -The remaining `readability-identifier-naming` suppressions are all on the -deprecated snake_case wrappers — those have to stay as long as the deprecated -symbols stay. - ---- - -## Category E — Things to flag, not fix in this pass - -These need product/architecture sign-off before changing: - -### E1. Public-API integers using bare `int` - -`AudioFrame`, `AudioSource`, `VideoSource` ctors use bare `int` for -`sample_rate`, `num_channels`, `samples_per_channel`. AGENTS.md prefers -fixed-width but explicitly says don't change existing public APIs without a -compat review. Flag for the breaking-changes list. - -### E2. `Participant::Participant(...)` ctor takes 8 positional args - -`include/livekit/participant.h:34-43`. Easy to misorder. A builder/options -struct would be more durable. Major-release scope. - -### E3. `local_participant.h:117` Python-idiom doc comment - -References `LocalParticipant.set_name` as the Python idiom — accurate but might -confuse C++ readers since our API is `setName`. Worth a brief parenthetical. - -### E4. `bridge/README.md:5` removal date - -Warns the folder is being removed on 06/01. Sync with B1 — same date. - ---- - -## Recommended quick-win batch (a single small PR) - -If you want a single low-risk PR that just clears the easy stuff before the -release branch cuts: - -- **A1** — FFI buffer length bounds-check -- **A3** — `toDisconnectReason` real mapping -- **A4, A9** — delete two stale TODO comments -- **C1** — clean up dangling `// see setName()` references in `participant.h` -- **C3** — drop unused `` -- **D1** — `= default` the `FfiClient` dtor if applicable - -That gets visibly cleaner code, fixes one real correctness bug (A3), and -leaves all the deprecated API surface untouched per the constraint. From 5e5298a45a112be0d4d7a35eb82b6ea6380fb22f Mon Sep 17 00:00:00 2001 From: Alan George Date: Tue, 19 May 2026 18:30:29 -0700 Subject: [PATCH 3/5] Try fixes --- CMakeLists.txt | 19 ++++++++++++------- scripts/clang-tidy.sh | 4 +++- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 6924211d..2b52be3b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -54,7 +54,7 @@ endif() function(livekit_enable_strict_compile_warnings target_name) if(MSVC) - target_compile_options(${target_name} PRIVATE /permissive- /Zc:__cplusplus /W4 /WX) + target_compile_options(${target_name} PRIVATE /permissive- /Zc:__cplusplus /W4 /WX /wd4251) else() target_compile_options(${target_name} PRIVATE -Wall @@ -134,12 +134,12 @@ else() message(FATAL_ERROR "No protobuf library target found (expected protobuf::libprotobuf)") endif() -set(LIVEKIT_EXTERNAL_SYSTEM_INCLUDE_DIRS +set(LIVEKIT_PROTO_INCLUDE_DIRS "${PROTO_BINARY_DIR}" ${Protobuf_INCLUDE_DIRS} ) +set(LIVEKIT_EXTERNAL_SYSTEM_INCLUDE_DIRS) foreach(_livekit_system_include_target - protobuf::libprotobuf absl::base spdlog::spdlog spdlog) @@ -154,10 +154,7 @@ foreach(_livekit_system_include_target endforeach() list(REMOVE_DUPLICATES LIVEKIT_EXTERNAL_SYSTEM_INCLUDE_DIRS) -target_include_directories(livekit_proto PRIVATE - "${PROTO_BINARY_DIR}" - ${Protobuf_INCLUDE_DIRS} -) +target_include_directories(livekit_proto PRIVATE ${LIVEKIT_PROTO_INCLUDE_DIRS}) target_link_libraries(livekit_proto PRIVATE ${LIVEKIT_PROTOBUF_TARGET}) if(TARGET absl::base) get_target_property(_absl_inc absl::base INTERFACE_INCLUDE_DIRECTORIES) @@ -444,6 +441,10 @@ add_library(livekit SHARED src/trace/trace_event.h src/trace/tracing.cpp ) +if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU") + set_source_files_properties(src/trace/event_tracer.cpp + PROPERTIES COMPILE_OPTIONS "-Wno-error=type-limits") +endif() # Symbol visibility: keep liblivekit's exported ABI restricted to the public # LiveKit API. By default everything is hidden unless marked by the LIVEKIT_API # macro in code @@ -466,6 +467,10 @@ endif() target_sources(livekit PRIVATE $) +target_include_directories(livekit + BEFORE PRIVATE + ${LIVEKIT_PROTO_INCLUDE_DIRS} +) target_include_directories(livekit PUBLIC $ diff --git a/scripts/clang-tidy.sh b/scripts/clang-tidy.sh index fdbc43b2..11327f84 100755 --- a/scripts/clang-tidy.sh +++ b/scripts/clang-tidy.sh @@ -206,7 +206,9 @@ fi # SDK and we forward it to every clang-tidy invocation via --extra-arg. Linux # CI doesn't need this -- the system clang-tidy already finds libstdc++/libc++ # through its built-in resource dir. -extra_args=() +# Match the Clang build's variadic macro diagnostic suppression when clang-tidy +# is driven from GCC compile commands in Linux CI. +extra_args=(-extra-arg=-Wno-gnu-zero-variadic-macro-arguments) if [[ "$(uname)" == "Darwin" ]]; then sdk_path="$(xcrun --show-sdk-path 2>/dev/null || true)" if [[ -n "${sdk_path}" ]]; then From abbc301de5f25015ce05f39bbfb9f4f1429d7400 Mon Sep 17 00:00:00 2001 From: Alan George Date: Tue, 19 May 2026 21:43:31 -0700 Subject: [PATCH 4/5] Maybe fix --- src/tests/CMakeLists.txt | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/tests/CMakeLists.txt b/src/tests/CMakeLists.txt index 6bcbad59..1ca7b725 100644 --- a/src/tests/CMakeLists.txt +++ b/src/tests/CMakeLists.txt @@ -67,6 +67,10 @@ if(UNIT_TEST_SOURCES) GTest::gtest_main ) + target_include_directories(livekit_unit_tests + BEFORE PRIVATE + ${LIVEKIT_PROTO_INCLUDE_DIRS} + ) target_include_directories(livekit_unit_tests PRIVATE ${LIVEKIT_ROOT_DIR}/include @@ -157,6 +161,10 @@ if(INTEGRATION_TEST_SOURCES) GTest::gtest_main ) + target_include_directories(livekit_integration_tests + BEFORE PRIVATE + ${LIVEKIT_PROTO_INCLUDE_DIRS} + ) target_include_directories(livekit_integration_tests PRIVATE ${LIVEKIT_ROOT_DIR}/include @@ -236,6 +244,10 @@ if(STRESS_TEST_SOURCES) GTest::gtest_main ) + target_include_directories(livekit_stress_tests + BEFORE PRIVATE + ${LIVEKIT_PROTO_INCLUDE_DIRS} + ) target_include_directories(livekit_stress_tests PRIVATE ${LIVEKIT_ROOT_DIR}/include From ce7e164bb4a169ced8ad67dd070873f1d33b0f17 Mon Sep 17 00:00:00 2001 From: Alan George Date: Wed, 20 May 2026 07:50:30 -0700 Subject: [PATCH 5/5] Maybe fix --- CMakeLists.txt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 2b52be3b..55c17e71 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -54,7 +54,7 @@ endif() function(livekit_enable_strict_compile_warnings target_name) if(MSVC) - target_compile_options(${target_name} PRIVATE /permissive- /Zc:__cplusplus /W4 /WX /wd4251) + target_compile_options(${target_name} PRIVATE /permissive- /Zc:__cplusplus /W4 /WX /wd4251 /wd4267) else() target_compile_options(${target_name} PRIVATE -Wall @@ -138,6 +138,9 @@ set(LIVEKIT_PROTO_INCLUDE_DIRS "${PROTO_BINARY_DIR}" ${Protobuf_INCLUDE_DIRS} ) +if(DEFINED livekit_abseil_SOURCE_DIR) + list(APPEND LIVEKIT_PROTO_INCLUDE_DIRS "${livekit_abseil_SOURCE_DIR}") +endif() set(LIVEKIT_EXTERNAL_SYSTEM_INCLUDE_DIRS) foreach(_livekit_system_include_target absl::base