docs(proposals): global hook for build-system dependency post-processing - #1272
docs(proposals): global hook for build-system dependency post-processing#1272vshawrh wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds a proposal for a Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@vshawrh CI is failing here |
8c0b285 to
21c93a4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/proposals/global-build-system-dependencies-hook.md`:
- Line 7: Update the GitHub PR reference in the proposal metadata to point to PR
`#1272`, replacing the current `#1271` link while preserving the existing link
format.
🪄 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 Plus
Run ID: 79113956-8103-4ccd-aad0-004c6a8a5c2e
📒 Files selected for processing (1)
docs/proposals/global-build-system-dependencies-hook.md
2e8b635 to
89eee4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/proposals/global-build-system-dependencies-hook.md`:
- Around line 139-146: Update the proposal documentation around
get_build_system_dependencies and its invocation examples to state that Fromager
materializes the Iterable[str] returned by overrides.find_and_invoke() into a
list before calling the global hook. Clarify that the hook receives list[str],
so expressions such as requirements + ["setuptools<82"] work for generator,
tuple, and set results from per-package plugins.
- Around line 124-134: Update the documented build-system dependency resolution
contract around the cache lookup and hook execution flow to define invalidation
when global hooks are installed or changed. Either require clearing existing
build-system-requirements.txt caches or specify how active hook
configuration/version data participates in the cache identity, ensuring stale
requirements cannot bypass run_get_build_system_dependencies_hooks().
- Around line 33-37: Update the Setuptools 81 description in the proposal to
state that it removed support for the setup.py --dry-run option and changed
related class/function signatures; remove the unsupported claim that
distutils.spawn(dry_run=...) and remove_tree(dry_run=...) were removed. Keep the
separate Setuptools 82 pkg_resources removal statement unchanged.
- Line 153: Define an explicit invocation-order contract for global
depends_on_requirement_hook handlers instead of relying on stevedore’s
HookManager discovery order. Either add a Fromager-controlled priority mechanism
or require hooks to be order-independent, and document and test behavior when
multiple global hooks modify build requirements.
🪄 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 Plus
Run ID: f2b63f90-4cbe-484c-940a-272885cc7a66
📒 Files selected for processing (1)
docs/proposals/global-build-system-dependencies-hook.md
rd4398
left a comment
rd4398
left a comment
There was a problem hiding this comment.
I have left a few comments on the updated proposal. Also, this proposal sets a precedent and if accepted, we may want other override hooks the same treatment (e.g.,get_build_backend_dependencies, get_build_sdist_dependencies). I would prefer if @LalatenduMohanty and @tiran take a look at this as well.
| A minimal hook that appends a constraint: | ||
|
|
||
| ```python | ||
| def get_build_system_dependencies( |
There was a problem hiding this comment.
Note: Today's global hooks (post_build, post_bootstrap, prebuilt_wheel) are fire-and-forget — they return nothing, they don't chain, and execution order doesn't matter. The proposed get_build_system_dependencies hook fundamentally changes this: hooks receive the previous hook's output and return a modified list.
There was a problem hiding this comment.
@rd4398 You are absolutely correct.
The stevedore hooks are event listener hooks. They are designed to act on events like "wheel is ready". The hooks are registered and enabled at installation time of a package. The hooks are not suited to change behavior.
| The hook receives the current requirements list and must return a | ||
| (possibly modified) `list[str]`. When multiple hooks are registered, | ||
| they chain: each receives the previous hook's output. Execution order | ||
| follows stevedore's `HookManager` iteration order. |
There was a problem hiding this comment.
HookManager doesn't guarantee order — it depends on entry point discovery, which varies across installations and Python versions. For side-effect hooks this is fine. For chained hooks where hook_A(hook_B(deps)) may differ from hook_B(hook_A(deps)), it's a problem. How should users control or reason about order?
|
|
||
| The hook receives the current requirements list and must return a | ||
| (possibly modified) `list[str]`. When multiple hooks are registered, | ||
| they chain: each receives the previous hook's output. Execution order |
There was a problem hiding this comment.
If one hook in the chain raises an exception, does the entire chain fail? Does the build fail? Are subsequent hooks skipped? The existing hooks don't need to answer this because they're independent but chained hooks do.
|
|
||
| ## Proposed approaches | ||
|
|
||
| ### Option A: Core logic in Fromager (PR [#1264](https://github.com/python-wheel-build/fromager/pull/1264)) |
There was a problem hiding this comment.
I think we already rejected option A right? Can we just document why we rejected it?
| adding build dependencies. However, the setuptools cap is conditional | ||
| and depends on what APIs a given `setup.py` actually uses. A static YAML | ||
| entry would either over-constrain all packages or require per-package | ||
| entries, which has the same maintenance burden as plugins. |
There was a problem hiding this comment.
I do not think per-package entries are same maintenance burden as plugins but they still need extra work to be done which can be handled in an easier way.
| - None. Delete the 22 identical plugins and their entry points. The | ||
| packages build correctly without any downstream configuration, | ||
| plugin, or hook registration. | ||
|
|
There was a problem hiding this comment.
We should keep one solution here the one you are suggesting and move the others to alternative section.
| `default_get_build_system_dependencies`. Fromager would automatically | ||
| scan `setup.py` for `pkg_resources` imports and `dry_run` keyword | ||
| arguments, and append the appropriate setuptools version cap to the | ||
| build dependencies. |
There was a problem hiding this comment.
I understand setuptools are an special use-case but having a generic solution which can be used to solve similar problems would be something I would prefer.
| - Register it as an entry point under `fromager.hooks` | ||
| - Delete the 22 identical per-package plugins and their entry points | ||
| - The packages still need to be listed in the downstream project's | ||
| requirements/collections |
There was a problem hiding this comment.
This sounds good to me. I agree whether we want to release the plugin as standalone installable package is a choice.
Document the motivation, design, hook signature, chaining behavior, execution order, and interaction with existing mechanisms for the new get_build_system_dependencies global hook point. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Vikash Shaw <vshaw@redhat.com>
89eee4c to
dab3621
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/proposals/global-build-system-dependencies-hook.md`:
- Around line 110-113: Update the hook execution-order section to define a
deterministic Fromager-controlled order for chained hooks, rather than relying
on stevedore HookManager discovery order. If no ordering can be guaranteed,
explicitly require hooks to be order-independent and add coverage for multiple
hooks whose outputs are chained. Remove or revise the existing contradictory
statement that only recommends order independence.
- Around line 12-15: Update the proposal’s description of the global
get_build_system_dependencies hook to state that it applies only to packages
entering the source-build dependency-resolution path, not every package;
preserve the intended post-processing behavior for build-system dependency
lists.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 506feab4-f97c-4fb7-ba66-268444abb215
📒 Files selected for processing (1)
docs/proposals/global-build-system-dependencies-hook.md
| This proposal suggests adding `get_build_system_dependencies` as a new | ||
| global hook point under `fromager.hooks`, so that downstream projects | ||
| can register hooks to post-process the build-system dependencies list | ||
| for all packages without needing per-package plugins. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Limit the documented hook scope.
src/fromager/bootstrapper/_prepare_source.py returns ProcessInstallDeps for prebuilt wheels before it calls dependencies.get_build_system_dependencies(). The proposed hook therefore does not run for every package. State that it applies to packages that enter the source-build dependency-resolution path.
Suggested wording
- for all packages without needing per-package plugins.
+ for all packages that require source-build dependency resolution without
+ needing per-package plugins.As per path instructions, this is a factual scope correction, not a formatting suggestion.
📝 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.
| This proposal suggests adding `get_build_system_dependencies` as a new | |
| global hook point under `fromager.hooks`, so that downstream projects | |
| can register hooks to post-process the build-system dependencies list | |
| for all packages without needing per-package plugins. | |
| This proposal suggests adding `get_build_system_dependencies` as a new | |
| global hook point under `fromager.hooks`, so that downstream projects | |
| can register hooks to post-process the build-system dependencies list | |
| for all packages that require source-build dependency resolution without | |
| needing per-package plugins. |
🤖 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 `@docs/proposals/global-build-system-dependencies-hook.md` around lines 12 -
15, Update the proposal’s description of the global
get_build_system_dependencies hook to state that it applies only to packages
entering the source-build dependency-resolution path, not every package;
preserve the intended post-processing behavior for build-system dependency
lists.
Source: Path instructions
| Hook execution order follows stevedore's `HookManager` discovery order. | ||
| Because hooks chain, the order can matter if two hooks modify the same | ||
| requirement. Hooks should be designed to be order-independent where | ||
| possible (e.g., appending constraints rather than replacing entries). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
Define a deterministic order for chained hooks.
The proposal documents stevedore HookManager discovery order. The supplied loader only discovers extensions; it does not establish a stable order. Because each hook receives the previous output, discovery-order changes can change requirements. Define a Fromager-controlled order or require order-independent hooks and test multiple-hook behavior. This repeats the previous ordering finding, which remains in the current text.
As per path instructions, this addresses a dependency-resolution contract, not a style issue.
🤖 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 `@docs/proposals/global-build-system-dependencies-hook.md` around lines 110 -
113, Update the hook execution-order section to define a deterministic
Fromager-controlled order for chained hooks, rather than relying on stevedore
HookManager discovery order. If no ordering can be guaranteed, explicitly
require hooks to be order-independent and add coverage for multiple hooks whose
outputs are chained. Remove or revise the existing contradictory statement that
only recommends order independence.
Source: Path instructions
|
@rd4398 @LalatenduMohanty I would like you to seriously consider and go through https://github.com/python-wheel-build/fromager/pull/1272/changes#diff-eac3b67ca88f82d425680bb6df4a0cecc7502118fe351a2498c35fca62eb74abR173 and consider this as well |
| A minimal hook that appends a constraint: | ||
|
|
||
| ```python | ||
| def get_build_system_dependencies( |
There was a problem hiding this comment.
@rd4398 You are absolutely correct.
The stevedore hooks are event listener hooks. They are designed to act on events like "wheel is ready". The hooks are registered and enabled at installation time of a package. The hooks are not suited to change behavior.
4 participants
Summary
Adds a proposal document for extending
fromager.hookswith aget_build_system_dependenciesglobal hook point, allowing downstream projects to post-process build-system dependencies for all packages without per-package plugins.The implementation is in a separate PR: #1271
Related issue: #1263