fix: fix multi-machine collision and add sstate support for eSDK - #308
fix: fix multi-machine collision and add sstate support for eSDK#308Teng Fan (TengFan-QC) wants to merge 5 commits into
Conversation
2610fa1 to
ce9f622
Compare
Qualcomm AI ReviewClick to expand Code ReviewReviewed Commits: 2610fa1
Signed-off-by: Teng Fan tengf@qti.qualcomm.com Pull Request OverviewThis PR introduces significant improvements to the QIRP SDK build system, focusing on sstate integration for packagegroup dependency collection and machine-specific artifact organization. Files Changed Summary
Key Changes
Critical Issues Identified
The PR successfully addresses sstate caching for SDK generation but requires fixes to variable references and task dependencies before merge. [FUNCTIONALITY] High Severity - Incorrect variable reference in TOOLCHAIN_HOST_TASK appendIn The issue is that line 41 should be removed or modified, as having both lines causes ROS_SDK_HOST_PACKAGES to be added twice for non-buildtools-tarball recipes. This is a logic error that will result in duplicate packages in the SDK toolchain. Fixed Code Snippet: # Remove line 41 entirely and keep only the conditional append:
TOOLCHAIN_HOST_TASK:append = "${@(' ' + (d.getVar('ROS_SDK_HOST_PACKAGES') or '')) if d.getVar('PN') != 'buildtools-tarball' else ''}"[FUNCTIONALITY] High Severity - Missing file-checksums for content_config.json in rdepends-collectorThe Looking at the code flow, Actually, upon closer inspection, this is not an issue with rdepends-collector.bbclass itself. The concern is valid for psdk-image.bbclass which already has the file-checksums declared at line 266. No fix needed for rdepends-collector.bbclass. However, there IS an issue: the Fixed Code Snippet: # Add a variable for the mandatory SDK package and include it in vardeps
ROBOTICS_SDK_PACKAGE ?= "qirp-sdk"
do_collect_rdepends[vardeps] = "RDEPENDS:${PN} ROBOTICS_SDK_PACKAGE"
# In the Python function at line 86:
sdk_package = d.getVar("ROBOTICS_SDK_PACKAGE") or "qirp-sdk"
f.write("{}\n".format(sdk_package))[MAINTAINABILITY] Medium Severity - Inconsistent variable naming between classesThe variable In Yocto/BitBake best practices, shared configuration variables should be defined in a common location (typically The variable should be moved to Fixed Code Snippet: # In conf/layer.conf, add after line 25:
ROBOTICS_PACKAGEGROUP_LIST_DIR = "${DEPLOY_DIR}/packagegroup-lists/${MACHINE}"
# In classes/rdepends-collector.bbclass, remove line 23 (the variable definition)
# In classes/psdk-image.bbclass, remove line 21 (the variable definition) |
![qualcomm-ai-code-review-assistant[bot]](https://avatars.githubusercontent.com/in/4128059?s=60&v=4){kind=small}
Qualcomm AI ReviewClick to expand Code ReviewReviewed commit: ce9f622 "fix: fix multi-machine collision and eSDK sstate support
Signed-off-by: Teng Fan tengf@qti.qualcomm.com" Pull Request OverviewThis PR introduces significant improvements to the QIRP SDK build system, focusing on sstate integration for packagegroup dependency collection and machine-specific artifact organization. Files Changed Summary
Key Changes
Critical Issues Identified
Architecture ComplianceThe changes properly align with the layer's architecture:
[FUNCTIONALITY] High Severity - Incorrect variable reference in SDK host task configurationIn This is a critical logic error that will cause:
The condition Fixed Code Snippet: TOOLCHAIN_TARGET_TASK:append = "${@(' ' + (d.getVar('ROS_SDK_TARGET_PACKAGES') or '')) if d.getVar('PN') != 'buildtools-tarball' else ''}" |
Test run workflowTest jobs for commit ce9f622
|
ce9f622 to
a049219
Compare
Qualcomm AI ReviewClick to expand Code ReviewReviewed commit: a049219 "'fix: fix multi-machine collision and eSDK sstate support
Signed-off-by: Teng Fan tengf@qti.qualcomm.com" Pull Request OverviewThis PR makes several improvements to the meta-qcom-robotics-sdk layer:
Files Changed Summary
Critical Issues
[FUNCTIONALITY] Missing sstate task dependency - High SeverityIn According to the Yocto Project Reference Manual, sstate tasks that depend on package metadata should explicitly depend on Fixed Code Snippet: # Add task: collect RDEPENDS after packagegroup build
addtask do_collect_rdepends after do_packagedata before do_build
ROBOTICS_SDK_PACKAGE ?= "qirp-sdk"
do_collect_rdepends[vardeps] = "RDEPENDS:${PN} ROBOTICS_SDK_PACKAGE"[FUNCTIONALITY] Buildtools-tarball check may fail in anonymous Python - Medium SeverityIn According to Yocto best practices, recipe-specific overrides should be handled in the recipe itself or via bbappend files, not in distro configuration includes. A more robust approach would be to use a bbappend for buildtools-tarball or to check for the recipe context more defensively. Fixed Code Snippet: # Avoid buildtools-tarball recipe packages the host *.armv8_2a.rpm for do_populate_sdk task
# Only add ROS SDK packages if PN is defined and not buildtools-tarball
TOOLCHAIN_TARGET_TASK:append = "${@(' ' + (d.getVar('ROS_SDK_TARGET_PACKAGES') or '')) if (d.getVar('PN') and d.getVar('PN') != 'buildtools-tarball') else ''}"
TOOLCHAIN_HOST_TASK:append = "${@(' ' + (d.getVar('ROS_SDK_HOST_PACKAGES') or '')) if (d.getVar('PN') and d.getVar('PN') != 'buildtools-tarball') else ''}"Alternatively, consider creating a # buildtools-tarball.bbappend
TOOLCHAIN_TARGET_TASK:remove = "${ROS_SDK_TARGET_PACKAGES}"
TOOLCHAIN_HOST_TASK:remove = "${ROS_SDK_HOST_PACKAGES}" |
Test run workflowTest jobs for commit a049219
|
a049219 to
6aa9881
Compare
Qualcomm AI ReviewClick to expand Code ReviewReviewed commit: 6aa9881 "'fix: fix multi-machine collision and eSDK sstate support
Signed-off-by: Teng Fan tengf@qti.qualcomm.com" Pull Request OverviewThis PR introduces significant improvements to the QIRP SDK build system for the meta-qcom-robotics-sdk layer, focusing on sstate integration for packagegroup dependency collection and SDK generation. The changes enable proper caching and restoration of build artifacts in eSDK environments. Files Changed Summary
Key Changes
Critical Issues Identified
The PR demonstrates good understanding of Yocto sstate mechanisms and follows the layer's architecture patterns. The TCL/expect fixes address real build issues in native toolchain assembly. [Error Handling] Missing exception handling for file write operations - Medium SeverityIn While Python's Fixed Code Snippet: python do_collect_rdepends() {
"""
Collect packagegroup RDEPENDS and write to file
Only collects direct RDEPENDS, not dependencies of dependencies
"""
import os
pn = d.getVar("PN")
bb.note("Collecting RDEPENDS for packagegroup: {}".format(pn))
# Get RDEPENDS
rdepends_var = 'RDEPENDS:{}'.format(pn)
rdepends = d.getVar(rdepends_var) or ""
if not rdepends:
bb.warn("RDEPENDS for {} is empty (will be added later)".format(pn))
rdepends = ""
# Write into the private staging dir so sstate can package it correctly.
# sstate copies staging -> ROBOTICS_PACKAGEGROUP_LIST_DIR on restore.
list_dir = d.getVar("ROBOTICS_PACKAGEGROUP_LIST_STAGING_DIR")
os.makedirs(list_dir, exist_ok=True)
list_file = os.path.join(list_dir, "{}.list".format(pn))
try:
with open(list_file, 'w') as f:
for pkg in rdepends.split():
pkg_clean = pkg.strip()
if pkg_clean:
f.write("{}\n".format(pkg_clean))
# qirp-sdk is a standalone package that must be present in all images.
sdk_package = d.getVar("ROBOTICS_SDK_PACKAGE") or "qirp-sdk"
f.write("{}\n".format(sdk_package))
except IOError as e:
bb.fatal("Failed to write packagegroup list file {}: {}".format(list_file, str(e)))
package_count = len(rdepends.split())
bb.note("Wrote {} packages to {}".format(package_count, list_file))
}[Functionality] Potential race condition in sstate cleanup - Medium SeverityIn The issue is that if multiple packagegroups are being cleaned concurrently (which can happen in parallel builds), and they share the same Additionally, the task runs before For better safety in a Yocto context, the cleanup should be more defensive: Fixed Code Snippet: do_clean_rdepends() {
# Only remove if the file exists and we're in a clean context
# The sstate manifest handles removal for sstate-restored files
if [ -f "${ROBOTICS_PACKAGEGROUP_LIST_DIR}/${PN}.list" ]; then
bbnote "Removing packagegroup list file for ${PN}"
rm -f "${ROBOTICS_PACKAGEGROUP_LIST_DIR}/${PN}.list"
fi
}Alternatively, since |
Test run workflowTest jobs for commit 6aa9881
|
6aa9881 to
ae304ae
Compare
Qualcomm AI ReviewClick to expand Code ReviewReviewed commit: ae304ae "fix: restore eSDK do_collect_rdepends via SDK_RECRDEP_TASKS Removed "*:do_collect_rdepends" from BB_SETSCENE_ENFORCE_IGNORE_TASKS, Add do_collect_rdepends to SDK_RECRDEP_TASKS so its signature and sstate Signed-off-by: Teng Fan tengf@qti.qualcomm.com" Pull Request OverviewThis PR modifies the Yocto SDK configuration for the Qualcomm Robotics SDK by changing the approach to handling the Files Changed Summary
Key Changes
Critical Issues Identified
[FUNCTIONALITY] Potential eSDK build failure due to task dependency change - High SeverityThe PR changes the approach from ignoring the The new approach using The original Recommendation: Either revert to the original Fixed Code Snippet: # The inner eSDK build runs with BB_SETSCENE_ENFORCE=1 and only whitelists SDK
# target tasks (%:*). do_collect_rdepends runs on packagegroup recipes, which are
# NOT SDK targets, and its sstate signature is not written into the SDK's
# locked-sigs.inc — so it cannot be setscene-restored inside the eSDK. Allow it
# to execute directly there; the task is cheap and its .list output is still
# published to ROBOTICS_PACKAGEGROUP_LIST_DIR via sstate-outputdirs.
BB_SETSCENE_ENFORCE_IGNORE_TASKS:append = " *:do_collect_rdepends"
# Add do_collect_rdepends task to target dependency chain to ensure proper task ordering
SDK_RECRDEP_TASKS:append = " do_collect_rdepends"[MAINTAINABILITY] Comment-code mismatch after implementation change - Medium SeverityThe multi-line comment spanning lines 43-48 describes the rationale for using This creates a documentation-code mismatch where the comment describes a solution that is no longer active in the code. The new comment on line 51 ("Add do_collect_rdepends task to target dependency chain to avoid eSDK inner build error") doesn't explain WHY this approach solves the problem described in the earlier comment. Per Yocto best practices and this layer's maintainability standards, comments should accurately reflect the implementation and provide clear rationale for configuration choices, especially for non-obvious workarounds. Recommendation: Update the comment block to accurately describe the current approach and explain how Fixed Code Snippet: # The inner eSDK build runs with BB_SETSCENE_ENFORCE=1 and only whitelists SDK
# target tasks (%:*). do_collect_rdepends runs on packagegroup recipes, which are
# NOT SDK targets, and its sstate signature is not written into the SDK's
# locked-sigs.inc. We handle this by:
# 1. Ignoring it during setscene enforcement so it can execute directly
# 2. Adding it to SDK recursive dependency tasks for proper task ordering
BB_SETSCENE_ENFORCE_IGNORE_TASKS:append = " *:do_collect_rdepends"
SDK_RECRDEP_TASKS:append = " do_collect_rdepends" |
Test run workflowTest jobs for commit ae304ae
|
ae304ae to
1c05eb3
Compare
Qualcomm AI ReviewClick to expand Code ReviewReviewed commit: 1c05eb3 "fix: replace MACHINE_ARCH with MACHINE for separate sstate Align with standard SDK behavior by using MACHINE_ARCH Signed-off-by: Teng Fan tengf@qti.qualcomm.com" Pull Request OverviewThis PR fixes an eSDK (extensible SDK) inner build failure related to the Files Changed Summary
Key Changes
Critical IssuesNo critical issues identified. The changes are well-structured and follow Yocto best practices:
The fix addresses the root cause (Path 1: missing task signature in locked-sigs.inc) rather than just working around the symptom, which is the correct approach. [YOCTO-SPECIFIC] Verify SDK_RECRDEP_TASKS effectiveness - Medium SeverityThe fix adds
The commented-out
If verification fails, the fallback approach should be uncommented. Verification Command: # After building the eSDK, check:
grep 'packagegroup.*:do_collect_rdepends' tmp/deploy/sdk/*-toolchain-ext-*.sh.manifest
# Or inspect the unpacked SDK's locked-sigs.inc |
| # locked-sigs.inc — so it cannot be setscene-restored inside the eSDK. Allow it | ||
| # to execute directly there; the task is cheap and its .list output is still | ||
| # published to ROBOTICS_PACKAGEGROUP_LIST_DIR via sstate-outputdirs. | ||
| # BB_SETSCENE_ENFORCE_IGNORE_TASKS:append = " *:do_collect_rdepends" | ||
|
|
||
| # Add do_collect_rdepends task to target dependency chain to avoid eSDK inner build error | ||
| SDK_RECRDEP_TASKS:append = " do_collect_rdepends" |
There was a problem hiding this comment.
[YOCTO-SPECIFIC] Verify SDK_RECRDEP_TASKS effectiveness - Medium Severity
The fix adds do_collect_rdepends to SDK_RECRDEP_TASKS to ensure the task is included in the eSDK's locked-sigs.inc. However, the documentation file itself (esdk_inner_build_issue.md, lines 185-190) includes a critical verification checklist that must be completed:
- Run a full eSDK build with the change
- Grep the newly generated inner
locked-sigs.incforpackagegroup-*:do_collect_rdepends— it MUST be present - If the recrdep traversal does not reach it, fall back to Option B (restore the whitelist)
The commented-out BB_SETSCENE_ENFORCE_IGNORE_TASKS line (line 49 in qcom-robotics-sdk.inc) serves as a fallback if SDK_RECRDEP_TASKS proves insufficient. Before merging, the PR author should confirm that:
- The task signature appears in the generated
locked-sigs.inc - The eSDK build completes successfully
- The
.listfiles are properly available in the eSDK environment
If verification fails, the fallback approach should be uncommented.
Verification Command:
# After building the eSDK, check:
grep 'packagegroup.*:do_collect_rdepends' tmp/deploy/sdk/*-toolchain-ext-*.sh.manifest
# Or inspect the unpacked SDK's locked-sigs.inc
1c05eb3 to
7ec6f32
Compare
Qualcomm AI ReviewClick to expand Code ReviewReviewed commit: 7ec6f32 "fix: replace MACHINE_ARCH with MACHINE for separate sstate Align with standard SDK behavior by using MACHINE_ARCH Signed-off-by: Teng Fan tengf@qti.qualcomm.com" Pull Request OverviewThis PR refactors the Summary Table
Key Changes
Critical Issues
[MAINTAINABILITY] Commented-out code should be removed (Medium Severity)In Issue: Leaving commented-out code in production creates maintenance debt and confusion. Future maintainers may wonder whether this line should be uncommented under certain conditions, or whether it represents an incomplete migration. The comment block above already documents the problem context, so the old solution line serves no purpose. Recommendation: Remove line 49 entirely. The comment block (lines 43-48) provides sufficient context about the eSDK build constraints, and the new solution using Fixed Code Snippet: # The inner eSDK build runs with BB_SETSCENE_ENFORCE=1 and only whitelists SDK
# target tasks (%:*). do_collect_rdepends runs on packagegroup recipes, which are
# NOT SDK targets, and its sstate signature is not written into the SDK's
# locked-sigs.inc — so it cannot be setscene-restored inside the eSDK. Allow it
# to execute directly there; the task is cheap and its .list output is still
# published to ROBOTICS_PACKAGEGROUP_LIST_DIR via sstate-outputdirs.
# Add do_collect_rdepends task to target dependency chain to avoid eSDK inner build error
SDK_RECRDEP_TASKS:append = " do_collect_rdepends" |
| # locked-sigs.inc — so it cannot be setscene-restored inside the eSDK. Allow it | ||
| # to execute directly there; the task is cheap and its .list output is still | ||
| # published to ROBOTICS_PACKAGEGROUP_LIST_DIR via sstate-outputdirs. | ||
| # BB_SETSCENE_ENFORCE_IGNORE_TASKS:append = " *:do_collect_rdepends" | ||
|
|
||
| # Add do_collect_rdepends task to target dependency chain to avoid eSDK inner build error | ||
| SDK_RECRDEP_TASKS:append = " do_collect_rdepends" |
There was a problem hiding this comment.
[MAINTAINABILITY] Commented-out code should be removed (Medium Severity)
In conf/distro/include/qcom-robotics-sdk.inc, the previous approach using BB_SETSCENE_ENFORCE_IGNORE_TASKS is commented out rather than removed. While the extensive comment block (lines 43-48) explaining the eSDK build context is valuable documentation, the commented-out configuration line (line 49) should be removed entirely.
Issue: Leaving commented-out code in production creates maintenance debt and confusion. Future maintainers may wonder whether this line should be uncommented under certain conditions, or whether it represents an incomplete migration. The comment block above already documents the problem context, so the old solution line serves no purpose.
Recommendation: Remove line 49 entirely. The comment block (lines 43-48) provides sufficient context about the eSDK build constraints, and the new solution using SDK_RECRDEP_TASKS (lines 51-52) is the active approach.
Fixed Code Snippet
# The inner eSDK build runs with BB_SETSCENE_ENFORCE=1 and only whitelists SDK
# target tasks (%:*). do_collect_rdepends runs on packagegroup recipes, which are
# NOT SDK targets, and its sstate signature is not written into the SDK's
# locked-sigs.inc — so it cannot be setscene-restored inside the eSDK. Allow it
# to execute directly there; the task is cheap and its .list output is still
# published to ROBOTICS_PACKAGEGROUP_LIST_DIR via sstate-outputdirs.
# Add do_collect_rdepends task to target dependency chain to avoid eSDK inner build error
SDK_RECRDEP_TASKS:append = " do_collect_rdepends"
|
Can you pls confirm this PR still need? |
- Scope packagegroup list output under to prevent file conflicts when building multiple machine configs in one workspace - Rename PACKAGEGROUP_LIST_STAGING_DIR to ROBOTICS_PACKAGEGROUP_LIST_STAGING_DIR to avoid collision with Yocto's standard variable - Register do_collect_rdepends as a proper sstate task (SSTATETASKS) so the inner eSDK build can restore it from sstate instead of failing with 'setscene ignore_tasks' - Add do_collect_rdepends[vardeps], [dirs], [cleandirs], [sstate-outputdirs] and [stamp-extra-info] for correct sstate/stamp behaviour Signed-off-by: Teng Fan <tengf@qti.qualcomm.com>
Align with standard SDK behavior by using MACHINE_ARCH to differentiate PACKAGEGROUP_LIST_DIR instead of MACHINE. Signed-off-by: Teng Fan <tengf@qti.qualcomm.com>
6658e1a to
e94c497
Compare
Qualcomm AI ReviewClick to expand Code ReviewReviewed commit: e94c497 "fix: add tcl8 sysroot process to fix esdk build error
Signed-off-by: Teng Fan tengf@qti.qualcomm.com" Pull Request OverviewThis PR makes minor corrections to configuration files in the meta-qcom-robotics-sdk layer:
Files Changed Summary
Critical IssuesNo critical issues identified. The changes are primarily cosmetic (whitespace cleanup) and a URL correction. [Best Practices] Missing FILESEXTRAPATHS in bbappend files - Low SeverityBoth bbappend files modify recipes but do not include According to Yocto documentation and this layer's conventions, bbappends should typically start with: This ensures that if you later add patches, configuration files, or other resources in a subdirectory matching the recipe name, bitbake will find them correctly. Impact: Currently no functional impact since no files are being added. However, this is a maintainability consideration for future modifications. Recommendation: Consider adding FILESEXTRAPATHS declarations to follow Yocto best practices, especially since these bbappends are making non-trivial modifications (EXTRA_OECONF changes and sysroot manipulation). |
- expect recipe depedency(tcl8) conflict with python3->tcl9 dependency(tcl9). - Add tcl8 sysroot preprocess function to resolve conflict. Signed-off-by: Teng Fan <tengf@qti.qualcomm.com>
e94c497 to
c0e4ffd
Compare
Qualcomm AI ReviewClick to expand Code ReviewReviewed commit: c0e4ffd "fix: add tcl8 sysroot process to fix esdk build error
Signed-off-by: Teng Fan tengf@qti.qualcomm.com" Pull Request OverviewThis PR modifies two Yocto bbappend files related to Tcl 8 and expect recipe configuration:
Summary of Changes
Key Findings
Architecture ComplianceBoth files are properly located in |
|
|
||
| if [ -f ${SYSROOT_DESTDIR}${libdir}/tclConfig.sh ]; then | ||
| cp -a ${SYSROOT_DESTDIR}${libdir}/tclConfig.sh \ | ||
| ${SYSROOT_DESTDIR}${libdir}/tcl8.6/tclConfig.sh |
There was a problem hiding this comment.
tcl8_${PV}
| @@ -0,0 +1 @@ | |||
| EXTRA_OECONF:append = " --with-tcl=${STAGING_LIBDIR}/tcl8.6" | |||
There was a problem hiding this comment.
tcl8_$PV
Signed-off-by: Teng Fan <33081913+TengFan-QC@users.noreply.github.com>
Signed-off-by: Teng Fan <33081913+TengFan-QC@users.noreply.github.com>
9d9f328 to
e4170a6
Compare
2 participants
CRs-Fixed: 4598715
Summary
Two bugs in rdepends-collector.bbclass are fixed.
Bug 1 — Multi-machine file collision
Symptom
Root cause
PACKAGEGROUP_LIST_DIR = "${DEPLOY_DIR}/packagegroup-lists"has no machine-level separation. Building two machine configs (e.g. iq_8275_evk and iq_9075_evk) in the same workspace causes both to write the same file path, triggering a shared-area conflict.Fix
Bug 2 — eSDK do_populate_sdk_ext setscene failure
Symptom
Root cause
do_populate_sdk_extruns an inner BitBake build under strict setscene enforcement. The %:* pattern inBB_SETSCENE_ENFORCE_IGNORE_TASKS#PR276 expands only to explicit build targets; tasks on dependency recipes (e.g. packagegroup-robotics-opensource) must either be in sstate or be explicitly exempted. do_collect_rdepends was neither — it had no sstate registration and wrote directly to DEPLOY_DIR, which sstate does not track.The previous workaround appended *:do_collect_rdepends to BB_SETSCENE_ENFORCE_IGNORE_TASKS, forcing the task to always re-run from scratch. This masked the root cause.
Fix
Properly register do_collect_rdepends as a sstate-restorable task:
Test case passed:
Robotics build includes image/SDK/eSDK generated and works normally:
Build robotics image via eSDK:
QRB ROS samples and cross-compile works well on QIR-SDK:
Robotics image is flashable and works well:
