-
-
Notifications
You must be signed in to change notification settings - Fork 277
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Match dependency readmes by package id #1915
Merged
Merged
Conversation
This file contains 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
✅ Deploy Preview for maturin-guide ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
We have a project which depends on a vendored version of `pep440_rs`, while some of our dependencies depend on the crates.io version. The published version uses `Readme.md`, while the vendored one follows a different convention and uses `README.md`. This causes `maturin sdist` to fail: ``` 🍹 Building a mixed python/rust project 🔗 Found bin bindings 📡 Using build options bindings from pyproject.toml 💥 maturin failed Caused by: Failed to build source distribution Caused by: failed to normalize path `Readme.md` Caused by: No such file or directory (os error 2) ``` Internally, maturin uses the package name to match dependencies with their resolved readme. This failed in our cased since the second entry for `pep440_rs` with the crates.io readme was overwriting the first entry with the project local readme. The fix is to match by package id instead. The implementation is somewhat awkward, since there's no API to get the package id directly, so we again have to get list of dependency package ids and dependency `Dependency`s and match them by name, assuming in a single `[dependency]` each package name must be unique. <details> <summary>Relevant `cargo metadata` excerpt</summary> ```javascript [ { "name": "pep440_rs", "version": "0.3.12", "id": "pep440_rs 0.3.12 (path+file:///home/konsti/projects/internal/crates/pep440-rs)", "license": "Apache-2.0 OR BSD-2-Clause", "license_file": null, "description": "A library for python version numbers and specifiers, implementing PEP 440", "source": null, "dependencies": ["..."], "targets": ["..."], "features": { "...": [ "..." ] }, "manifest_path": "/home/konsti/projects/internal/crates/pep440-rs/Cargo.toml", "metadata": null, "publish": null, "authors": [ "Puffin" ], "categories": [], "keywords": [], "readme": "README.md", "repository": "https://github.com/astral-sh/internal", "homepage": "https://pypi.org/project/internal-alpha/", "documentation": "https://pypi.org/project/internal-alpha/", "edition": "2021", "links": null, "default_run": null, "rust_version": "1.74" }, { "name": "pep440_rs", "version": "0.3.12", "id": "pep440_rs 0.3.12 (registry+https://github.com/rust-lang/crates.io-index)", "license": "Apache-2.0 OR BSD-2-Clause", "license_file": null, "description": "A library for python version numbers and specifiers, implementing PEP 440", "source": "registry+https://github.com/rust-lang/crates.io-index", "dependencies": ["..."], "targets": ["..."], "features": { "...": [ "..." ] }, "manifest_path": "/home/konsti/.cargo/registry/src/index.crates.io-6f17d22bba15001f/pep440_rs-0.3.12/Cargo.toml", "metadata": null, "publish": null, "authors": [], "categories": [], "keywords": [], "readme": "Readme.md", "repository": "https://github.com/konstin/pep440-rs", "homepage": null, "documentation": null, "edition": "2021", "links": null, "default_run": null, "rust_version": null } ] ``` </details>
konstin
force-pushed
the
konsti/use-package-ids-to-match-readmes
branch
from
January 19, 2024 12:34
cd2ad95
to
e302648
Compare
charliermarsh
added a commit
to astral-sh/uv
that referenced
this pull request
Jan 19, 2024
messense
approved these changes
Jan 22, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
bmwiedemann
pushed a commit
to bmwiedemann/openSUSE
that referenced
this pull request
Mar 18, 2024
https://build.opensuse.org/request/show/1158801 by user mia + anag+factory - Update to 1.5.0 * tutorial: fix abi to match comment gh#PyO3/maturin#1876 * Allow identical VIRTUAL_ENV and CONDA_PREFIX env vars gh#PyO3/maturin#1879 * Upgrade pyo3 to 0.20 gh#PyO3/maturin#1881 * Skip directory when adding license files to wheel gh#PyO3/maturin#1890 * Reject -i python when cross compiling gh#PyO3/maturin#1891 * simplified clear-cache github action gh#PyO3/maturin#1897 * Support uniffi-bindgen in cargo workspaces gh#PyO3/maturin#1909 * Upgrade globlin to 0.8.0 gh#PyO3/maturin#1912 * Update **Note** to [!NOTE] in README gh#PyO3/maturin#1917 * Match dependency readmes gh#PyO3/maturin#1915 * Update some actions version in generate ci cli gh#PyO3/maturin#1916 * Use extension name as library name, instead of h
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We have a project which depends on a vendored version of
pep440_rs
, while some of our dependencies depend on the crates.io version. The published version usesReadme.md
, while the vendored one follows a different convention and usesREADME.md
. This causesmaturin sdist
to fail:Internally, maturin uses the package name to match dependencies with their resolved readme. This failed in our cased since the second entry for
pep440_rs
with the crates.io readme was overwriting the first entry with the project local readme.The fix is to match by package id instead. The implementation is somewhat awkward, since there's no API to get the package id directly (rust-lang/cargo#7289), so we have to get list of dependency package ids and dependency
Dependency
s and match them by name, assuming in a single[dependency]
each package name must be unique.I also made the error message more distinct so easier to map a failure back to its source code origin.
Relevant `cargo metadata` excerpt