port_manager: generate SPDX SBOM v2.2 during build#288
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces SPDX v2.2 Software Bill of Materials (SBOM) generation capabilities to the port manager, refactoring candidate classes to track metadata such as licenses, SHA256 checksums, sources, and CPEs, and adding a shared environment file (ports.env). Feedback on these changes highlights a missing Path import in sbom.py that would cause a runtime error, a backward-compatibility syntax error with nested double quotes in f-strings, and a fragile relative path when sourcing ports.env in Bash. Additionally, improvements were suggested to use timezone-aware UTC datetimes for SPDX compliance, specify file encoding explicitly, and enhance the robustness of the environment parser.
6ca5559 to
231f487
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
231f487 to
0585304
Compare
Unit Test Results10 890 tests 10 220 ✅ 53m 38s ⏱️ Results for commit c826481. ♻️ This comment has been updated with latest results. |
c826481 to
d95e269
Compare
| b_build_test | ||
| fi | ||
|
|
||
| SBOM_ARGS=() |
d95e269 to
47057fa
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces Software Bill of Materials (SBOM) generation capabilities to the Phoenix-RTOS build system, adding a new aggregation script, integrating metadata collection into the build process, and updating the port manager to generate ports SBOMs. The review feedback highlights several critical improvements, such as resolving potential KeyError exceptions when accessing environment variables, fixing an incorrect logging call, adding safety checks for directory existence and enum parsing, specifying explicit file encoding, and replacing external hashing subprocesses with Python's native hashlib for better portability and performance.
| ) | ||
|
|
||
| write_document_to_file(doc, str(output_path)) | ||
| logger.info("SBOM saved to", output_path.absolute()) |
There was a problem hiding this comment.
In Python's logging library (and typical custom loggers), logger.info does not concatenate multiple arguments like print does. Instead, additional arguments are used as string formatting parameters. Since there are no placeholders in the format string, the output path will be omitted from the log. Use an f-string instead.
| logger.info("SBOM saved to", output_path.absolute()) | |
| logger.info(f"SBOM saved to {output_path.absolute()}") |
| def run_reuse_on_directory(directory_path: Path) -> tuple[str, str | None]: | ||
| """Runs reuse tool on given path. Returns a pair of strings: copyrights | ||
| field and licenses SPDX expression""" | ||
| project = Project(directory_path) |
There was a problem hiding this comment.
If directory_path does not exist or is not a directory, initializing Project(directory_path) or calling project.all_files() will raise an exception (e.g., FileNotFoundError). It is safer to check if the path is a directory before proceeding.
| def run_reuse_on_directory(directory_path: Path) -> tuple[str, str | None]: | |
| """Runs reuse tool on given path. Returns a pair of strings: copyrights | |
| field and licenses SPDX expression""" | |
| project = Project(directory_path) | |
| def run_reuse_on_directory(directory_path: Path) -> tuple[str, str | None]: | |
| """Runs reuse tool on given path. Returns a pair of strings: copyrights | |
| field and licenses SPDX expression""" | |
| if not directory_path.is_dir(): | |
| logger.warning(f"Directory {directory_path} does not exist or is not a directory.") | |
| return "", None | |
| project = Project(directory_path) |
| def run_subprocess(filepath: Path, command: str) -> str: | ||
| result = subprocess.run( | ||
| [command, filepath.absolute()], capture_output=True, text=True, check=True | ||
| ) | ||
| return result.stdout.split()[0] | ||
|
|
||
|
|
||
| def libpath_to_spdx_file(path: Path, license_spdx: str) -> File: | ||
| lib_name = path.name | ||
| spdx_id = libpath_to_spdx_id(str(path)) | ||
|
|
||
| if not os.path.isfile(path): | ||
| raise FileNotFoundError(path) | ||
|
|
||
| return File( | ||
| name=lib_name, | ||
| spdx_id=spdx_id, | ||
| checksums=[ | ||
| Checksum(ChecksumAlgorithm.SHA1, run_subprocess(path, "sha1sum")), | ||
| Checksum(ChecksumAlgorithm.SHA256, run_subprocess(path, "sha256sum")), | ||
| ], |
There was a problem hiding this comment.
Spawning external processes like sha1sum and sha256sum via subprocess.run is slow, resource-intensive, and not cross-platform (e.g., macOS does not have these commands by default). Since this is Python, we can easily compute SHA1 and SHA256 in pure Python using the standard hashlib library. This is much faster, more secure, and completely portable.
def compute_file_hash(filepath: Path, algo: str) -> str:
import hashlib
hasher = hashlib.new(algo)
with filepath.open("rb") as f:
for chunk in iter(lambda: f.read(65536), b""):
hasher.update(chunk)
return hasher.hexdigest()
def libpath_to_spdx_file(path: Path, license_spdx: str) -> File:
lib_name = path.name
spdx_id = libpath_to_spdx_id(str(path))
if not os.path.isfile(path):
raise FileNotFoundError(path)
return File(
name=lib_name,
spdx_id=spdx_id,
checksums=[
Checksum(ChecksumAlgorithm.SHA1, compute_file_hash(path, "sha1")),
Checksum(ChecksumAlgorithm.SHA256, compute_file_hash(path, "sha256")),
],| sys.exit(1) | ||
| version = frag.get("version", phoenix_version) | ||
| makefile_path = Path(frag["makefile_path"]) | ||
| generated_type = GeneratedType(frag.get("type", "")) |
There was a problem hiding this comment.
If frag does not contain a "type" key, or if the value is not one of the defined GeneratedType enum values (e.g., if it is empty or has an unexpected value), GeneratedType(frag.get("type", "")) will raise a ValueError and crash the script. It is safer to handle this gracefully or use a fallback.
| generated_type = GeneratedType(frag.get("type", "")) | |
| try: | |
| generated_type = GeneratedType(frag.get("type", "")) | |
| except ValueError: | |
| generated_type = None |
|
|
||
| def generate_sbom(self): | ||
| from .sbom import generate_ports_sbom | ||
| output_path = Path(os.environ.get("PORTS_SBOM_PATH", Path(os.environ["PREFIX_BUILD"]) / "ports.spdx.json")) |
There was a problem hiding this comment.
In Python, the default argument of dict.get(key, default) is always evaluated, even if the key exists in the dictionary. Therefore, os.environ["PREFIX_BUILD"] will be accessed and can raise a KeyError if PREFIX_BUILD is missing, even when PORTS_SBOM_PATH is set. Use a conditional expression to avoid evaluating the default value unnecessarily.
| output_path = Path(os.environ.get("PORTS_SBOM_PATH", Path(os.environ["PREFIX_BUILD"]) / "ports.spdx.json")) | |
| output_path = Path(os.environ["PORTS_SBOM_PATH"]) if "PORTS_SBOM_PATH" in os.environ else Path(os.environ["PREFIX_BUILD"]) / "ports.spdx.json" |
| def get_phoenix_ver() -> str: | ||
| # ignore any abbrevs that may possibly be emitted if version is taken with `git describe` | ||
| return os.environ["PHOENIX_VER"].split("-", 1)[0] |
There was a problem hiding this comment.
If PHOENIX_VER is not set in the environment (for example, if build-ports.sh or port_manager.py is run directly without going through build.sh), os.environ["PHOENIX_VER"] will raise a KeyError. It is safer to use os.environ.get() with a sensible default fallback.
| def get_phoenix_ver() -> str: | |
| # ignore any abbrevs that may possibly be emitted if version is taken with `git describe` | |
| return os.environ["PHOENIX_VER"].split("-", 1)[0] | |
| def get_phoenix_ver() -> str: | |
| # ignore any abbrevs that may possibly be emitted if version is taken with `git describe` | |
| phoenix_ver = os.environ.get("PHOENIX_VER", "v0.0.0") | |
| return phoenix_ver.split("-", 1)[0] |
| fragments = [] | ||
| for path in sorted(build_dir.rglob("*.sbom.json")): | ||
| try: | ||
| data = json.loads(path.read_text()) |
There was a problem hiding this comment.
When reading files using Path.read_text(), it is highly recommended to specify the encoding explicitly (e.g., encoding="utf-8"). Otherwise, Python will fall back to the system's default encoding, which can cause UnicodeDecodeError on systems with different default locales (like Windows or some minimal Docker containers).
| data = json.loads(path.read_text()) | |
| data = json.loads(path.read_text(encoding="utf-8")) |
TASK: RTOS-1378
Assisted-by: Claude:Claude-sonnet-4-6 TASK: RTOS-1378
47057fa to
e80cbe2
Compare
1 participant
TASK: RTOS-1378
Description
Motivation and Context
Types of changes
How Has This Been Tested?
Checklist:
Special treatment