[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
📝 WalkthroughWalkthroughAdded a "Fromager Enhancement Proposals" section to the Sphinx docs by creating Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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/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 resolver and source downloader can be configuration for all variants of | ||
| a package as well as re-defined for each variant. A package can be configured | ||
| as prebuilt for all variants or a variant can have a different resolver and | ||
| sources than other. |
There was a problem hiding this comment.
Suggestion: global source_defaults in settings.yaml
There is a fair chance that a lot of the mirrors share the same base URL (https://<>/mirrors/github/...), consider adding a source_defaults key:
# settings.yaml
source_defaults:
gitlab_url: https://<>/mirrors/github
Per-package configs could then use a short project path instead of repeating the full URL:
Before
source:
provider: gitlab-git
url: https://<>/mirrors/github/ROCm/aiter
After
source:
provider: gitlab-git
project: ROCm/aiter # resolved against source_defaults.gitlab_url
This would reduce repetition across many package configs and avoid typo-induced breakage.
The settings file model already loads settings.yaml adding a source_defaults field is a small extension.
Full url would still work for projects outside the default base.
There was a problem hiding this comment.
Counter argument: "explicit is better than implicit"
I prefer to have full URLs in config files. It makes it easier to understand and just copy 'n paste the URL into a browser.
| source: | ||
| # resolve on PyPI, git clone, and build as tarball | ||
| provider: pypi-git | ||
| clone_url: https://git.example/viking/viking.git |
There was a problem hiding this comment.
What's the difference between url and clone_url? (gitlab-git uses url, pypi-git uses clone_url — why different names for the same concept?
There was a problem hiding this comment.
I like self-documenting configuration. The fields contain different types of URLs with different meaning. The pypi-* provides use index_url instead of url, too. I'm going to rename the Gitlab url field to project_url.
| - The `gitlab-download` and `github-download` are similar to `gitlab-git` and | ||
| `github-git` profiles. Instead of cloning a git repository, they download | ||
| a git tarball or an release artifact. | ||
|
|
There was a problem hiding this comment.
I am wondering if we should defines what fields each profile accepts. Without a schema,
there's no validation. A user could put clone_url on a pypi-sdist profile and get no error until runtime
There was a problem hiding this comment.
See "How" section of the proposal:
Each use case is handled a provider profile. The profile name acts as a tag (discriminated union). Each use case has a well-defined set of mandatory and optional arguments.
|
|
||
| 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?
| The default matcher factory parsed the tag with `packaging.version.Version` | ||
| and ignores any error. Fromager will provide additional matcher factories for | ||
| common tag patterns like `v1.2`, `1.2`, and `v1.2-stable`. | ||
|
|
There was a problem hiding this comment.
We shuld add a short list of built-in factories with their import strings and what they match:
For example:
fromager.matchers:pep440 (default) — parses any PEP 440 version
fromager.matchers:v_prefix — matches v1.2.3, strips the v
fromager.matchers:v_prefix_stable — matches v1.2.3-stable, strips v and -stable
There was a problem hiding this comment.
FYI, v1.2.3 is a valid PEP 440 version number.
|
@tiran The proposal looks good. We should merge it asap. |
tiran
force-pushed
the
proposal-new-resolver-config
branch
from
37d1bea to
b32d482
Compare
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
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
| - Provide SSH transport for git. The feature can be added at a later point | ||
| when it's needed. |
There was a problem hiding this comment.
Resolve contradiction on SSH support in git profiles
The proposal currently says SSH transport is out of scope, but later says git profiles clone over https or ssh. Please make these sections consistent.
Suggested 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 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`.
4 participants
Add design proposal for new resolver configuration.
See: #937