[proposal] New resolver configuration#938
[proposal] New resolver configuration#938tiran wants to merge 1 commit intopython-wheel-build:mainfrom
Conversation
tiran
force-pushed
the
proposal-new-resolver-config
branch
3 times, most recently
from
ceb5b48 to
071d46c
Compare
|
@shifa-khan has been looking at one of the other tickets related to similar work. |
|
I have inspect our downstream repo. Most
|
tiran
force-pushed
the
proposal-new-resolver-config
branch
from
071d46c to
3356160
Compare
|
downstream has few plugins call |
tiran
left a comment
tiran
left a comment
There was a problem hiding this comment.
downstream has few plugins call ensure_pkg_info() in prepare_source as boilerplate. Git clones never have PKG-INFO, but build tools (like setuptools-scm) need it before building. Could the git-based providers (gitlab-git, github-git, pypi-git) handle this automatically?
Great point! Yes, this will be handled automatically in two ways:
- Fromager already adds
PKG-INFOto the final sdist. In some cases, the file is added too late for some PEP 517 hooks. - Fromager will add a
.git_archival.txtfile to clones. Lala is working on the feature in #961 / #962 . This works for all setuptools-scm cases.
I'll update the design with the PKG-INFO and .git_archival.txt details.
tiran
force-pushed
the
proposal-new-resolver-config
branch
from
3356160 to
6559cd3
Compare
|
@Mergifyio rebase |
✅ Branch has been successfully rebased |
tiran
force-pushed
the
proposal-new-resolver-config
branch
from
6559cd3 to
37d1bea
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new proposal document, docs/proposals/new-resolver-config.md, and registers it in docs/proposals/index.rst. The new markdown defines a declarative top-level Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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/proposals/new-resolver-config.md`:
- Line 9: Replace the incorrect verb in the sentence "This enhancement document
proposal a new approach to configure the package" by changing "proposal" to
"proposes" so the sentence reads "This enhancement document proposes a new
approach to configure the package"; update the single sentence in the
docs/proposals/new-resolver-config.md content where that exact phrase appears.
- Line 187: Replace the lowercase platform name "github" with the capitalized
"GitHub" in the phrase referring to the `release_artifact` parameter of the
`github` or `gitlab` provider so the documentation consistently uses the
official name; update the occurrence that mentions "github" (in the context of
`release_artifact` and provider descriptions) to "GitHub" while leaving code
identifiers/backticks intact if they must remain lowercase.
- Line 169: The comment contains a duplicated word "must" in the text "tag must
must v1.2+midstream.1.cpu"; edit that comment to remove the duplicate so it
reads "tag must v1.2+midstream.1.cpu" (preserve the rest of the comment and the
Version("1.2+midstream.1") example).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26a9cf54-1ff6-456b-89d4-e20a9f61d2f0
📒 Files selected for processing (3)
docs/index.rstdocs/proposals/index.rstdocs/proposals/new-resolver-config.md
| ### Deprecations | ||
|
|
||
| - `download_source.url` is handled by `pypi-download` profile or | ||
| `release_artifact` parameter of `github` or `gitlab` provider |
There was a problem hiding this comment.
Capitalize "GitHub" for consistency.
The official platform name should be capitalized as "GitHub" to match its usage elsewhere in the document.
📝 Proposed fix
- `release_artifact` parameter of `github` or `gitlab` provider
+ `release_artifact` parameter of `GitHub` or `GitLab` provider📝 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.
| `release_artifact` parameter of `github` or `gitlab` provider | |
| `release_artifact` parameter of `GitHub` or `GitLab` provider |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~187-~187: The official name of this software platform is spelled with a capital “H”.
Context: ...le or release_artifact parameter of github or gitlab provider - `download_sourc...
(GITHUB)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/proposals/new-resolver-config.md` at line 187, Replace the lowercase
platform name "github" with the capitalized "GitHub" in the phrase referring to
the `release_artifact` parameter of the `github` or `gitlab` provider so the
documentation consistently uses the official name; update the occurrence that
mentions "github" (in the context of `release_artifact` and provider
descriptions) to "GitHub" while leaving code identifiers/backticks intact if
they must remain lowercase.
|
|
||
| The new system will use a new top-level configuration key `source`. The old | ||
| `download_source` and `resolver_dist` settings will stay supported for a | ||
| while. Eventually the old options will be deprecated and removed. |
There was a problem hiding this comment.
We need to clarify where source config enters the dispatch chain. For example
If someone adds to the YAML:
source:
provider: gitlab-git
url: https://gitlab.com/.../detectron2
But forgets to delete detectron2.py. Now what?
- Does the plugin win? (User thinks they migrated but the old code still runs silently)
- Does source config win? (Plugin is ignored without warning )
- Is it an error?
There was a problem hiding this comment.
That's TBD. What do you recommend?
There was a problem hiding this comment.
I think we should go for source: config wins, skip hooks (with warning)
source: config should take precedence over plugin hooks for get_resolver_provider and download_source only. Other hooks (prepare_source, build_wheel, update_extra_environ) are unaffected.
There was a problem hiding this comment.
With this this is how the migration plan looks like
- Step 1: Add source: to YAML — resolver/download switches to config. Warning logged if old plugin functions still exist.
- Step 2: Delete the old get_resolver_provider/download_source functions from the plugin file. Warning disappears.
- Step 3 (optional): If no hooks remain, delete the entire plugin file and entry point.
There was a problem hiding this comment.
I have added a paragraph and a new profile for hooks.
|
@tiran The proposal looks good. We should merge it asap. |
tiran
force-pushed
the
proposal-new-resolver-config
branch
2 times, most recently
from
b32d482 to
45fd388
Compare
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 `@docs/proposals/new-resolver-config.md`:
- Around line 49-50: The doc currently contradicts itself by saying "Provide SSH
transport for git" is out of scope while elsewhere stating git profiles clone
over `https` or `ssh`; update both occurrences (the "Provide SSH transport for
git" sentence and the phrase "git profiles clone over `https` or `ssh`") to be
consistent: either explicitly state SSH is out of scope everywhere (e.g.,
replace "`ssh`" with "`https`" and add a note that SSH support is future work)
or explicitly include SSH as supported and remove the out-of-scope sentence;
ensure the same wording appears at both the earlier mention and the later lines
currently referencing `https` or `ssh`.
🪄 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: a3439f13-0234-4da4-b356-bffb61612fd1
📒 Files selected for processing (3)
docs/index.rstdocs/proposals/index.rstdocs/proposals/new-resolver-config.md
✅ Files skipped from review due to trivial changes (2)
- docs/index.rst
- docs/proposals/index.rst
tiran
force-pushed
the
proposal-new-resolver-config
branch
from
45fd388 to
8d12612
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/proposals/new-resolver-config.md (1)
49-50:⚠️ Potential issue | 🟠 MajorResolve the SSH support contradiction in the proposal
The doc says SSH is out of scope, but later says git profiles clone over
httpsorssh. Please keep one consistent behavior statement.Proposed doc fix
-- The `gitlab-tag-git` and `github-tag-git` profiles use the - `GitLabTagProvider` or `GitHubTagProvider` to resolve versions. The - profiles git clone a project over `https` or `ssh` protocol. +- The `gitlab-tag-git` and `github-tag-git` profiles use the + `GitLabTagProvider` or `GitHubTagProvider` to resolve versions. The + profiles git clone a project over `https` protocol.As per coding guidelines, "
**/*.md: Only comment on factual errors or broken links."Also applies to: 138-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/proposals/new-resolver-config.md` around lines 49 - 50, The proposal currently contradicts itself about SSH support: one sentence says "Provide SSH transport for git" (SSH out of scope) while another states "git profiles clone over `https` or `ssh`"; pick a single consistent stance and update all occurrences to match it (either remove mention of SSH everywhere and state clones use HTTPS only, or explicitly include SSH support and clarify scope/limitations). Search for the exact phrases "Provide SSH transport for git" and "git profiles clone over `https` or `ssh`" (also the similar block at the later occurrence) and edit them so the document consistently describes the chosen behavior, including any notes about planned future work if SSH is deferred.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@docs/proposals/new-resolver-config.md`:
- Around line 49-50: The proposal currently contradicts itself about SSH
support: one sentence says "Provide SSH transport for git" (SSH out of scope)
while another states "git profiles clone over `https` or `ssh`"; pick a single
consistent stance and update all occurrences to match it (either remove mention
of SSH everywhere and state clones use HTTPS only, or explicitly include SSH
support and clarify scope/limitations). Search for the exact phrases "Provide
SSH transport for git" and "git profiles clone over `https` or `ssh`" (also the
similar block at the later occurrence) and edit them so the document
consistently describes the chosen behavior, including any notes about planned
future work if SSH is deferred.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4a23137c-f8fd-4964-9a9a-a72959dfec59
📒 Files selected for processing (2)
docs/proposals/index.rstdocs/proposals/new-resolver-config.md
✅ Files skipped from review due to trivial changes (1)
- docs/proposals/index.rst
tiran
force-pushed
the
proposal-new-resolver-config
branch
from
8d12612 to
02859ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/proposals/new-resolver-config.md (1)
138-140:⚠️ Potential issue | 🟠 MajorResolve SSH support contradiction in git-tag profiles
This section states git clones may use
ssh, but Line 49–50 explicitly says SSH transport is out of scope. Please align both sections to one behavior.Suggested fix
-- The `gitlab-tag-git` and `github-tag-git` profiles use the - `GitLabTagProvider` or `GitHubTagProvider` to resolve versions. The - profiles git clone a project over `https` or `ssh` protocol. +- The `gitlab-tag-git` and `github-tag-git` profiles use the + `GitLabTagProvider` or `GitHubTagProvider` to resolve versions. The + profiles git clone a project over `https` protocol.As per coding guidelines "
**/*.md: Only comment on factual errors or broken links."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/proposals/new-resolver-config.md` around lines 138 - 140, The doc currently contradicts itself about SSH support for git-tag-git profiles: pick a single behavior (either "SSH transport is supported" or "SSH transport is out of scope") and make the text consistent; update the mentions of the git-tag-git profiles and the provider descriptions (GitLabTagProvider and GitHubTagProvider) so they all state the same transport policy, remove or change the phrase that says "git clone ... over `https` or `ssh`" to match the chosen policy, and update the other sentence that currently declares SSH out of scope to reflect that same decision.
🤖 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/proposals/new-resolver-config.md`:
- Around line 146-147: Update the documentation to use the correct hook name:
replace the incorrect symbol "resolver_provider" with the actual dispatched hook
"get_resolver_provider" wherever it appears in the hooks profile text (including
the occurrences noted around the `hooks` profile and the `download_source`
hooks). Ensure the sentence reads that the `hooks` profile calls the
`get_resolver_provider` and `download_source` hooks, and verify the three
occurrences mentioned (around lines referenced) are all changed to
`get_resolver_provider`.
- Around line 160-161: The text incorrectly requires "profile: hooks" for a
source setting while the document uses "provider" as the discriminator; update
the sentence so it requires at least one `source` setting (top-level or variant)
to use `provider: hooks` instead of `profile: hooks`, and ensure any surrounding
examples or mentions that reference `profile: hooks` are changed to use the
`provider` discriminator consistently with the schema described (look for
occurrences of "source" setting, the phrase "profile: hooks", and the "provider"
discriminator).
---
Duplicate comments:
In `@docs/proposals/new-resolver-config.md`:
- Around line 138-140: The doc currently contradicts itself about SSH support
for git-tag-git profiles: pick a single behavior (either "SSH transport is
supported" or "SSH transport is out of scope") and make the text consistent;
update the mentions of the git-tag-git profiles and the provider descriptions
(GitLabTagProvider and GitHubTagProvider) so they all state the same transport
policy, remove or change the phrase that says "git clone ... over `https` or
`ssh`" to match the chosen policy, and update the other sentence that currently
declares SSH out of scope to reflect that same decision.
🪄 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: 4ca9ff78-2638-438f-9761-8658f76f0652
📒 Files selected for processing (2)
docs/proposals/index.rstdocs/proposals/new-resolver-config.md
✅ Files skipped from review due to trivial changes (1)
- docs/proposals/index.rst
| - The `hooks` profile calls the `resolver_provider` and `download_source` | ||
| [hooks](../reference/hooks.rst). |
There was a problem hiding this comment.
Use the correct hook name: get_resolver_provider
The document references resolver_provider, but current hook dispatch uses get_resolver_provider. This is a factual mismatch with the existing API and can mislead migration.
Suggested fix
-- The `hooks` profile calls the `resolver_provider` and `download_source`
+- The `hooks` profile calls the `get_resolver_provider` and `download_source`
[hooks](../reference/hooks.rst).
...
-then Fromager keep its old behavior. It first looks for `resolver_provider`
+then Fromager keep its old behavior. It first looks for `get_resolver_provider`
and `download_source` [hooks](../reference/hooks.rst), then looks for source
distributions on PyPI.
...
-When a package has a plugin with a `resolver_provider` or `download_source`
+When a package has a plugin with a `get_resolver_provider` or `download_source`As per coding guidelines "**/*.md: Only comment on factual errors or broken links."
Also applies to: 155-156, 159-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/proposals/new-resolver-config.md` around lines 146 - 147, Update the
documentation to use the correct hook name: replace the incorrect symbol
"resolver_provider" with the actual dispatched hook "get_resolver_provider"
wherever it appears in the hooks profile text (including the occurrences noted
around the `hooks` profile and the `download_source` hooks). Ensure the sentence
reads that the `hooks` profile calls the `get_resolver_provider` and
`download_source` hooks, and verify the three occurrences mentioned (around
lines referenced) are all changed to `get_resolver_provider`.
| hook and `source` settings, then at least on `source` setting (top-level or | ||
| variant) must use `profile: hooks`. The rule ensures that the hooks are |
There was a problem hiding this comment.
source discriminator key should be provider, not profile
The proposal defines discriminated profiles via provider, but this line requires profile: hooks. That is internally inconsistent with the schema described in the same document.
Suggested fix
-hook and `source` settings, then at least on `source` setting (top-level or
-variant) must use `profile: hooks`. The rule ensures that the hooks are
+hook and `source` settings, then at least one `source` setting (top-level or
+variant) must use `provider: hooks`. The rule ensures that the hooks are
used.As per coding guidelines "**/*.md: Only comment on factual errors or broken links."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/proposals/new-resolver-config.md` around lines 160 - 161, The text
incorrectly requires "profile: hooks" for a source setting while the document
uses "provider" as the discriminator; update the sentence so it requires at
least one `source` setting (top-level or variant) to use `provider: hooks`
instead of `profile: hooks`, and ensure any surrounding examples or mentions
that reference `profile: hooks` are changed to use the `provider` discriminator
consistently with the schema described (look for occurrences of "source"
setting, the phrase "profile: hooks", and the "provider" discriminator).
Add design proposal for new resolver configuration. See: python-wheel-build#937 Signed-off-by: Christian Heimes <cheimes@redhat.com>
tiran
force-pushed
the
proposal-new-resolver-config
branch
from
02859ee to
aeac1f7
Compare
4 participants
Add design proposal for new resolver configuration.
See: #937