RDKECOREMW-871: Enhance/create generic tool for RDKE release documentation - #64
RDKECOREMW-871: Enhance/create generic tool for RDKE release documentation#64arun-madhavan-013 wants to merge 17 commits into
Conversation
…hub.com/rdkcentral/meta-rdk-auxiliary into feature/rdke-specific-release-note-util
…stead of buildhistory" This reverts commit e52989e.
…logic instead of buildhistory"" This reverts commit ae95dc0.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a comprehensive BitBake class for automatically generating RDKE (RDK Extended) component documentation during builds. The tool collects package metadata from recipes during the build process, stores it in a shared cache, and generates JSON and Markdown documentation files at build completion.
Key Changes:
- Implements a recipe-level data collection task (
do_collect_component_data) that extracts package version information, source URIs, and Git commit references - Uses BitBake's SimpleCache to aggregate component data across multiple recipes during parallel builds
- Generates layer-specific JSON and Markdown files with hyperlinked version information for Git repositories, artifacts, and layer-hosted packages
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not (len(srcrev_value) == 40 and srcrev_value.isalnum()): | ||
| # SRCREV doesn't look like a commit hash, try to extract from version | ||
| if "+git" in version and "_" in version: | ||
| parts = version.split("_") | ||
| for part in reversed(parts): | ||
| if len(part) >= 8 and part.isalnum(): | ||
| actual_commit = part | ||
| rdke_log(f"Extracted commit from version: {actual_commit}", "DEBUG") | ||
| break | ||
|
|
||
| # Determine link type based on commit value | ||
| if len(actual_commit) >= 8 and actual_commit.isalnum(): |
There was a problem hiding this comment.
The check srcrev_value.isalnum() will return False for valid lowercase hexadecimal SHA-1 hashes because isalnum() only checks if all characters are alphanumeric but doesn't validate if they're valid hex characters. A SHA-1 hash should be validated with characters in the range [0-9a-fA-F]. Consider using a more robust check: all(c in '0123456789abcdefABCDEF' for c in srcrev_value) or a regex pattern.
| if not (len(srcrev_value) == 40 and srcrev_value.isalnum()): | |
| # SRCREV doesn't look like a commit hash, try to extract from version | |
| if "+git" in version and "_" in version: | |
| parts = version.split("_") | |
| for part in reversed(parts): | |
| if len(part) >= 8 and part.isalnum(): | |
| actual_commit = part | |
| rdke_log(f"Extracted commit from version: {actual_commit}", "DEBUG") | |
| break | |
| # Determine link type based on commit value | |
| if len(actual_commit) >= 8 and actual_commit.isalnum(): | |
| if not (len(srcrev_value) == 40 and all(c in '0123456789abcdefABCDEF' for c in srcrev_value)): | |
| # SRCREV doesn't look like a commit hash, try to extract from version | |
| if "+git" in version and "_" in version: | |
| parts = version.split("_") | |
| for part in reversed(parts): | |
| if len(part) >= 8 and all(c in '0123456789abcdefABCDEF' for c in part): | |
| actual_commit = part | |
| rdke_log(f"Extracted commit from version: {actual_commit}", "DEBUG") | |
| break | |
| # Determine link type based on commit value | |
| if len(actual_commit) >= 8 and all(c in '0123456789abcdefABCDEF' for c in actual_commit): |
| if len(part) >= 8 and part.isalnum(): | ||
| actual_commit = part | ||
| rdke_log(f"Extracted commit from version: {actual_commit}", "DEBUG") | ||
| break | ||
|
|
||
| # Determine link type based on commit value | ||
| if len(actual_commit) >= 8 and actual_commit.isalnum(): |
There was a problem hiding this comment.
The check part.isalnum() is insufficient for validating abbreviated commit hashes. Similar to the issue on line 596, this should validate hexadecimal characters specifically, not just alphanumeric characters.
| if len(part) >= 8 and part.isalnum(): | |
| actual_commit = part | |
| rdke_log(f"Extracted commit from version: {actual_commit}", "DEBUG") | |
| break | |
| # Determine link type based on commit value | |
| if len(actual_commit) >= 8 and actual_commit.isalnum(): | |
| if len(part) >= 8 and all(c in "0123456789abcdefABCDEF" for c in part): | |
| actual_commit = part | |
| rdke_log(f"Extracted commit from version: {actual_commit}", "DEBUG") | |
| break | |
| # Determine link type based on commit value | |
| if len(actual_commit) >= 8 and all(c in "0123456789abcdefABCDEF" for c in actual_commit): |
| default_srcrev = srcrev_data.get('SRCREV') | ||
| if default_srcrev: | ||
| # Use default SRCREV | ||
| if len(default_srcrev) == 40 and default_srcrev.isalnum(): |
There was a problem hiding this comment.
Same hexadecimal validation issue: default_srcrev.isalnum() should validate hex characters specifically.
| component_cache.save(updated_data) | ||
| rdke_log(f"Cache save() completed successfully for {pkg_pn}", "INFO", d) | ||
| except Exception as save_ex: | ||
| rdke_log(f"Cache save failed for {pkg_pn} with exception: {save_ex}", "ERROR", d) | ||
| import traceback | ||
| rdke_log(f"Save exception traceback: {traceback.format_exc()}", "ERROR", d) | ||
| raise save_ex |
There was a problem hiding this comment.
The cache save operation is not protected by any locking mechanism. Since do_collect_component_data can run concurrently for multiple recipes (after do_package), there's a potential race condition where multiple tasks could read-modify-write the cache simultaneously, leading to data loss. Consider using file locking (e.g., fcntl.flock) or BitBake's locking mechanisms to protect the cache during updates.
| component_cache.save(updated_data) | |
| rdke_log(f"Cache save() completed successfully for {pkg_pn}", "INFO", d) | |
| except Exception as save_ex: | |
| rdke_log(f"Cache save failed for {pkg_pn} with exception: {save_ex}", "ERROR", d) | |
| import traceback | |
| rdke_log(f"Save exception traceback: {traceback.format_exc()}", "ERROR", d) | |
| raise save_ex | |
| # Use file locking to protect cache save operation | |
| import fcntl | |
| lock_path = os.path.join(cache_dir, "rdke_component_data.lock") | |
| with open(lock_path, "w") as lock_file: | |
| try: | |
| fcntl.flock(lock_file, fcntl.LOCK_EX) | |
| component_cache.save(updated_data) | |
| rdke_log(f"Cache save() completed successfully for {pkg_pn}", "INFO", d) | |
| finally: | |
| fcntl.flock(lock_file, fcntl.LOCK_UN) |
| srcuri = pkg_info.get('srcuri', '') | ||
|
|
||
| rdke_log(f"create_version_hyperlink_for_repo called with: repo_name={repo_name}, srcrev_value={srcrev_value}, srcuri_type={srcuri_type}", "INFO") | ||
| rdke_log(f"pv={pv}, pr={pr}, srcuri={srcuri[:100]}...", "INFO") |
There was a problem hiding this comment.
String slicing with [:100] on srcuri can result in incomplete logging when the URI is longer than 100 characters. The ellipsis ... is always appended even if the string is shorter than 100 characters, which is misleading. Consider using srcuri[:100] + ('...' if len(srcuri) > 100 else '') instead.
| rdke_log(f"pv={pv}, pr={pr}, srcuri={srcuri[:100]}...", "INFO") | |
| rdke_log(f"pv={pv}, pr={pr}, srcuri={srcuri[:100] + ('...' if len(srcuri) > 100 else '')}", "INFO") |
| # Sort packages: packagegroup entries first, then alphabetically | ||
| def sort_key(pkg): | ||
| pkg_name = pkg.get('package-name', '') | ||
| # Put packagegroup entries first | ||
| if pkg_name.startswith('packagegroup-'): | ||
| return (0, pkg_name) | ||
| else: | ||
| return (1, pkg_name) |
There was a problem hiding this comment.
The inline lambda function for sorting (lines 815-821) makes the code harder to read. Since this is a reusable sorting pattern, consider defining it as a module-level function with a descriptive name like package_sort_key to improve code clarity.
| actual_commit = srcrev_value | ||
| if not (len(srcrev_value) == 40 and srcrev_value.isalnum()): | ||
| # SRCREV doesn't look like a commit hash, try to extract from version | ||
| if "+git" in version and "_" in version: | ||
| parts = version.split("_") | ||
| for part in reversed(parts): | ||
| if len(part) >= 8 and part.isalnum(): | ||
| actual_commit = part | ||
| rdke_log(f"Extracted commit from version: {actual_commit}", "DEBUG") | ||
| break |
There was a problem hiding this comment.
The logic for extracting commit hashes from version strings (lines 595-604) is duplicated in the create_version_hyperlink function. Consider extracting this into a shared helper function to reduce code duplication.
| else: | ||
| rdke_log(f"RDKE configured for qualifying recipe: {pn}", "DEBUG", d) | ||
|
|
||
| #rdke_log(f"RDKE configured for qualifying recipe: {pn}", "DEBUG", d) |
There was a problem hiding this comment.
This commented-out debug line should be removed. It appears to be leftover debug code that duplicates the logging on line 166.
| artifact_extensions = ['.tar.gz', '.tar.xz', '.tar.bz2', '.zip', '.tgz', | ||
| '.ipk', '.deb', '.rpm', '.jar', '.war', '.tar'] |
There was a problem hiding this comment.
The artifact_extensions list uses inconsistent spacing in the definition. The second line should align with the first line for better readability.
| break | ||
|
|
||
| # Determine link type based on commit value | ||
| if len(actual_commit) >= 8 and actual_commit.isalnum(): |
There was a problem hiding this comment.
The magic number 8 (minimum abbreviated commit hash length) is used here and on line 601. Consider defining it as a constant (e.g., MIN_ABBREVIATED_COMMIT_LENGTH = 8) for better code maintainability.
No description provided.