Skip to content

fix(build): auto-resolve snapshot dependency closure (#638) - #655

Merged
Ulrond merged 4 commits into
developfrom
feature/638-snapshot-dep-resolution
Jul 5, 2026
Merged

fix(build): auto-resolve snapshot dependency closure (#638)#655
Ulrond merged 4 commits into
developfrom
feature/638-snapshot-dep-resolution

Conversation

@Ulrond

@Ulrond Ulrond commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Closes #638.

Problem

./build_modules.sh videosink --version 0.2.0.0 fails on a fresh checkout with fatal error: com/rdk/hal/PropertyValue.h: No such file or directory. The single-module snapshot path assumed its dependency headers were already staged in out/build/include and dependency libraries already installed in out/target/lib/halif — but only the manifest path arranged that. On a clean tree the compile fails (missing header), and even with headers the link would fail (missing lib<dep>-v<ver>-cpp.so). It "worked" only for anyone who had already run a full/manifest build.

Fix

Resolve the dependency closure before building the requested snapshot:

  • snapshot_deps() reads the (comp, ver) pairs the snapshot already declares via its ${HALIF_INCLUDE_DIR}/<comp>/<ver>/include references in CMakeLists.txt — no new metadata.
  • Each dependency is built through a recursive build_modules.sh invocation, so transitive deps resolve and an already-built dependency is skipped (the common diamond builds once).
  • stage_snapshot_headers() copies each snapshot's committed include/ tree into out/build/include after a successful build, so dependents resolve it.

Test

From a wiped closure (removed staged headers + libs + build dirs for videosink/videodecoder/avclock/common 0.2.0.0):

$ ./build_modules.sh videosink --version 0.2.0.0
📸 Snapshot build: videosink/0.2.0.0
   ↳ building dependency avclock/0.2.0.0 ...
   ↳ building dependency common/0.2.0.0 ...   (leaf — compiles PropertyValue.cpp)
   ✓ dependency common/0.2.0.0 already built   (diamond reused)
   ↳ building dependency videodecoder/0.2.0.0 ...

Result: all four lib*-v0.2.0.0-cpp.so produced and all four header trees staged (incl. common/.../PropertyValue.h). bash -n clean.

Copilot AI review requested due to automatic review settings June 29, 2026 17:18
@github-project-automation github-project-automation Bot moved this to Architecture Review Required in halif_aidl Jun 29, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes fresh-checkout snapshot builds (non-current --version) by ensuring the dependency closure is built first and snapshot headers are staged so downstream snapshot builds can find required headers/libs (e.g., com/rdk/hal/PropertyValue.h from common).

Changes:

  • Added snapshot_deps() to infer snapshot dependencies by parsing ${HALIF_INCLUDE_DIR}/<comp>/<ver>/include references in snapshot CMakeLists.txt.
  • Added recursive dependency builds for snapshots prior to configuring/building the requested snapshot.
  • Added stage_snapshot_headers() to copy each snapshot’s committed include/ tree into out/build/include after a successful snapshot build.

Comment thread build_modules.sh
@Ulrond
Ulrond requested a review from a team July 3, 2026 10:50
@Ulrond Ulrond self-assigned this Jul 3, 2026
@Ulrond Ulrond added the bug Something isn't working label Jul 3, 2026
@Ulrond Ulrond moved this from Architecture Review Required to Review Requested in halif_aidl Jul 3, 2026
@Ulrond Ulrond added this to the Next Release milestone Jul 3, 2026
@Ulrond Ulrond moved this from Review Requested to Under Review in halif_aidl Jul 4, 2026
@Ulrond

Ulrond commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the review note on the smoke_test table: defined the <c> (component) / <v> (version) placeholders above the table, and reframed the dependency-closure row as a variant precondition ("the same build, but with <c>'s dependency snapshots deleted first") so it no longer reads as a duplicate of the row above.

Ulrond added a commit that referenced this pull request Jul 4, 2026
…eview)

- Define the <c> (component) and <v> (version) placeholders above the table.
- Reframe the dependency-closure row as a variant precondition ('the same
  build, but with <c>'s dependency snapshots deleted first') so it no longer
  reads as a duplicate of the row above.
- Add the 'builds' verb and fix 'dependencies'' -> 'each dependency's'.
Copilot AI review requested due to automatic review settings July 4, 2026 07:49
@Ulrond Ulrond added scope:infrastructure Repo tooling, CI/CD, scripts, governance docs Minor Change Additive, backwards-compatible interface change — bumps minor; the default for real work labels Jul 4, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comment thread build_modules.sh Outdated
Comment thread build_modules.sh Outdated
Ulrond added a commit that referenced this pull request Jul 4, 2026
Copilot review:
- snapshot_deps: guard the grep so a snapshot with no HAL deps (grep exit 1)
  doesn't trip set -o pipefail — an empty dep list is normal, not an error.
- stage_snapshot_headers: fail (return 1) on mkdir/cp error instead of
  silently succeeding and leaving dependents to fail later on missing headers.
- caller: check stage_snapshot_headers' status and exit non-zero on failure.
Ulrond added 4 commits July 5, 2026 09:32
A standalone snapshot build (e.g. ./build_modules.sh videosink --version
0.2.0.0) failed on a fresh checkout with 'com/rdk/hal/PropertyValue.h: No
such file or directory': the single-snapshot path assumed dependency headers
were already staged in out/build/include and dependency libraries already
installed, which only the 'manifest' path arranged. On a clean tree neither
exists, so the compile (and then the link) failed.

Resolve the dependency closure before building the target:

- snapshot_deps() reads the (comp, ver) pairs the snapshot already declares
  via its ${HALIF_INCLUDE_DIR}/<comp>/<ver>/include references in
  CMakeLists.txt — no new metadata.
- Each dependency is built via a recursive build_modules.sh invocation, so
  transitive deps resolve and an already-built dependency is skipped (the
  common-diamond builds once).
- stage_snapshot_headers() copies each snapshot's committed include/ tree
  into out/build/include after a successful build, so dependents resolve it.

Verified: from a wiped closure, 'build_modules.sh videosink --version 0.2.0.0'
builds common -> avclock/videodecoder -> videosink, staging all headers and
producing all four lib*-v0.2.0.0-cpp.so.
Steps 1-4 leave out/build/include + out/target fully populated, so the
per-version snapshot step never proves a standalone build can stand up its
own dependencies — exactly the gap that hid #638. Add step 5: pick the first
released snapshot that declares HAL dependencies, wipe that snapshot and its
dependencies' staged headers + libraries + build dirs, then build ONLY that
snapshot and assert every closure library is rebuilt and each dependency's
headers are re-staged. Documents all build paths in tests/README.md.

Verified: full suite 6/6 pass; step 5 wipes audiodecoder/0.1.0.0's common
0.1.0.0 dep and confirms it is auto-resolved.
…eview)

- Define the <c> (component) and <v> (version) placeholders above the table.
- Reframe the dependency-closure row as a variant precondition ('the same
  build, but with <c>'s dependency snapshots deleted first') so it no longer
  reads as a duplicate of the row above.
- Add the 'builds' verb and fix 'dependencies'' -> 'each dependency's'.
Copilot review:
- snapshot_deps: guard the grep so a snapshot with no HAL deps (grep exit 1)
  doesn't trip set -o pipefail — an empty dep list is normal, not an error.
- stage_snapshot_headers: fail (return 1) on mkdir/cp error instead of
  silently succeeding and leaving dependents to fail later on missing headers.
- caller: check stage_snapshot_headers' status and exit non-zero on failure.
Copilot AI review requested due to automatic review settings July 5, 2026 08:32
@Ulrond
Ulrond force-pushed the feature/638-snapshot-dep-resolution branch from 0bb7a5b to 0ed48e8 Compare July 5, 2026 08:32
@Ulrond
Ulrond merged commit 304bc8e into develop Jul 5, 2026
1 check passed
@Ulrond
Ulrond deleted the feature/638-snapshot-dep-resolution branch July 5, 2026 08:33
@github-project-automation github-project-automation Bot moved this from Under Review to Resolved in halif_aidl Jul 5, 2026
Copilot stopped reviewing on behalf of Ulrond due to an error July 5, 2026 08:33
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 5, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something isn't working Minor Change Additive, backwards-compatible interface change — bumps minor; the default for real work scope:infrastructure Repo tooling, CI/CD, scripts, governance docs

Projects

Status: Resolved

Development

Successfully merging this pull request may close these issues.

Build failure in VideoSink (v0.2.0.0) – Missing PropertyValue.h dependency

2 participants