RDKB-66207 BananaPI ci build check overhaul - #1320
Open
mateuszCieslak-GL wants to merge 14 commits into
Open
Conversation
Contributor
Author
|
CC: @bmilcz-comcast @WellPeter @amarnathhullur please take a look |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR overhauls the CI “makefile” build checks to increase build gating coverage (more apps/features), prepare for upcoming GitHub runner/toolchain changes, and make builds faster/more reliable (parallel compilation + better dependency ordering).
Changes:
- Pin CI runner to Ubuntu 24.04, update GitHub Actions versions, add missing system deps (e.g., protobuf-c), and enable parallel builds (
make -j). - Expand BananaPi CI coverage (additional OneWifi apps + optional “EXTRA_COVERAGE” tier) and add compat headers/stubs under
build/linux/compatto keep the coverage build linkable. - Make
make -jsafe by explicitly ordering program link vslibwifihal.acreation across affected platform makefiles; adjust symbol collision handling (hwaddr_atonweak).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| source/apps/em/wifi_em_utils.h | Removes unused radio_index_to_radio_type_str declaration from EM utils. |
| source/apps/em/wifi_em_utils.c | Removes dead/incorrect radio_index_to_radio_type_str implementation. |
| lib/common/os.c | Makes hwaddr_aton weak to avoid link-time symbol collisions in CI coverage builds. |
| build/linux/rpi/makefile | Adds libwifihal.a as an explicit prerequisite for parallel-safe linking; fixes clean spacing. |
| build/linux/mockplatform/makefile | Adds libwifihal.a as an explicit prerequisite for parallel-safe linking. |
| build/linux/compat/secure_wrapper.h | Adds a CI-only compat header for libsecure_wrapper APIs (prototype needs adjustment—see comment). |
| build/linux/compat/safec_lib_common.h | Adds a CI-only compat header mapping a subset of safeclib APIs to libc. |
| build/linux/compat/rbus/rbus.h | Adds an empty stub header to satisfy <rbus/rbus.h> includes in coverage builds. |
| build/linux/compat/coverage_stubs.c | Adds weak, no-op symbols to let coverage builds link without full HAL/DB support. |
| build/linux/bpi/makefile | Adds coverage tiers/flags, expands app/source coverage, adds compat include path + protobuf-c, scopes hostap/HAL warning behavior, and makes linking parallel-safe. |
| .github/workflows/makefile.yml | Pins runner to ubuntu-24.04, updates action versions, installs protobuf-c dev package, enables parallel make, and passes coverage-tier env. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
build/linux/bpi/makefile:648
- Comment typo: "it's" should be "its" (possessive) in this sentence; also consider capitalizing for readability since this comment documents warning-scoping behavior.
#scope out HOSTAP. so it's code does not generate ANY warnings
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (3)
build/linux/compat/telemetry_busmessage_sender.h:51
- The telemetry stub uses non-const parameters and leaves them unused. This can trigger warnings (e.g., discarded qualifiers when calling
t2_init("..."), and-Wunused-parameterwhen compiling the inline stubs). Adjust the signatures to acceptconstwhere appropriate and explicitly(void)unused parameters.
static inline void t2_init(char* name) {
// Stub: do nothing
}
static inline T2ERROR t2_event_s(const char* marker, char* value) {
build/linux/bpi/makefile:648
- Typo in the new makefile comment: use "its" (possessive) instead of "it's".
#scope out HOSTAP. so it's code does not generate ANY warnings
build/linux/compat/safec_lib_common.h:66
- The
sprintf_scompatibility wrapper currently treats truncation (whenvsnprintfreturns >=dmax) as success. Real safeclib treats this as an error, and several call sites only checkrc < EOK; letting truncation pass here can mask bugs that would fail on real RDK builds. Consider converting truncation into an error return and clearingdestsimilarly to safeclib constraint handling.
static inline int sprintf_s(char *dest, rsize_t dmax, const char *fmt, ...)
{
va_list ap;
int rc;
if (dest == NULL || fmt == NULL || dmax == 0) {
mateuszCieslak-GL
force-pushed
the
build_gate
branch
from
August 5, 2026 16:15
1db77df to
92bffc1
Compare
bmilcz-comcast
approved these changes
Aug 5, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (3)
build/linux/compat/safec_lib_common.h:66
sprintf_swrapper currently returnsvsnprintf()'s value directly. On truncationvsnprintf()returns a non-negative value >= dmax, so existing call sites that rely onif (rc < EOK)(e.g. source/apps/whix/wifi_whix.c:315) will treat truncated output as success, diverging from safeclib behavior and potentially hiding real errors. Consider returning a negative value (and clearing dest) when output would not fit.
* Truncation (output would exceed dmax-1 chars) is an ERROR in real safeclib,
* which clears dest and returns a negative constraint code (-ESNOSPC) - not the
* would-be length. vsnprintf instead returns that positive would-be length on
* truncation, which slips through callers' `rc < EOK` checks and would mask a
* bug that fails on a real RDK build. Convert it to the safeclib error form. */
static inline int sprintf_s(char *dest, rsize_t dmax, const char *fmt, ...)
{
va_list ap;
int rc;
if (dest == NULL || fmt == NULL || dmax == 0) {
return -1;
}
build/linux/compat/telemetry_busmessage_sender.h:28
- Header comment and inline notes refer to "unit test" usage, but this file is included by non-test code paths in the repo (e.g. include/tr_181/ml/cosa_wifi_internal.h). Updating the wording to match its actual purpose (CI coverage-build compatibility header) will avoid confusion for future maintainers.
/*
* Mock header for unit test simulation of telemetry_busmessage_sender.h
*/
#ifndef TELEMETRY_BUSMESSAGE_SENDER_H
#define TELEMETRY_BUSMESSAGE_SENDER_H
// Minimal type definitions for unit test
#include <stddef.h>
#include <stdbool.h>
build/linux/bpi/makefile:658
- This comment line ends with a trailing
\, which make will treat as a line-continuation before stripping comments. It’s harmless today because the next line is also a comment, but it’s easy to accidentally turn into a real parsing bug during future edits. Removing the continuation avoids that risk.
# OneWifi/hostap objects are unaffected. Warnings flagged by -Wall and -Wextra stay visible, \
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (2)
build/linux/compat/telemetry_busmessage_sender.h:34
t2_inittakeschar*, but call sites pass string literals (e.g.t2_init("ccsp-wifi-agent")), which triggers a discarded-qualifiers warning and can become an error under stricter warning policies. Useconst char*for the parameter to match typical usage.
static inline void t2_init(char* name) {
build/linux/bpi/makefile:648
- Typo in comment: use possessive "its" instead of "it's".
#scope out HOSTAP. so it's code does not generate ANY warnings
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (2)
build/linux/bpi/makefile:677
- Grammar in comment: "splitted" should be "split".
# never leak onto each other's objects. Warnings are splitted by language used.
build/linux/bpi/makefile:648
- Typo in comment: use possessive "its" (not "it's").
This issue also appears on line 677 of the same file.
#scope out HOSTAP. so it's code does not generate ANY warnings
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (3)
build/linux/compat/safec_lib_common.h:102
strncpy_ssilently truncates whenn >= dmaxand still returnsEOK. Real safeclib treatsn >= dmaxas a runtime-constraint violation (clearsdestand returns non-EOK). The current behavior can mask truncation bugs that would fail on real RDK builds.
static inline errno_t strncpy_s(char *dest, rsize_t dmax, const char *src, rsize_t n)
{
if (dest == NULL || src == NULL || dmax == 0) {
return -1;
}
if (n >= dmax) {
n = dmax - 1;
}
strncpy(dest, src, n);
dest[n] = '\0';
return EOK;
}
build/linux/compat/safec_lib_common.h:116
strcat_sreturns an error on overflow, but unlike safeclib it does not cleardest. It also treats a non-NUL-terminateddest(withindmax) as an overflow case but still leaves the unterminated buffer intact. Clearingdeston these runtime-constraint violations better matches safeclib behavior and avoids callers accidentally using a partially valid/unterminated string.
static inline errno_t strcat_s(char *dest, rsize_t dmax, const char *src)
{
size_t dlen;
if (dest == NULL || src == NULL || dmax == 0) {
return -1;
}
dlen = strnlen(dest, dmax);
if (dlen + strlen(src) >= dmax) {
return -1;
}
strcat(dest, src);
return EOK;
}
build/linux/bpi/makefile:658
- This comment line ends with a trailing
\, which GNU make treats as a line-continuation before comment stripping. It currently happens to continue into another comment line, but it’s brittle and can accidentally comment-out the next non-comment line if edited later. Remove the trailing backslash.
# OneWifi/hostap objects are unaffected. Warnings flagged by -Wall and -Wextra stay visible, \
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (1)
build/linux/bpi/makefile:658
- This comment line ends with a trailing
\, which in GNU make escapes the newline before parsing. Even though the next line is also a comment today, this can accidentally concatenate lines and cause confusing behavior if the surrounding lines change later. Remove the trailing backslash to keep the makefile parse unambiguous.
# OneWifi/hostap objects are unaffected. Warnings flagged by -Wall and -Wextra stay visible, \
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (5)
source/db/wifi_db_apis.c:1939
sprintf()writes without a bound; even with a largerindexbuffer this is still an avoidable overflow risk. Prefersnprintf(index, sizeof(index), ...)so the conversion can’t overrun the stack buffer.
This issue also appears in the following locations of the same file:
- line 6351
- line 6442
- line 6500
char index[12] = {0};
wifi_db_t *g_wifidb;
g_wifidb = (wifi_db_t*) get_wifidb_obj();
sprintf(index,"%d",rfc_id);
where = onewifi_ovsdb_tran_cond(OCLM_STR, "rfc_id", OFUNC_EQ, index);
source/db/wifi_db_apis.c:6447
sprintf()is unbounded. Switching tosnprintf()avoids a potential stack overflow ifadvertisement_idformatting exceeds the buffer.
char index[12] = {0};
wifi_db_t *g_wifidb;
g_wifidb = (wifi_db_t*) get_wifidb_obj();
sprintf(index,"%d",advertisement_id);
where = onewifi_ovsdb_tran_cond(OCLM_STR, "advertisement_id", OFUNC_EQ, index);
source/db/wifi_db_apis.c:6505
sprintf()is unbounded. Usesnprintf()to guarantee theindexbuffer is not overrun when formattingadvertisement_id.
char index[12] = {0};
wifi_db_t *g_wifidb;
g_wifidb = (wifi_db_t*) get_wifidb_obj();
sprintf(index,"%d",advertisement_id);
where = onewifi_ovsdb_tran_cond(OCLM_STR, "advertisement_id", OFUNC_EQ, index);
source/db/wifi_db_apis.c:6362
sprintf()writes without a bound; usesnprintf()to prevent potential overflow if the numeric ID is larger than expected.
char index[12] = {0};
wifi_db_t *g_wifidb;
g_wifidb = (wifi_db_t*) get_wifidb_obj();
if (rfc_param == NULL) {
wifi_util_error_print(WIFI_DB, "%s:%d: rfc_param is NULL\n", __func__, __LINE__);
return -1;
}
wifi_util_error_print(WIFI_DB, "%s:%d:rfc_param->link_quality_rfc =%d\n", __func__, __LINE__,rfc_param->link_quality_rfc);
sprintf(index,"%d",rfc_id);
where = onewifi_ovsdb_tran_cond(OCLM_STR, "rfc_id", OFUNC_EQ, index);
lib/common/os.c:167
- The comment says this is a CI-only change, but the
weakattribute is applied unconditionally in this library and will affect all builds. Update the comment to avoid misleading future readers about the scope/impact of making this symbol weak.
/* CI coverage build: added 'weak' attr so hostap's identical hwaddr_aton (linked via
* libwifihal.a) wins instead of colliding; Nothing uses this copy directly. */
__attribute__((weak))
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (5)
source/db/wifi_db_apis.c:1938
indexis filled usingsprintf(index, "%d", rfc_id), butrfc_idisUINT(unsigned) and%dis the wrong format specifier; alsosprintfdoesn’t enforce the destination buffer size. Usesnprintfwith%uto avoid UB and keep the write bounded.
This issue also appears on line 6351 of the same file.
char index[12] = {0};
wifi_db_t *g_wifidb;
g_wifidb = (wifi_db_t*) get_wifidb_obj();
sprintf(index,"%d",rfc_id);
source/db/wifi_db_apis.c:6351
indexis populated withsprintf(index, "%d", rfc_id), butrfc_idisUINT(unsigned) and%dis the wrong format specifier; alsosprintfis unbounded. Prefersnprintfwith%uso the write is size-limited and the format matches the argument type.
char index[12] = {0};
source/db/wifi_db_apis.c:6446
advertisement_idisUINT(unsigned), butsprintf(index, "%d", advertisement_id)uses%dand is unbounded. Usesnprintfwith%uto avoid UB and keep the write withinindex.
This issue also appears on line 6500 of the same file.
char index[12] = {0};
wifi_db_t *g_wifidb;
g_wifidb = (wifi_db_t*) get_wifidb_obj();
sprintf(index,"%d",advertisement_id);
source/db/wifi_db_apis.c:6504
advertisement_idisUINT(unsigned), butsprintf(index, "%d", advertisement_id)uses%dand is unbounded. Switch tosnprintfwith%uso the conversion is type-correct and size-bounded.
char index[12] = {0};
wifi_db_t *g_wifidb;
g_wifidb = (wifi_db_t*) get_wifidb_obj();
sprintf(index,"%d",advertisement_id);
build/linux/compat/telemetry_busmessage_sender.h:34
t2_initis called with a string literal (e.g.t2_init("ccsp-wifi-agent")), so takingchar*forces an unnecessary const-discard at call sites. Make the parameterconst char *to match typical usage and avoid warnings when stricter flags are enabled.
static inline void t2_init(char* name) {
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (8)
source/db/wifi_db_apis.c:6351
- Same issue here:
sprintf(index, "%d", rfc_id)uses a signed format for aUINTand is unbounded. Prefersnprintfwith%uandsizeof(index)to avoid incorrect keys and potential overflows.
char index[12] = {0};
source/db/wifi_db_apis.c:6442
advertisement_idis aUINT, but this uses%dand unboundedsprintf, which can produce wrong keys for large IDs. Use boundedsnprintfwith an unsigned format.
char index[12] = {0};
source/db/wifi_db_apis.c:6500
advertisement_idis aUINT, but this uses%dand unboundedsprintf. Switch tosnprintfwith%uandsizeof(index)to prevent incorrect IDs and make the buffer size change actually enforce safety.
char index[12] = {0};
source/db/wifi_db_apis.c:1938
sprintf(index, "%d", ...)is using a signed format for aUINTID and is unbounded. If the ID exceedsINT_MAXthis will format a negative value and generate an incorrect OVSDB key;sprintfalso risks overflow if the typedef changes. Usesnprintfwith the correct unsigned format andsizeof(index).
This issue also appears in the following locations of the same file:
- line 6351
- line 6442
- line 6500
char index[12] = {0};
wifi_db_t *g_wifidb;
g_wifidb = (wifi_db_t*) get_wifidb_obj();
sprintf(index,"%d",rfc_id);
.github/workflows/makefile.yml:146
- GitHub Actions workflow commands (
::error::...) require escaping not only%but also carriage returns/newlines in the message. As-is, a\r(e.g. CRLF logs) can corrupt the annotation rendering. Escape\r/\nalongside%before emitting the command.
printf '%s\n' "$errs" | head -10 | sed 's/%/%25/g' \
| while IFS= read -r l; do echo "::error::$l"; done
build/linux/bpi/makefile:657
- This comment line ends with a trailing
\, which triggers Make's line-continuation behavior and will join with the next line before parsing. It currently happens to join another comment, but it's brittle and can accidentally comment-out or alter future edits. Remove the trailing backslash.
# OneWifi/hostap objects are unaffected. Warnings flagged by -Wall and -Wextra stay visible, \
build/linux/compat/safec_lib_common.h:102
strncpy_scurrently truncates whenn >= dmaxand still returnsEOK. That deviates from safeclib’s constraint-handling and can mask truncation bugs that would fail in real RDK builds (similar to what you already guarded against insprintf_s). Consider treatingn >= dmaxas an error and clearingdestto better match safeclib behavior.
if (n >= dmax) {
n = dmax - 1;
}
strncpy(dest, src, n);
dest[n] = '\0';
.github/workflows/makefile.yml:197
- Same escaping issue for
::warning::...workflow commands: escape\r/\n(and%) to avoid malformed annotations when log lines contain CRLF or embedded control characters.
printf '%s\n' "$found" | head -10 | sed 's/%/%25/g' \
| while IFS= read -r l; do echo "::warning::$l"; done
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Use clean -Werr promotion, disable noisy warning flags - based on yocto flag set.
mateuszCieslak-GL
force-pushed
the
build_gate
branch
from
August 6, 2026 14:18
4a09471 to
654f7b4
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (4)
source/db/wifi_db_apis.c:1938
rfc_idis aUINT, but the code formats it with%dand uses unboundedsprintf, which can trigger-Werror=formatand is undefined behavior for mismatched types. Usesnprintfwith an unsigned format (or an explicit cast) to keep this safe under the new warning gating.
This issue also appears on line 6351 of the same file.
char index[12] = {0};
wifi_db_t *g_wifidb;
g_wifidb = (wifi_db_t*) get_wifidb_obj();
sprintf(index,"%d",rfc_id);
source/db/wifi_db_apis.c:6446
advertisement_idis aUINT, but this block formats it with%din both the OVSDB key string and the log message. Under the new-Werror=formatpolicy this will fail the build, andsprintfis unbounded. Usesnprintfand an unsigned format (or cast) for theUINT.
This issue also appears on line 6500 of the same file.
char index[12] = {0};
wifi_db_t *g_wifidb;
g_wifidb = (wifi_db_t*) get_wifidb_obj();
sprintf(index,"%d",advertisement_id);
source/db/wifi_db_apis.c:6504
advertisement_idis aUINT, but it is formatted with%dviasprintf. This is a format-type mismatch (undefined behavior) and can become a build failure under-Werror=format. Use boundedsnprintfwith an unsigned format/cast.
char index[12] = {0};
wifi_db_t *g_wifidb;
g_wifidb = (wifi_db_t*) get_wifidb_obj();
sprintf(index,"%d",advertisement_id);
source/db/wifi_db_apis.c:6351
rfc_idis aUINT, but this block formats it with%din both the string key and the log message. With-Werror=formatenabled for OneWifi sources, this is a build-breaking format-type mismatch. Also prefer boundedsnprintfoversprintf.
char index[12] = {0};
Contributor
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (7)
source/db/wifi_db_apis.c:1938
rfc_idis aUINT, but this builds the index string withsprintf(index, "%d", rfc_id), which uses the wrong format specifier for an unsigned type and has no bounds check. This can mis-format values > INT_MAX and is avoidable since you already size the buffer for the decimal representation.
This issue also appears on line 6351 of the same file.
char index[12] = {0};
wifi_db_t *g_wifidb;
g_wifidb = (wifi_db_t*) get_wifidb_obj();
sprintf(index,"%d",rfc_id);
source/db/wifi_db_apis.c:6351
rfc_idis aUINT, but the index string is still built withsprintf(index, "%d", rfc_id)(signed format + no bounds). Usesnprintfwith%uso the value is formatted correctly and bounded by theindexbuffer size.
char index[12] = {0};
source/db/wifi_db_apis.c:6446
advertisement_idis aUINT, but this usessprintf(index, "%d", advertisement_id)(signed format + unbounded write). Usesnprintfwith%uto avoid UB for values > INT_MAX and keep the write bounded.
This issue also appears on line 6500 of the same file.
char index[12] = {0};
wifi_db_t *g_wifidb;
g_wifidb = (wifi_db_t*) get_wifidb_obj();
sprintf(index,"%d",advertisement_id);
source/db/wifi_db_apis.c:6504
advertisement_idis aUINT, but this usessprintf(index, "%d", advertisement_id)(signed format + no bounds). Prefersnprintfwith%uso the output is correct and bounded by the buffer size.
char index[12] = {0};
wifi_db_t *g_wifidb;
g_wifidb = (wifi_db_t*) get_wifidb_obj();
sprintf(index,"%d",advertisement_id);
build/linux/compat/telemetry_busmessage_sender.h:25
- This header is described as being for “unit test simulation”, but it’s being added under
build/linux/compatand used as a CI coverage-build compatibility shim. The comment (and the “unit test” wording below) is misleading for future maintainers.
/*
* Mock header for unit test simulation of telemetry_busmessage_sender.h
*/
build/linux/compat/secure_wrapper.h:30
- The comment says this file “must never be pushed”, but it’s now part of this repository. If the intent is “not part of upstream RDK / only for CI linux coverage builds”, please reword so the guidance is actionable in this repo context.
* This file is NOT upstream and must never be pushed; it lives under
* build/linux/compat and is only on the include path for the coverage builds.
.github/workflows/clang-format.yml:29
- Typo in the comment: “merege commit” → “merge commit”.
# For pull_request events actions/checkout defaults to the merege commit
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.
As part of repo maintenance efforts, this PR expands and starts gating on the build results.
Changes include (might be not exhaustive)
Preparation for gating builds on code presented in PR :
BPI Build will be gated on failure only if PR code will introduce warning from the already fixed class (~9Werror flags). For now clang-tidy and clang-formatter won't fail the build.
ToDo (in separate PRs) :
3. Fix single occurences to promote warning flags to errors, so no new-ones are introduced:
- int-conversion: 1 occurence
- stringop-overread: 1
- sizeof-pointer-memaccess: 1
- missing-field-initializers: 2
- format-security: 2
- address: 3
- maybe-uninitialized: 3