feat(build): resolve build deps from dependency graph instead of PEP 517 hooks#1009
Open
LalatenduMohanty wants to merge 1 commit intopython-wheel-build:mainfrom
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe pull request makes build-sequence consume a dependency graph file. Documentation examples and user docs now show Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Detailed notes
Actionable checks/suggestions
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/using.md`:
- Around line 190-193: The doc text is inconsistent: the intro correctly states
that dependency relationships come from graph.json but the Step 1 bullet still
points to build-order.json; update the Step 1 bullet in the using.md section
describing the build-sequence command so it says dependency relationships are
read from graph.json (and that build-order.json only contains the predetermined
build order), and ensure any mention of "dependency relationships" references
graph.json while build-order.json is described as the resolved build-order
artifact used to drive the build.
In `@src/fromager/build_environment.py`:
- Around line 343-353: The current graph-resolved install path wrongly tags
failures as RequirementType.BUILD_SYSTEM; change this to use a neutral label
instead: add a new enum member (e.g., RequirementType.GRAPH or
RequirementType.GRAPH_INSTALL) and pass that to _safe_install when installing
the mixed graph (the call site that currently passes
RequirementType.BUILD_SYSTEM for variables reqs, ctx, build_env, req). Ensure
the new enum value is declared where RequirementType is defined and update any
consumers/tests if needed so graph-driven install failures are reported with the
neutral label.
In `@src/fromager/commands/build.py`:
- Around line 201-203: The current lookup in _get_build_requirements_from_graph
(used where build_requirements is assigned) only matches dist_name and
resolved_version and can return stale graph.json entries; update the function to
also compare the graph node's download_url with the build-order
entry["source_url"] (or equivalent source field) and raise an explicit error
(fail fast) when they mismatch so the build aborts instead of using wrong
requirements; ensure the validation is applied in all call sites (including the
other block referenced around lines 585-598) and include a clear error message
identifying the package, expected source_url and actual download_url to aid
debugging.
🪄 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: ed405c67-ae44-42a1-916d-b0d1c084f5f8
📒 Files selected for processing (8)
docs/how-tos/bootstrap-constraints.rstdocs/using.mde2e/test_build_order.she2e/test_build_sequence_git_url.she2e/test_prebuilt_wheel_hook.shsrc/fromager/build_environment.pysrc/fromager/commands/build.pytests/test_build_environment.py
Comment on lines
+343
to
+353
| if reqs: | ||
| # Graph-resolved deps are a mix of build-system, build-backend, | ||
| # build-sdist, and their transitive install deps. We use | ||
| # BUILD_SYSTEM as the label since they all go into the build env. | ||
| _safe_install( | ||
| ctx=ctx, | ||
| req=req, | ||
| build_env=build_env, | ||
| deps=reqs, | ||
| dep_req_type=RequirementType.BUILD_SYSTEM, | ||
| ) |
There was a problem hiding this comment.
Don’t report mixed graph installs as build-system failures.
This path installs a mix of build-system, build-backend, build-sdist, and transitive install deps. If _safe_install() fails, labeling the whole batch as RequirementType.BUILD_SYSTEM points operators at the wrong phase. A neutral label for the graph-driven install path would make the error output accurate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fromager/build_environment.py` around lines 343 - 353, The current
graph-resolved install path wrongly tags failures as
RequirementType.BUILD_SYSTEM; change this to use a neutral label instead: add a
new enum member (e.g., RequirementType.GRAPH or RequirementType.GRAPH_INSTALL)
and pass that to _safe_install when installing the mixed graph (the call site
that currently passes RequirementType.BUILD_SYSTEM for variables reqs, ctx,
build_env, req). Ensure the new enum value is declared where RequirementType is
defined and update any consumers/tests if needed so graph-driven install
failures are reported with the neutral label.
…517 hooks build-sequence and build-parallel now use DependencyNode.iter_build_requirements() to populate build environments from the graph, eliminating the silent fallback to PEP 517 discovery hooks when cached dependency files were missing. build-sequence now requires a GRAPH_FILE argument alongside BUILD_ORDER_FILE. The fromager build command (debug/test) is unchanged. Closes: python-wheel-build#996 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>
LalatenduMohanty
force-pushed
the
issue-996-no-discovery-fallback
branch
from
3b4378f to
11abef4
Compare
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
1 participant
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.
build-sequence and build-parallel now use DependencyNode.iter_build_requirements() to populate build environments from the graph, eliminating the silent fallback to PEP 517 discovery hooks when cached dependency files were missing.
build-sequence now requires a GRAPH_FILE argument alongside BUILD_ORDER_FILE. The fromager build command (debug/test) is unchanged.
Breaking change:
build-sequencenow takesGRAPH_FILE BUILD_ORDER_FILE(two positional args instead of one).Closes: #996