ART-22058: Discover transitive build deps missed by pip_find_builddeps.py - #83
Conversation
…builddeps.py
The ART hermetic build for openshift-enterprise-ansible-operator has been
failing since Aug 10 (9 consecutive build_error runs) with:
Collecting setuptools
Using cached setuptools-82.0.1-py3-none-any.whl
ERROR: Could not find a version that satisfies the requirement calver
trove-classifiers (pulled into requirements-build.txt only as a transitive
dependency of hatchling) declares `calver` in its own pyproject.toml
[build-system].requires. pip_find_builddeps.py never scans it, because
Stage 3 only runs on Pipfile.lock's runtime packages -- and even pointed at
it directly, pip_find_builddeps.py couldn't discover it anyway, since it
relies on pip's dependency-report machinery, which doesn't surface
build-system requirements for a package that already has a wheel
(pypa/pip#7863). Hermeto/Cachi2 defaults to source-only prefetching, so
every package -- wheel or not -- is actually built from its sdist in the
hermetic sandbox, invoking that requirement regardless.
Add Stage 4b: after phase-splitting, iteratively fetch each newly-resolved
package's pyproject.toml directly from its sdist (bypassing pip entirely)
and inject any undeclared build-system requirement into the owning phase,
converging to a fixed point. This is fully dynamic -- no package names are
hardcoded -- so it will catch the same class of gap for any future package,
not just this one.
Verified with a fresh `make generate-requirements` run: requirements-build.txt
now pins calver==2025.10.20 (and poetry-core, needed to build hatchling's new
tomlkit dependency from source); no other files changed beyond routine
version drift (hatchling/wheel/vcs-versioning bumps already expected from
re-running pip-compile).
Co-authored-by: Cursor <cursoragent@cursor.com>
WalkthroughThe requirements generator now discovers transitive build requirements from package sdists, injects them into build phases, and iterates until closure or five passes. Supporting dependencies and documentation were updated. ChangesBuild dependency closure
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant main
participant Stage4b
participant _pyproject_build_system_requires
participant PyPI
participant build_phases
main->>Stage4b: process compiled build-phase pins
Stage4b->>_pyproject_build_system_requires: inspect uncovered package
_pyproject_build_system_requires->>PyPI: fetch sdist metadata
PyPI-->>_pyproject_build_system_requires: return build requirements
_pyproject_build_system_requires-->>Stage4b: return discovered requirements
Stage4b->>build_phases: inject requirements and recompile
build_phases-->>Stage4b: return compiled phase results
Stage4b-->>main: finish closure before CVE checks
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openshift/hack/generate_requirements.md`:
- Around line 441-445: Add the text language identifier to both fenced code
blocks in the requirements documentation: the error-output block near the
setuptools/calver message and the pseudocode block near the second reported
section. Leave their contents unchanged.
In `@openshift/hack/generate_requirements.py`:
- Around line 1171-1173: Update the Stage 3 probe tracking near rpm_norms and
probed so entries are keyed by phase and resolved package version rather than
normalized package name alone. Initialize probed from scanned using the matching
resolved versions, and ensure later probe checks preserve distinct package
versions across phases.
- Around line 1194-1228: Update the additions handling around _pip_compile so a
conflict in one build-system requirement does not discard compatible additions;
compile additions independently or split failed batches and retain successful
ones. Move probed.add(norm) from the initial loop until each package’s addition
is retained or explicitly rejected, ensuring failed batch entries remain
eligible for retry.
- Around line 1200-1207: Update the requirement handling loop around _norm and
resolved so req is parsed as a complete requirement, including markers and
specifiers, rather than comparing only its package name. Evaluate the marker and
check whether the resolved version satisfies the requirement; skip only
satisfied requirements, and append the original constraint when it is
unsatisfied while preserving the queued_norms behavior.
- Around line 233-235: Validate sdist_url at the trust boundary immediately
before urlopen in the sdist download flow. Allow only HTTPS URLs to the expected
PyPI file host, with no credentials and the default HTTPS port; reject every
other scheme, host, port, or malformed URL. Ensure redirects are disabled or
validated at each hop so the final download cannot leave the approved host.
In `@openshift/requirements-build.txt`:
- Around line 7-8: Update openshift/hack/generate_requirements.py to generate
build requirement files with hashes by enabling its --generate-hashes option,
and update openshift/install-ansible.sh to pass --require-hashes when installing
those requirements. Ensure regenerated artifacts contain hashes and pip enforces
them.
🪄 Autofix
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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3e102a6e-dfdb-4274-a9ca-ed7e10570770
📒 Files selected for processing (4)
openshift/Dockerfile.requirementsopenshift/hack/generate_requirements.mdopenshift/hack/generate_requirements.pyopenshift/requirements-build.txt
| ``` | ||
| Collecting setuptools | ||
| Using cached setuptools-82.0.1-py3-none-any.whl | ||
| ERROR: Could not find a version that satisfies the requirement calver (from versions: none) | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language identifiers to both fenced blocks.
Lines 441 and 459 start fenced blocks without a language identifier. markdownlint reports MD040 for both blocks. Use text for the error output and the pseudocode block.
Also applies to: 459-471
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 441-441: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift/hack/generate_requirements.md` around lines 441 - 445, Add the text
language identifier to both fenced code blocks in the requirements
documentation: the error-output block near the setuptools/calver message and the
pseudocode block near the second reported section. Leave their contents
unchanged.
Source: Linters/SAST tools
| try: | ||
| with urllib.request.urlopen(sdist_url, timeout=60) as resp: | ||
| raw = resp.read() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Allow-list the sdist download origin.
sdist_url comes from remote JSON and reaches urlopen() without validation. A substituted metadata response can direct the generator to an internal, non-HTTPS, or otherwise unapproved endpoint.
Require HTTPS and the expected PyPI file host before the download. Reject credentials, non-default ports, redirects to other hosts, and all other schemes.
As per path instructions, “Validate at trust boundaries with allow-lists, not deny-lists.”
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 233-233: Request-controlled URL passed to urlopen; validate against an allowlist to prevent SSRF.
Context: urllib.request.urlopen(sdist_url, timeout=60)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(urlopen-unsanitized-data)
🪛 Ruff (0.16.1)
[error] 234-234: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift/hack/generate_requirements.py` around lines 233 - 235, Validate
sdist_url at the trust boundary immediately before urlopen in the sdist download
flow. Allow only HTTPS URLs to the expected PyPI file host, with no credentials
and the default HTTPS port; reject every other scheme, host, port, or malformed
URL. Ensure redirects are disabled or validated at each hop so the final
download cannot leave the approved host.
Sources: Path instructions, Linters/SAST tools
| rpm_norms = {_norm(p) for p in RPM_INSTALLED} | ||
| probed: set[str] = set(scanned) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Track probes by phase and resolved version.
probed stores only normalized package names. _pyproject_build_system_requires() reads version-specific metadata, and Stage 4 can place different versions in different phases. After probing package==A, this code skips package==B in another phase.
Key probe state by phase and version. Initialize the Stage 3 probe state with the matching resolved versions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift/hack/generate_requirements.py` around lines 1171 - 1173, Update the
Stage 3 probe tracking near rpm_norms and probed so entries are keyed by phase
and resolved package version rather than normalized package name alone.
Initialize probed from scanned using the matching resolved versions, and ensure
later probe checks preserve distinct package versions across phases.
| additions: list[str] = [] | ||
| queued_norms: set[str] = set() | ||
| for norm, pkg_line in sorted(new_pkgs.items()): | ||
| probed.add(norm) | ||
| pkg_name, pkg_version = pkg_line.split("==", 1) | ||
| requires = _pyproject_build_system_requires(pkg_name, pkg_version) | ||
| for req in requires: | ||
| m = re.match(r"^([A-Za-z0-9][A-Za-z0-9._-]*)", req.strip()) | ||
| if not m: | ||
| continue | ||
| req_norm = _norm(m.group(1)) | ||
| if req_norm in resolved or req_norm in queued_norms: | ||
| continue # already pinned in this phase, or already queued below | ||
| additions.append(req.strip()) | ||
| queued_norms.add(req_norm) | ||
| print( | ||
| f" + {req.strip()} (build-system requirement of" | ||
| f" {pkg_name}=={pkg_version}, undetected by" | ||
| " pip_find_builddeps.py — see pypa/pip#7863)" | ||
| ) | ||
|
|
||
| if not additions: | ||
| continue | ||
|
|
||
| original_in = in_path.read_text() | ||
| in_path.write_text(original_in.rstrip("\n") + "\n" + "\n".join(additions) + "\n") | ||
| ok, stderr = _pip_compile(in_path, txt_path, ["--allow-unsafe"]) | ||
| if not ok: | ||
| print( | ||
| f" WARNING: pip-compile failed after injecting" | ||
| f" {additions} into {label}; reverting:\n{stderr}", | ||
| file=sys.stderr, | ||
| ) | ||
| in_path.write_text(original_in) | ||
| continue |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep compatible additions when one addition conflicts.
This code compiles all additions as one batch. If one requirement conflicts, Lines 1221-1228 revert every addition. Lines 1196-1199 already mark every package as probed, so compatible requirements dropped with the batch are never retried.
Compile additions independently, or split a failed batch. Record probe completion only after an addition is retained or explicitly recorded as rejected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift/hack/generate_requirements.py` around lines 1194 - 1228, Update the
additions handling around _pip_compile so a conflict in one build-system
requirement does not discard compatible additions; compile additions
independently or split failed batches and retain successful ones. Move
probed.add(norm) from the initial loop until each package’s addition is retained
or explicitly rejected, ensuring failed batch entries remain eligible for retry.
| for req in requires: | ||
| m = re.match(r"^([A-Za-z0-9][A-Za-z0-9._-]*)", req.strip()) | ||
| if not m: | ||
| continue | ||
| req_norm = _norm(m.group(1)) | ||
| if req_norm in resolved or req_norm in queued_norms: | ||
| continue # already pinned in this phase, or already queued below | ||
| additions.append(req.strip()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Check whether the resolved version satisfies the requirement.
The req_norm in resolved check only compares package names. If the phase contains calver==2024.1 and the sdist requires calver>=2025, this code skips the constraint and leaves an incompatible build environment.
Parse the complete requirement, evaluate its marker and specifier against the resolved version, and inject the constraint when it is not satisfied.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift/hack/generate_requirements.py` around lines 1200 - 1207, Update the
requirement handling loop around _norm and resolved so req is parsed as a
complete requirement, including markers and specifiers, rather than comparing
only its package name. Evaluate the marker and check whether the resolved
version satisfies the requirement; skip only satisfied requirements, and append
the original constraint when it is unsatisfied while preserving the queued_norms
behavior.
| calver==2025.10.20 | ||
| # via -r /requirements-build.in |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --glob='*.py' --glob='*.sh' --glob='Dockerfile*' \
'pip-compile|--generate-hashes|--require-hashes|pip install.*requirements-(pre-build|build1|build)\.txt' \
openshiftRepository: openshift/ansible-operator-plugins
Length of output: 4840
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- requirements-build.txt ---'
cat -n openshift/requirements-build.txt | sed -n '1,90p'
printf '%s\n' '--- install-ansible.sh ---'
cat -n openshift/install-ansible.sh | sed -n '1,45p'
printf '%s\n' '--- generation and container configuration ---'
cat -n openshift/Dockerfile.requirements | sed -n '1,90p'
printf '%s\n' '--- requirements files ---'
for f in openshift/requirements-*.txt; do
printf '\n[%s]\n' "$f"
sed -n '1,14p' "$f"
done
printf '%s\n' '--- repository diff summary and changed dependency lines ---'
git diff --stat
git diff -- openshift/requirements-build.txt openshift/install-ansible.sh openshift/Dockerfile.requirements | sed -n '1,220p'
printf '%s\n' '--- hash-related configuration and consumers ---'
rg -n --hidden --glob '!*.pyc' \
'generate-hashes|require-hashes|--hash=|requirements-(pre-build|build1|build)\.txt|PIP_OPTS' \
openshiftRepository: openshift/ansible-operator-plugins
Length of output: 13196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- pip-compile invocations and output writing ---'
rg -n -C 8 \
'def .*compile|pip-compile|generate-hashes|--output-file|requirements-(pre-build|build1|build)\.txt|write_text|write_bytes' \
openshift/hack/generate_requirements.py
printf '%s\n' '--- all compiled requirement contents and hash presence ---'
for f in openshift/requirements.txt openshift/requirements-pre-build.txt openshift/requirements-build1.txt openshift/requirements-build.txt; do
if [ -f "$f" ]; then
printf '\n[%s]\n' "$f"
cat -n "$f" | sed -n '1,45p'
fi
done
printf '%s\n' '--- requirement inputs and generation documentation ---'
for f in openshift/*.in; do
printf '\n[%s]\n' "$f"
cat -n "$f"
done
rg -n -C 5 'hash|pip-compile|requirements-build\.txt|install-ansible' \
openshift/hack/generate_requirements.md openshift/MakefileRepository: openshift/ansible-operator-plugins
Length of output: 37667
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- final build-file generation and post-processing ---'
sed -n '995,1140p' openshift/hack/generate_requirements.py
sed -n '1180,1240p' openshift/hack/generate_requirements.py
sed -n '1340,1405p' openshift/hack/generate_requirements.py
printf '%s\n' '--- deterministic hash coverage check ---'
python3 - <<'PY'
from pathlib import Path
import re
for path in sorted(Path("openshift").glob("requirements*.txt")):
text = path.read_text()
active = []
current = None
for line in text.splitlines():
stripped = line.strip()
if not stripped or stripped.startswith("#"):
continue
m = re.match(r"^([A-Za-z0-9][A-Za-z0-9._-]*)==([^ #]+)", stripped)
if m:
current = m.group(1)
active.append(current)
elif "--hash=" in stripped:
current = None
# A simple package-level check: each active pin must have a hash before
# the next active pin or end of file.
missing = []
for package in active:
pattern = rf"(?ms)^{re.escape(package)}==[^\\n]+\\n(?:(?!^[A-Za-z0-9][A-Za-z0-9._-]*==).)*?--hash="
if not re.search(pattern, text):
missing.append(package)
print(f"{path}: active_pins={len(active)} missing_hashes={len(missing)}")
if missing:
print(" " + ", ".join(missing))
PY
printf '%s\n' '--- generation command sites ---'
rg -n -C 3 '_pip_compile\([^)]*txt_path|_pip_compile\([^)]*out_txt|_pip_compile\([^)]*build_txt|_pip_compile\(' \
openshift/hack/generate_requirements.pyRepository: openshift/ansible-operator-plugins
Length of output: 17725
Enable hash checking for generated build requirements.
openshift/hack/generate_requirements.py generates and rewrites build requirement files without --generate-hashes. openshift/install-ansible.sh installs them without --require-hashes. Update both so regenerated artifacts contain hashes and pip enforces them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift/requirements-build.txt` around lines 7 - 8, Update
openshift/hack/generate_requirements.py to generate build requirement files with
hashes by enabling its --generate-hashes option, and update
openshift/install-ansible.sh to pass --require-hashes when installing those
requirements. Ensure regenerated artifacts contain hashes and pip enforces them.
Source: Path instructions
|
/retest |
mytreya-rh
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@mytreya-rh: This pull request references ART-22058 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
chiragkyal
left a comment
chiragkyal
left a comment
There was a problem hiding this comment.
As discussed over Slack, we need to revisit the automation approach again to make it robust. Approving this PR to make room for ART build, with a hope to fix the breaking build.
/lgtm
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chiragkyal, mytreya-rh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/verified by ci |
openshift-ci-robot
added
the
verified
|
@chiragkyal: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@mytreya-rh: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Summary
The ART hermetic build for
openshift-enterprise-ansible-operatorhas been failing since Aug 10 (9 consecutivebuild_errorruns) with:Root cause:
trove-classifiers(pulled intorequirements-build.txtonly as a transitive dependency ofhatchling) declarescalverin its ownpyproject.toml[build-system].requires.pip_find_builddeps.pynever scans it — Stage 3 ofgenerate_requirements.pyonly runs onPipfile.lock's runtime packages, not on second-order build tools pulled in bypip-compile. Even pointed at it directly,pip_find_builddeps.pycouldn't discover this, since it relies on pip's own dependency-report machinery, which doesn't surface[build-system]requirements for a package that already has a wheel (pypa/pip#7863). Hermeto/Cachi2 defaults to source-only prefetching, so every package — wheel or not — is actually built from its sdist inside the hermetic sandbox, invoking that requirement regardless.Fix: Add Stage 4b to
generate_requirements.py. After phase-splitting, it iteratively fetches each newly-resolved (previously unscanned) package'spyproject.tomldirectly from its sdist on PyPI — bypassing pip entirely — and injects any undeclared build-system requirement into the owning phase, converging to a fixed point. This is fully dynamic (no package names hardcoded), so it will catch the same class of gap for any future package, not justcalver/trove-classifiers.openshift/hack/generate_requirements.py: new_pyproject_build_system_requires()helper andstage4b_close_transitive_build_deps()stage, wired in after Stage 4.openshift/hack/generate_requirements.md: new "Stage 4b" section documenting the problem and fix.openshift/Dockerfile.requirements: updated stage comment list.openshift/requirements-build.txt: regenerated — addscalver==2025.10.20(the fix) andpoetry-core(needed to buildhatchling's newtomlkitdependency from source); other changes are routinepip-compileversion drift (hatchling/wheel/vcs-versioningbumps) unrelated to this fix.Test plan
_pyproject_build_system_requiresdirectly againsttrove-classifiers(returns['setuptools', 'calver']) and edge cases (missing package, no[build-system]table).pip-toolsvenv reproduced the bug (hatchling → trove-classifiers with nocalver) and confirmed Stage 4b's loop discovers and injects it correctly.make -f openshift/Makefile generate-requirementsrun (fresh--no-cachecontainer build): Stage 4b fires twice, addscalver/poetry-core, converges cleanly; Stage 5/6 pass with no warnings.requirements-build.txtchanged (the new packages plus expected drift);requirements.txt,requirements-pre-build.txt,requirements-build1.txt,Pipfile.lockuntouched.go build ./...andgo vet ./...pass (no Go code touched).Made with Cursor
Summary by CodeRabbit
New Features
Documentation
Chores