Open
Conversation
tiran
reviewed
Mar 24, 2026
Contributor
Author
|
@lcarva fyi, in case you (or someone else from the Calunga team) wanted to give this a looksy as well. |
tiran
reviewed
Mar 25, 2026
lcarva
reviewed
Mar 25, 2026
mprpic
force-pushed
the
generate-sbom
branch
2 times, most recently
from
e1fe09a to
7cdcffb
Compare
Contributor
Author
|
Tested locally in Builder: main!bootstrap-output.cpu-ubi9/wheels-repo/downloads [*] > d
diff --git a/overrides/settings.yaml b/overrides/settings.yaml
index b452bc03..29a12a52 100644
--- a/overrides/settings.yaml
+++ b/overrides/settings.yaml
@@ -1,4 +1,10 @@
---
+sbom:
+ supplier: "Organization: Red Hat"
+ namespace: "https://www.redhat.com"
+ creators:
+ - "Organization: Red Hat"
+
[...truncated...]
main!bootstrap-output.cpu-ubi9/wheels-repo/downloads [*] > ccat markupsafe-3.0.3.dist-info/sboms/fromager.spdx.json
{
"spdxVersion": "SPDX-2.3",
"dataLicense": "CC0-1.0",
"SPDXID": "SPDXRef-DOCUMENT",
"name": "markupsafe-3.0.3",
"documentNamespace": "https://www.redhat.com/markupsafe-3.0.3.spdx.json",
"creationInfo": {
"created": "2026-03-27T19:42:02Z",
"creators": [
"Organization: Red Hat",
"Tool: fromager-0.1.dev2028+g7cdcffb44"
]
},
"packages": [
{
"SPDXID": "SPDXRef-wheel",
"name": "markupsafe",
"versionInfo": "3.0.3",
"downloadLocation": "NOASSERTION",
"supplier": "Organization: Red Hat"
}
],
"relationships": [
{
"spdxElementId": "SPDXRef-DOCUMENT",
"relationshipType": "DESCRIBES",
"relatedSpdxElement": "SPDXRef-wheel"
}
]
} |
rd4398
requested changes
Mar 31, 2026
Contributor
rd4398
left a comment
rd4398
left a comment
There was a problem hiding this comment.
The implementation looks okay to me! I had a few comments / suggestions.
Log any existing SBOM files in .dist-info/sboms/ at INFO level during wheel metadata injection. This provides visibility into upstream SBOMs (e.g. from maturin) before Fromager generates its own. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Martin Prpič <mprpic@redhat.com>
📝 WalkthroughWalkthroughAdds SPDX 2.3 SBOM generation and writing for wheels. Introduces a new SbomSettings model and exposes it from the packagesettings package, and adds an optional per-package Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Generate and embed SPDX 2.3 SBOM documents in built wheels per PEP770.
SBOM generation is enabled by adding an `sbom` section to settings.yaml
with supplier, namespace, and creators fields. When absent, no SBOMs
are generated.
- Add SbomSettings model (supplier, namespace, creators) to settings.yaml
- Add per-package purl override with {name}/{version} format substitution
- Create sbom module that generates and writes out SBOMs
- Wire into add_extra_metadata_to_wheels() gated by sbom settings presence
- Write fromager.spdx.json to .dist-info/sboms/, preserving existing SBOMs
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Martin Prpič <mprpic@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/fromager/sbom.py (1)
116-133: Add docstring note about overwrite behavior.Per past review discussion: the function will overwrite an existing
fromager.spdx.jsonif present. Consider adding a brief note to the docstring explaining this is expected since Fromager generates exactly one SBOM per wheel build.📝 Suggested docstring update
def write_sbom( *, sbom: dict[str, typing.Any], dist_info_dir: pathlib.Path, ) -> pathlib.Path: """Write an SBOM document to the .dist-info/sboms/ directory. Creates the sboms/ subdirectory if it does not already exist. Returns the path to the written file. + + Note: This will overwrite any existing fromager.spdx.json file, + which is expected since Fromager generates exactly one SBOM per + wheel build. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/sbom.py` around lines 116 - 133, The docstring for write_sbom currently omits that it will overwrite an existing SBOM file; update the function's docstring (write_sbom) to explicitly state that writing to dist_info_dir / "sboms" / SBOM_FILENAME will replace any existing file (this is expected because Fromager generates a single SBOM per wheel build), so callers should not rely on preservation of previous files. Include a brief one-line note about overwrite behavior in the existing docstring.tests/test_sbom.py (1)
17-33: Avoid mutating privateSettingsinternals in test setup.Line 33 directly writes to
settings._package_settings, which makes tests brittle if internals change. Prefer passing package settings through the public constructor argument.♻️ Proposed refactor
def _make_ctx( tmp_path: pathlib.Path, sbom_settings: SbomSettings | None = None, purl: str | None = None, ) -> context.WorkContext: """Create a minimal WorkContext with SBOM settings.""" + package_settings: list[packagesettings.PackageSettings] = [] + if purl is not None: + package_settings.append( + packagesettings.PackageSettings.from_mapping( + "test-pkg", + {"purl": purl}, + source="test", + has_config=True, + ) + ) + settings_file = packagesettings.SettingsFile(sbom=sbom_settings) settings = packagesettings.Settings( settings=settings_file, - package_settings=[], + package_settings=package_settings, patches_dir=tmp_path / "patches", variant="cpu", max_jobs=None, ) - if purl is not None: - # Create a package setting with purl override - ps = packagesettings.PackageSettings.from_mapping( - "test-pkg", - {"purl": purl}, - source="test", - has_config=True, - ) - settings._package_settings[ps.name] = ps🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_sbom.py` around lines 17 - 33, Test mutates private internals by writing to settings._package_settings; instead construct the Settings with package settings provided publicly: create the PackageSettings via packagesettings.PackageSettings.from_mapping(...) and pass it in the package_settings list to packagesettings.Settings(...) (use SettingsFile / Settings constructors shown) instead of assigning to the private attribute _package_settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/fromager/sbom.py`:
- Around line 116-133: The docstring for write_sbom currently omits that it will
overwrite an existing SBOM file; update the function's docstring (write_sbom) to
explicitly state that writing to dist_info_dir / "sboms" / SBOM_FILENAME will
replace any existing file (this is expected because Fromager generates a single
SBOM per wheel build), so callers should not rely on preservation of previous
files. Include a brief one-line note about overwrite behavior in the existing
docstring.
In `@tests/test_sbom.py`:
- Around line 17-33: Test mutates private internals by writing to
settings._package_settings; instead construct the Settings with package settings
provided publicly: create the PackageSettings via
packagesettings.PackageSettings.from_mapping(...) and pass it in the
package_settings list to packagesettings.Settings(...) (use SettingsFile /
Settings constructors shown) instead of assigning to the private attribute
_package_settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a88eb459-5fe5-4470-8a85-fa2f3b555d69
📒 Files selected for processing (9)
src/fromager/packagesettings/__init__.pysrc/fromager/packagesettings/_models.pysrc/fromager/packagesettings/_pbi.pysrc/fromager/packagesettings/_settings.pysrc/fromager/sbom.pysrc/fromager/wheels.pytests/test_packagesettings.pytests/test_sbom.pytests/test_wheels.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fromager/sbom.py`:
- Around line 41-43: The purl_override.format(...) call can raise
KeyError/ValueError for malformed templates; wrap the .format(name=package_name,
version=package_version) call in a try/except that catches KeyError and
ValueError and re-raises a new ValueError with a clear message including the
offending template (purl_override) and allowed placeholders ("name" and
"version") plus package_name/package_version for context; ensure the original
exception is chained (raise ... from e) so traceback is preserved and return
None only when purl_override is falsy as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8787098-25e1-4989-b849-3e0e8e491b67
📒 Files selected for processing (9)
src/fromager/packagesettings/__init__.pysrc/fromager/packagesettings/_models.pysrc/fromager/packagesettings/_pbi.pysrc/fromager/packagesettings/_settings.pysrc/fromager/sbom.pysrc/fromager/wheels.pytests/test_packagesettings.pytests/test_sbom.pytests/test_wheels.py
✅ Files skipped from review due to trivial changes (2)
- tests/test_packagesettings.py
- tests/test_sbom.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/fromager/packagesettings/_pbi.py
- src/fromager/packagesettings/_settings.py
- tests/test_wheels.py
- src/fromager/wheels.py
- src/fromager/packagesettings/init.py
Comment on lines
+41
to
+43
| if purl_override: | ||
| return purl_override.format(name=package_name, version=package_version) | ||
| return None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify direct template expansion site and whether malformed-template handling exists.
rg -n -C3 'purl_override\.format\(' src/fromager/sbom.py
rg -n -C2 'invalid purl template|supported fields|KeyError|ValueError' src/fromager/sbom.py tests/test_sbom.py || trueRepository: python-wheel-build/fromager
Length of output: 239
Handle malformed PURL templates explicitly.
Line 42 directly calls .format() without error handling. Malformed templates (e.g., unknown placeholders, bad braces) will raise KeyError or ValueError with unclear context. Wrap it in a try-catch and re-raise with a helpful message per coding guidelines.
Suggested fix
if purl_override:
- return purl_override.format(name=package_name, version=package_version)
+ try:
+ return purl_override.format(name=package_name, version=str(package_version))
+ except (KeyError, ValueError) as err:
+ raise ValueError(
+ f"invalid purl template {purl_override!r}; supported fields are "
+ "{name} and {version}"
+ ) from err
return None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if purl_override: | |
| return purl_override.format(name=package_name, version=package_version) | |
| return None | |
| if purl_override: | |
| try: | |
| return purl_override.format(name=package_name, version=str(package_version)) | |
| except (KeyError, ValueError) as err: | |
| raise ValueError( | |
| f"invalid purl template {purl_override!r}; supported fields are " | |
| "{name} and {version}" | |
| ) from err | |
| return None |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fromager/sbom.py` around lines 41 - 43, The purl_override.format(...)
call can raise KeyError/ValueError for malformed templates; wrap the
.format(name=package_name, version=package_version) call in a try/except that
catches KeyError and ValueError and re-raises a new ValueError with a clear
message including the offending template (purl_override) and allowed
placeholders ("name" and "version") plus package_name/package_version for
context; ensure the original exception is chained (raise ... from e) so
traceback is preserved and return None only when purl_override is falsy as
before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds logging for discovering existing SBOMs in wheel being built, and a minimal implementation of a Fromager-specific SBOM. The values are set to Red Hat-specific metadata right now, but can be exposed via configuration to arbitrary values in a future PR (or I can tack on that change here as well if necessary).