RDKB-62974 RDKB-62976: Native build for Coverity - Updating code#75
RDKB-62974 RDKB-62976: Native build for Coverity - Updating code#75GoutamD2905 merged 3 commits intodevelopfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Coverity native build system by centralizing build scripts in an external build_tools_workflows repository. Instead of maintaining local copies of build scripts, the component now uses lightweight wrapper scripts that clone and invoke the centralized build tools.
Changes:
- Removed four local build scripts (setup_dependencies.sh, build_native.sh, common_build_utils.sh, common_external_build.sh) totaling ~1,068 lines
- Added two wrapper scripts (run_setup_dependencies.sh and run_native_build.sh) that clone and use scripts from build_tools_workflows repository
- Simplified README from 1,459 to 402 lines, focusing on wrapper script usage rather than comprehensive build system documentation
- Updated GitHub Actions workflow to use new wrapper scripts
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| cov_docker_script/setup_dependencies.sh | Removed: Dependency setup logic now sourced from build_tools_workflows |
| cov_docker_script/build_native.sh | Removed: Component build logic now sourced from build_tools_workflows |
| cov_docker_script/common_build_utils.sh | Removed: Utility functions now sourced from build_tools_workflows |
| cov_docker_script/common_external_build.sh | Removed: Build orchestration now handled by separate wrapper scripts |
| cov_docker_script/run_setup_dependencies.sh | Added: Wrapper that clones build_tools_workflows and runs dependency setup |
| cov_docker_script/run_native_build.sh | Added: Wrapper that verifies build tools and runs component build |
| cov_docker_script/README.md | Simplified: Updated to document wrapper script usage instead of full build system |
| .github/workflows/native-build.yml | Updated: Modified to use new wrapper scripts instead of direct script invocation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| log "Cloning build_tools_workflows (develop)" | ||
| cd "$NATIVE_COMPONENT_DIR" | ||
| git clone -b develop "$BUILD_TOOLS_REPO_URL" || { err "Clone failed"; exit 1; } |
There was a problem hiding this comment.
The hardcoded branch name 'develop' could cause issues if the build_tools_workflows repository changes its default branch or if different environments need to use different branches. Consider making the branch configurable through an environment variable with 'develop' as the default.
For example:
BUILD_TOOLS_BRANCH="${BUILD_TOOLS_BRANCH:-develop}"
git clone -b "$BUILD_TOOLS_BRANCH" "$BUILD_TOOLS_REPO_URL"This would allow users to override the branch if needed while maintaining the current default behavior.
| if [[ -d "$BUILD_TOOLS_DIR" ]]; then | ||
| log "build_tools_workflows already exists, skipping clone" | ||
| else | ||
| log "Cloning build_tools_workflows (develop)" | ||
| cd "$NATIVE_COMPONENT_DIR" | ||
| git clone -b develop "$BUILD_TOOLS_REPO_URL" || { err "Clone failed"; exit 1; } | ||
| ok "Repository cloned" | ||
| fi |
There was a problem hiding this comment.
The script checks if build_tools_workflows already exists at line 29 and skips cloning if it does. However, there's no validation that the existing directory contains a valid, up-to-date clone of the repository. If the directory exists but is corrupted, incomplete, or from a stale clone, the script will proceed without ensuring the scripts are current.
Consider adding validation of the existing directory (e.g., verify it's a git repository and optionally pull latest changes) or providing a way to force a fresh clone through an environment variable.
| # Cleanup build_tools_workflows directory | ||
| log "Cleaning up build_tools_workflows directory..." | ||
| rm -rf "$BUILD_TOOLS_DIR" | ||
| ok "Cleanup completed" |
There was a problem hiding this comment.
The cleanup of the build_tools_workflows directory at lines 46-49 happens after the native build completes successfully. However, if the build fails (causing the script to exit due to 'set -e'), the cleanup will not occur, leaving the directory in place. This could lead to the stale directory issue mentioned in another comment, where subsequent runs might use outdated scripts.
Consider using a trap to ensure cleanup happens on both success and failure, or document this behavior clearly if it's intentional to preserve the directory for debugging failed builds.
| # Clone build_tools_workflows | ||
| if [[ -d "$BUILD_TOOLS_DIR" ]]; then | ||
| log "build_tools_workflows already exists, skipping clone" | ||
| else | ||
| log "Cloning build_tools_workflows (develop)" | ||
| cd "$NATIVE_COMPONENT_DIR" | ||
| git clone -b develop "$BUILD_TOOLS_REPO_URL" || { err "Clone failed"; exit 1; } |
There was a problem hiding this comment.
Cloning from an external GitHub repository ('develop' branch) in the build process introduces a dependency on network connectivity and the availability of the external repository. If the repository is unavailable, renamed, or the develop branch is deleted/renamed, all builds will fail. Additionally, there's a security consideration: the build process trusts whatever code is currently in the develop branch of build_tools_workflows without any verification or pinning to a specific commit/tag.
Consider one of the following approaches:
- Pin to a specific commit SHA or tag instead of a branch name for reproducibility and security
- Implement checksum verification of the cloned scripts
- Add fallback mechanisms for network failures
- Document the external dependency clearly in the PR description and component documentation
| ### Important: Add to .gitignore | ||
|
|
||
| | Component | Responsibilities | | ||
| |-----------|------------------| | ||
| | **Docker** | • Consistent build environment<br>• Pre-installed toolchain<br>• System-level dependencies | | ||
| | **GitHub Actions** | • Automated build triggers<br>• PR validation<br>• Quality gate enforcement | | ||
| | **Coverity** | • Static analysis<br>• Security scanning<br>• Code quality metrics | | ||
| Add the following to your component's `.gitignore` to exclude temporary build artifacts: | ||
|
|
||
| ### High-Level Workflow | ||
| ```gitignore | ||
| # Build tools (downloaded by wrapper scripts) | ||
| build_tools_workflows/ | ||
|
|
||
| 1. **Setup** → Docker environment prepared using [docker-rdk-ci](https://github.com/rdkcentral/docker-rdk-ci) | ||
| 2. **Integrate** → Component adds `cov_docker_script` directory | ||
| 3. **Configure** → Dependencies declared in JSON | ||
| 4. **Build** → Native build executes locally and in CI | ||
| 5. **Validate** → GitHub Actions confirms build stability | ||
| 6. **Enable** → Component eligible for Coverity scanning | ||
| 7. **Onboard** → CMFSUPPORT ticket raised | ||
| # Dependency build artifacts | ||
| build/ | ||
| ``` |
There was a problem hiding this comment.
The README recommends adding 'build_tools_workflows/' and 'build/' to .gitignore (lines 27-31), but the PR doesn't include these updates to the .gitignore file. The repository has a .gitignore file, but it doesn't contain these entries.
This PR should update .gitignore to include:
# Build tools (downloaded by wrapper scripts)
build_tools_workflows/
# Dependency build artifacts
build/
Without this change, developers might accidentally commit these directories to version control.
| cd "$NATIVE_COMPONENT_DIR" | ||
| "$BUILD_TOOLS_DIR/cov_docker_script/setup_dependencies.sh" "$SCRIPT_DIR/component_config.json" |
There was a problem hiding this comment.
The script changes directory to NATIVE_COMPONENT_DIR at line 62 before running setup_dependencies.sh, but setup_dependencies.sh expects the config file path as an argument (line 63). The path "$SCRIPT_DIR/component_config.json" is relative to the original SCRIPT_DIR, not NATIVE_COMPONENT_DIR. This should still work because SCRIPT_DIR is an absolute path (computed at line 10), but it's not immediately clear from reading the code.
Consider using an absolute path for component_config.json or adding a comment explaining that SCRIPT_DIR is absolute and therefore safe to use after changing directories.
| # Verify setup_dependencies.sh exists before running | ||
| if [[ ! -f "$BUILD_TOOLS_DIR/cov_docker_script/setup_dependencies.sh" ]]; then | ||
| err "setup_dependencies.sh not found in build_tools_workflows" | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
The verification at lines 53-57 is redundant with the earlier verification at lines 41-50. The script already verifies that setup_dependencies.sh exists as part of the REQUIRED_SCRIPTS check. This duplicate check adds unnecessary code and could cause maintenance issues if one check is updated but not the other.
| # Verify setup_dependencies.sh exists before running | |
| if [[ ! -f "$BUILD_TOOLS_DIR/cov_docker_script/setup_dependencies.sh" ]]; then | |
| err "setup_dependencies.sh not found in build_tools_workflows" | |
| exit 1 | |
| fi |
| fi | ||
|
|
||
| if [[ ! -f "$BUILD_TOOLS_DIR/cov_docker_script/build_native.sh" ]]; then | ||
| err "build_native.sh not found in build_tools_workflows. Please run run_setup_dependencies.sh first." |
There was a problem hiding this comment.
The error message states "Please run run_setup_dependencies.sh first" but this assumes that running the setup script will fix the issue. However, if build_tools_workflows exists but doesn't contain build_native.sh (e.g., due to a failed or incomplete previous run), running run_setup_dependencies.sh won't help because it skips cloning when the directory already exists (line 29-30 in run_setup_dependencies.sh).
Consider providing more specific guidance, such as suggesting to remove the build_tools_workflows directory and re-run setup, or automatically detecting and handling this scenario.
| err "build_native.sh not found in build_tools_workflows. Please run run_setup_dependencies.sh first." | |
| err "build_native.sh not found in build_tools_workflows. The build_tools_workflows directory may be incomplete or corrupted." | |
| err "Please remove the build_tools_workflows directory and then run run_setup_dependencies.sh again." |
| err "build_tools_workflows directory not found. Please run run_setup_dependencies.sh first." | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [[ ! -f "$BUILD_TOOLS_DIR/cov_docker_script/build_native.sh" ]]; then | ||
| err "build_native.sh not found in build_tools_workflows. Please run run_setup_dependencies.sh first." |
There was a problem hiding this comment.
The README states "Leaves build_tools_workflows in place for run_native_build.sh" (line 102) but this creates a temporal coupling between the two scripts. If someone runs only run_native_build.sh without first running run_setup_dependencies.sh, it will fail. While the script checks for this at lines 24-28, the design could be more robust.
Consider having run_native_build.sh handle the setup of build_tools_workflows itself if it's missing, or documenting more clearly that these scripts must be run in sequence and cannot be used independently.
| err "build_tools_workflows directory not found. Please run run_setup_dependencies.sh first." | |
| exit 1 | |
| fi | |
| if [[ ! -f "$BUILD_TOOLS_DIR/cov_docker_script/build_native.sh" ]]; then | |
| err "build_native.sh not found in build_tools_workflows. Please run run_setup_dependencies.sh first." | |
| err "build_tools_workflows directory not found. Attempting to run run_setup_dependencies.sh..." | |
| if [[ -x "$SCRIPT_DIR/run_setup_dependencies.sh" ]]; then | |
| "$SCRIPT_DIR/run_setup_dependencies.sh" | |
| else | |
| err "run_setup_dependencies.sh not found or not executable in $SCRIPT_DIR. Please run it manually before retrying." | |
| exit 1 | |
| fi | |
| fi | |
| # Re-verify build_tools_workflows after potential setup | |
| if [[ ! -d "$BUILD_TOOLS_DIR" ]]; then | |
| err "build_tools_workflows directory still not found after running run_setup_dependencies.sh." | |
| exit 1 | |
| fi | |
| if [[ ! -f "$BUILD_TOOLS_DIR/cov_docker_script/build_native.sh" ]]; then | |
| err "build_native.sh not found in build_tools_workflows. Attempting to run run_setup_dependencies.sh..." | |
| if [[ -x "$SCRIPT_DIR/run_setup_dependencies.sh" ]]; then | |
| "$SCRIPT_DIR/run_setup_dependencies.sh" | |
| else | |
| err "run_setup_dependencies.sh not found or not executable in $SCRIPT_DIR. Please run it manually before retrying." | |
| exit 1 | |
| fi | |
| fi | |
| # Re-verify build_native.sh after potential setup | |
| if [[ ! -f "$BUILD_TOOLS_DIR/cov_docker_script/build_native.sh" ]]; then | |
| err "build_native.sh still not found in build_tools_workflows after running run_setup_dependencies.sh." |
| REQUIRED_SCRIPTS=("build_native.sh" "common_build_utils.sh" "common_external_build.sh" "setup_dependencies.sh") | ||
|
|
||
| # Basic logging functions | ||
| log() { echo "[INFO] $*"; } | ||
| ok() { echo "[OK] $*"; } | ||
| err() { echo "[ERROR] $*" >&2; } | ||
|
|
||
| echo "" | ||
| echo "===== Setup Dependencies Pipeline =====" | ||
| echo "" | ||
|
|
||
| # Setup build tools | ||
| log "Setting up build tools..." | ||
|
|
||
| # Clone build_tools_workflows | ||
| if [[ -d "$BUILD_TOOLS_DIR" ]]; then | ||
| log "build_tools_workflows already exists, skipping clone" | ||
| else | ||
| log "Cloning build_tools_workflows (develop)" | ||
| cd "$NATIVE_COMPONENT_DIR" | ||
| git clone -b develop "$BUILD_TOOLS_REPO_URL" || { err "Clone failed"; exit 1; } | ||
| ok "Repository cloned" | ||
| fi | ||
|
|
||
| # Verify required scripts | ||
| [[ ! -d "$BUILD_TOOLS_DIR/cov_docker_script" ]] && { err "cov_docker_script not found"; exit 1; } | ||
|
|
||
| log "Verifying required scripts..." | ||
| MISSING=() | ||
| for script in "${REQUIRED_SCRIPTS[@]}"; do | ||
| [[ -f "$BUILD_TOOLS_DIR/cov_docker_script/$script" ]] || MISSING+=("$script") | ||
| done |
There was a problem hiding this comment.
The REQUIRED_SCRIPTS array lists four scripts that must be present, but only one of them (setup_dependencies.sh) is actually used by this wrapper script at line 63. The other three scripts (build_native.sh, common_build_utils.sh, common_external_build.sh) are not referenced by run_setup_dependencies.sh.
While build_native.sh is needed by run_native_build.sh, common_build_utils.sh and common_external_build.sh are implementation details that are sourced by the build scripts themselves. Checking for them here creates unnecessary coupling and could make the script fragile to changes in the build_tools_workflows implementation.
Consider verifying only the scripts that this wrapper directly invokes (setup_dependencies.sh), or remove the verification entirely since line 54-57 already checks for setup_dependencies.sh specifically.
Reason for change: Enable coverity scan using native build.
Test Procedure: All the checks should pass in github
Risks: Low
Priority: P1
Signed-off-by: sowmiya_chelliah@comcast.com