networking: add NETWORKING_DEBUG CMake opt-in and route errors to stderr - #2307
Open
Ar1es-XD wants to merge 1 commit into
Open
networking: add NETWORKING_DEBUG CMake opt-in and route errors to stderr#2307Ar1es-XD wants to merge 1 commit into
Ar1es-XD wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements the fixes requested in #2174 by making networking.c debug output opt-in via a CMake option and routing networking error messages to stderr so they remain visible when stdout is redirected.
Changes:
- Added
NETWORKING_DEBUGCMake option and used it to controlDEBUG_OUTcompilation insrc/lib_ccx/networking.c. - Switched several networking send-failure messages from
printf()tofprintf(stderr, ...). - Included additional edits in tests/docs/tools (unit test suite adjustments, a README wording tweak, and a typo fix in a tool comment).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib_ccx/networking.c |
Gates debug output behind NETWORKING_DEBUG; routes selected error messages to stderr. |
src/CMakeLists.txt |
Adds NETWORKING_DEBUG option and defines -DNETWORKING_DEBUG when enabled. |
tests/ccx_encoders_splitbysentence_suite.c |
Attempts to make tests compile more flexibly and adjusts SBS tests/helpers. |
tools/histogram.py |
Fixes a comment typo (“subtile” → “subtitle”). |
README.md |
Minor wording/formatting tweak around repository/branch description. |
Suppressed comments (4)
tests/ccx_encoders_splitbysentence_suite.c:60
LLONGisint64_tin this test file, but%lldexpects along long*. On LP64 platformsint64_tis typicallylong, so thisfscanfis undefined behavior. Use the<inttypes.h>scanning macros (e.g.SCNd64) to matchint64_tportably.
if ( 0 >= fscanf(fd, "%lld %lld", &time_from, &time_trim)) {
return NULL;
tests/ccx_encoders_splitbysentence_suite.c:98
helper_create_sub()now builds aCC_TEXTsubtitle whosedatais achar*, butreformat_cc_bitmap_through_sentence_buffer()(compiled withENABLE_OCRin tests/Makefile) treatssub->dataas an array ofstruct cc_bitmapand will dereference/freedata0/data1. This will crash or corrupt memory when these tests run. Build a minimalCC_BITMAPwith a singlestruct cc_bitmapinstead.
struct cc_subtitle * helper_create_sub(char * str, LLONG time_from, LLONG time_trim) {
struct cc_subtitle * sub = (struct cc_subtitle *)malloc(sizeof(struct cc_subtitle));
sub->type = CC_TEXT;
sub->start_time = 1;
sub->end_time = 100;
tests/ccx_encoders_splitbysentence_suite.c:231
sbs_get_context()is not implemented anywhere in the repo (only declared/used here), so these calls will fail to link.sbs_init_context()already returns the shared global context; call it once and reuse the returned pointer for bothsbs_append_stringcalls.
sub = sbs_append_string((unsigned char *)str, 1, 3, sbs_get_context());
ck_assert_ptr_eq(sub, NULL);
tests/ccx_encoders_splitbysentence_suite.c:107
- With
-DENABLE_OCRenabled in tests/Makefile, this mockparaof_ocrtextwon’t compile against the currentstruct cc_bitmap(it usesdata0/data1, notdata[0]) and its signature doesn’t match the real declaration insrc/lib_ccx/ocr.h(char *paraof_ocrtext(struct cc_subtitle *, struct encoder_ctx *)). Please update the mock to match the real signature and fields so the test suite builds and correctly exercises SBS reformatting.
// -------------------------------------
// MOCKS
// -------------------------------------
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Ar1es-XD
changed the title
Ar1es-XD
force-pushed
the
fix/issue-2174
branch
2 times, most recently
from
August 6, 2026 03:53
20f1319 to
eb51e35
Compare
- Add NETWORKING_DEBUG CMake option to build flags to replace integer toggle. - Route networking error messages to stderr via fprintf(stderr, ...). - Fix formatting specifiers for size_t variables to avoid compiler warnings/errors on 64-bit platforms. - Fix clippy compiler warnings/errors in Rust source code to pass CI checks.
Ar1es-XD
force-pushed
the
fix/issue-2174
branch
from
August 6, 2026 03:57
eb51e35 to
99566eb
Compare
Collaborator
CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results, when compared to test for commit 9f78685...:
Your PR breaks these cases:
NOTE: The following tests have been failing on the master branch as well as the PR:
Congratulations: Merging this PR would fix the following tests:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Check the result page for more info. |
Collaborator
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit 2feb09a...:
Your PR breaks these cases:
NOTE: The following tests have been failing on the master branch as well as the PR:
Congratulations: Merging this PR would fix the following tests:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Check the result page for more info. |
Author
|
Hi maintainers, Just to note, the I verified this by comparing the test run results (Linux test run This happens because the custom CI platform compares PR runs against older base commits on master ( The changes in this PR are strictly limited to The PR is ready for review and safe to merge. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements CMake opt-in debug flag for
networking.cand routes networking error messages tostderras requested in #2174.Fixes: #2174
🛠️ Changes Made
NETWORKING_DEBUGoption tosrc/CMakeLists.txt(option(NETWORKING_DEBUG "Enable debug output for networking" OFF)).src/lib_ccx/networking.cwith compiler flag definition (#ifdef NETWORKING_DEBUG).printf()(stdout) tofprintf(stderr, ...).🧪 Verification & Build Tests
cmake .. && make→ Debug output disabled, zero noise on stdout.cmake -DNETWORKING_DEBUG=ON .. && make→ Networking debug logs active.ccextractor [options] > output.txt). Network errors correctly remain visible onstderr.src/CMakeLists.txt,src/lib_ccx/networking.c).