-
Notifications
You must be signed in to change notification settings - Fork 1
RDKB-63007 RDKB-63448: Update build_tools_workflows to integrate with submodule change #27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request modifies the build automation scripts to integrate submodule support for native builds. The changes affect how dependencies are cloned, how build scripts receive configuration paths, and how the component directory is determined.
Changes:
- Added Git submodule initialization after repository cloning
- Modified header file pattern matching to include additional header file extensions
- Refactored command execution to read configuration before changing directories
- Changed component directory calculation from parameter-based to path-derived approach
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| cov_docker_script/setup_dependencies.sh | Added config file path argument when invoking dependency build scripts |
| cov_docker_script/common_build_utils.sh | Added submodule update logic, expanded header pattern, and refactored command reading to happen before directory changes |
| cov_docker_script/build_native.sh | Changed COMPONENT_DIR from parameter-based to computed from SCRIPT_DIR location |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if [[ -d "$src" ]]; then | ||
| log "Copying headers: $src → $dst" | ||
| if ! find "$src" -maxdepth 1 -name "*.h" -exec cp {} "$dst/" \; 2>/dev/null; then | ||
| if ! find "$src" -maxdepth 1 -name "*.h*" -exec cp {} "$dst/" \; 2>/dev/null; then |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the pattern from ".h" to ".h*" is overly broad and will match unintended files such as .h.in, .h.orig, .h.bak, .html, etc. If the goal is to include C++ headers (.hpp, .hxx, .hh), use a more specific pattern like -name ".h" -o -name ".hpp" -o -name ".hxx" -o -name ".hh" instead.
| if ! find "$src" -maxdepth 1 -name "*.h*" -exec cp {} "$dst/" \; 2>/dev/null; then | |
| if ! find "$src" -maxdepth 1 \( -name "*.h" -o -name "*.hpp" -o -name "*.hxx" -o -name "*.hh" \) -exec cp {} "$dst/" \; 2>/dev/null; then |
|
|
||
| pushd "$repo_dir" >/dev/null || return 1 | ||
| if ! "$full_script"; then | ||
| if ! "$full_script" "./cov_docker_script/component_config.json"; then |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change hardcodes the config file path as "./cov_docker_script/component_config.json" when executing a dependency's build script. However, we're inside the dependency repository (after pushd $repo_dir), and this assumes the dependency has this exact file structure. The original code didn't pass any argument, which would cause build_native.sh to use its default "$SCRIPT_DIR/component_config.json". This change will likely fail for dependencies that don't have a component_config.json in this exact location. Consider either:
- Making the config file path configurable in the JSON configuration
- Passing an absolute path to a config file that exists
- Not passing the argument and letting the dependency's build script use its own default
| if ! "$full_script" "./cov_docker_script/component_config.json"; then | |
| if ! "$full_script" "$CONFIG_FILE"; then |
| fi | ||
| ok "$name cloned successfully" | ||
|
|
||
| git -C "$dest" submodule update --init --recursive --remote |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repository is cloned with --depth 1 (shallow clone) at line 53, but then submodule update is performed without --depth. For consistency and to avoid downloading more history than necessary, consider adding --depth 1 to the submodule update command as well: "git -C "$dest" submodule update --init --recursive --depth 1"
| git -C "$dest" submodule update --init --recursive --remote | |
| git -C "$dest" submodule update --init --recursive --remote --depth 1 |
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| CONFIG_FILE="${1:-$SCRIPT_DIR/component_config.json}" | ||
| COMPONENT_DIR="${2:-$(cd "$SCRIPT_DIR/.." && pwd)}" | ||
| COMPONENT_DIR="$(dirname "$(dirname "$SCRIPT_DIR")")" |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code now computes COMPONENT_DIR as two directories above SCRIPT_DIR instead of using the second parameter. However, line 60 in common_external_build.sh still passes a second parameter when calling this script. This change breaks the calling convention. Either:
- Restore the previous behavior: COMPONENT_DIR="${2:-$(cd "$SCRIPT_DIR/.." && pwd)}"
- Or update all callers to not pass the second parameter and update the usage documentation
Additionally, the new calculation assumes a specific directory structure (SCRIPT_DIR is two levels deep from component root), which may not always be correct if scripts are called from different locations or if the directory structure changes.
| COMPONENT_DIR="$(dirname "$(dirname "$SCRIPT_DIR")")" | |
| COMPONENT_DIR="${2:-$(cd "$SCRIPT_DIR/.." && pwd)}" |
| fi | ||
| ok "$name cloned successfully" | ||
|
|
||
| git -C "$dest" submodule update --init --recursive --remote |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The --remote flag in submodule update makes submodules track the remote branch instead of the specific commit recorded in the parent repository. This can lead to non-reproducible builds because different commits may be checked out on different runs. For build reproducibility, remove the --remote flag to ensure submodules use the specific commits recorded in the parent repository.
| git -C "$dest" submodule update --init --recursive --remote | |
| git -C "$dest" submodule update --init --recursive |
| fi | ||
| ok "$name cloned successfully" | ||
|
|
||
| git -C "$dest" submodule update --init --recursive --remote |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The git submodule update command does not check for errors. If submodule initialization fails, the build will continue without the required submodule code, leading to build failures that are hard to diagnose. Add error checking to ensure submodule update succeeds before proceeding.
| git -C "$dest" submodule update --init --recursive --remote | |
| if ! git -C "$dest" submodule update --init --recursive --remote; then | |
| err "Failed to update git submodules for $name" | |
| return 1 | |
| fi |
Reason for change: Scripts needs to be modified to integrate submodule change for Native Build
Test Procedure: Native build workflow should pass for all components
Risks: Low
Priority: P1
Signed-off-by: Hirrangandhi_Devaraj@comcast.com