Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Parses wheels WHEEL and METADATA files content as email messages #6616

Merged
merged 1 commit into from
Aug 25, 2024
Merged

Parses wheels WHEEL and METADATA files content as email messages #6616

merged 1 commit into from
Aug 25, 2024

Conversation

Coruscant11
Copy link
Contributor

Summary

Fixes: #6615
Currently, some packages are not installable with uv, like ziglang on Linux.
Everything is described in the issue! 😄

Test Plan

I added a unit test for the problematic use case.
I also checked that previous unit test are still running in order to ensure the backward compatibility.

@Coruscant11 Coruscant11 changed the title Parses wheel metadata files content as email header Parses wheel WHEEL and METADATA files content as email message Aug 25, 2024
@Coruscant11 Coruscant11 changed the title Parses wheel WHEEL and METADATA files content as email message Parses wheels WHEEL and METADATA files content as email message Aug 25, 2024
@Coruscant11 Coruscant11 force-pushed the fix/wheel-metadata-parsing branch from f805a2f to 3fe2744 Compare August 25, 2024 19:57
@Coruscant11 Coruscant11 changed the title Parses wheels WHEEL and METADATA files content as email message Parses wheels WHEEL and METADATA files content as email messages Aug 25, 2024
@Coruscant11 Coruscant11 mentioned this pull request Aug 25, 2024
Closed
@Coruscant11 Coruscant11 force-pushed the fix/wheel-metadata-parsing branch from 3fe2744 to f533a7f Compare August 25, 2024 19:59
@charliermarsh charliermarsh self-assigned this Aug 25, 2024
@charliermarsh
Copy link
Member

Thanks for investigating this. It seems like a reasonable change to me.

@charliermarsh charliermarsh added the bug Something isn't working label Aug 25, 2024
@charliermarsh
Copy link
Member

Is it possible to use mailparse instead? Or change our uses of mailparse to use mail-parser? We already have this dependency elsewhere.

@Coruscant11
Copy link
Contributor Author

Is it possible to use mailparse instead? Or change our uses of mailparse to use mail-parser? We already have this dependency elsewhere.

Of course! Let me fix this quickly 😄
Thanks for the fast review!

@charliermarsh
Copy link
Member

(It might be easiest to use mailparse to start, and then consider migrating to mail-parser in a separate PR. I honestly can't remember why we use mailparse -- I think it's just a smaller dependency -- but I'm open to switching. Take a look at crates/pypi-types/src/metadata.rs for an example of how we use it today... I think it's the same problem, roughly?)

@Coruscant11 Coruscant11 force-pushed the fix/wheel-metadata-parsing branch from f533a7f to 0cf6f46 Compare August 25, 2024 20:25
@Coruscant11
Copy link
Contributor Author

Coruscant11 commented Aug 25, 2024 •
edited
Loading

(It might be easiest to use mailparse to start, and then consider migrating to mail-parser in a separate PR. I honestly can't remember why we use mailparse -- I think it's just a smaller dependency -- but I'm open to switching. Take a look at crates/pypi-types/src/metadata.rs for an example of how we use it today... I think it's the same problem, roughly?)

Alright! I volunteer to look into switching from mailparse to mail-parser in a future PR if you want!
But I will also look if a change is really needed in a first place, mailparse seems much more downloaded 😄
But maybe mail-parser is faster, and seems to be heavily tested and benchmarked.

@Coruscant11 Coruscant11 force-pushed the fix/wheel-metadata-parsing branch 2 times, most recently from 418742a to 96fc2d8 Compare August 25, 2024 20:30
@charliermarsh
Copy link
Member

It might be fun to run a small benchmark (see, e.g., crates/bench/benches/distribution_filename.rs) to compare the two on some representative files :)

@Coruscant11
Copy link
Contributor Author

Coruscant11 commented Aug 25, 2024 •
edited
Loading

It might be fun to run a small benchmark (see, e.g., crates/bench/benches/distribution_filename.rs) to compare the two on some representative files :)

Thank for the link! Will use that.
I will also try to benchmark the two crates in different machines, I have x86_64 and arm64 computers, Intel, AMD, M3 Pro... can be fun!

If I could just trust the running time of the unit test inside install-wheel-rs, mail-parser seems around 0.008 seconds faster 😆
Will try to check this more in details in a next PR then 😄

@charliermarsh
Copy link
Member

Thanks, this is much appreciated!

konstin
konstin approved these changes Aug 25, 2024
View reviewed changes
crates/install-wheel-rs/Cargo.toml Outdated Show resolved Hide resolved
@charliermarsh charliermarsh added the compatibility Compatibility with a specification or another tool label Aug 25, 2024
@charliermarsh charliermarsh enabled auto-merge (squash) August 25, 2024 20:51
auto-merge was automatically disabled August 25, 2024 20:55

Head branch was pushed to by a user without write access

@Coruscant11 Coruscant11 force-pushed the fix/wheel-metadata-parsing branch 4 times, most recently from d18d139 to f6f5bb3 Compare August 25, 2024 21:14
@Coruscant11
Parses wheels WHEEL and METADATA files as email messages
8529d22 
Fixing some wheel parsing crashes, like for the ziglang package on
Linux.
@Coruscant11 Coruscant11 force-pushed the fix/wheel-metadata-parsing branch from d4bcc49 to 8529d22 Compare August 25, 2024 21:23
@charliermarsh charliermarsh merged commit 2bfc450 into astral-sh:main Aug 25, 2024
57 checks passed
@charliermarsh
Copy link
Member

Looks good, thanks!

@BrewTestBot BrewTestBot mentioned this pull request Aug 26, 2024
Merged
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Aug 28, 2024
@tmeijn
build(deps): update astral-sh/uv to v0.3.5
2b6d5d6 
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.3.2` -> `0.3.5` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>astral-sh/uv (astral-sh/uv)</summary>

### [`v0.3.5`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#035)

[Compare Source](astral-sh/uv@0.3.4...0.3.5)

##### Enhancements

-   Add support for `--allow-insecure-host` (aliased to `--trusted-host`) ([#&#8203;6591](astral-sh/uv#6591))
-   Read requirements from `requires.txt` when available ([#&#8203;6655](astral-sh/uv#6655))
-   Respect `tool.uv.environments` in `pip compile --universal` ([#&#8203;6663](astral-sh/uv#6663))
-   Use relative paths by default in `uv add` ([#&#8203;6686](astral-sh/uv#6686))
-   Improve messages for empty solves and installs ([#&#8203;6588](astral-sh/uv#6588))

##### Bug fixes

-   Avoid reusing state across tool upgrades ([#&#8203;6660](astral-sh/uv#6660))
-   Detect musl and error for musl Python builds ([#&#8203;6643](astral-sh/uv#6643))
-   Ignore `send` errors in installer ([#&#8203;6667](astral-sh/uv#6667))

##### Documentation

-   Add development section to Docker guide and reference new example project ([#&#8203;6666](astral-sh/uv#6666))
-   Add docs for `constraint-dependencies` and `override-dependencies` ([#&#8203;6596](astral-sh/uv#6596))
-   Clarify package priority order in pip compatibility guide ([#&#8203;6619](astral-sh/uv#6619))
-   Fix docs for disabling build isolation with `uv sync` ([#&#8203;6674](astral-sh/uv#6674))
-   Improve consistency of directory lookup instructions in Docker ([#&#8203;6665](astral-sh/uv#6665))
-   Improve lockfile concept documentation, add coverage for upgrades ([#&#8203;6698](astral-sh/uv#6698))
-   Shift the order of some of the Docker guide content ([#&#8203;6664](astral-sh/uv#6664))
-   Use `python` to highlight requirements and use more content tabs ([#&#8203;6549](astral-sh/uv#6549))

### [`v0.3.4`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#034)

[Compare Source](astral-sh/uv@0.3.3...0.3.4)

##### CLI

-   Show `--editable` on the `uv add` CLI ([#&#8203;6608](astral-sh/uv#6608))
-   Add `--refresh` to `tool run` warning for `--with` dependencies ([#&#8203;6609](astral-sh/uv#6609))

##### Bug fixes

-   Allow per dependency build isolation for `setup.py`-based projects ([#&#8203;6517](astral-sh/uv#6517))
-   Avoid un-strict syncing by-default for build isolation ([#&#8203;6606](astral-sh/uv#6606))
-   Respect `--no-build-isolation-package` in `uv sync` ([#&#8203;6605](astral-sh/uv#6605))
-   Respect extras and markers on virtual dev dependencies ([#&#8203;6620](astral-sh/uv#6620))
-   Support PEP 723 scripts in GUI files ([#&#8203;6611](astral-sh/uv#6611))
-   Update lockfile after setting minimum bounds in `uv add` ([#&#8203;6618](astral-sh/uv#6618))
-   Use relative paths for `--find-links` and local registries ([#&#8203;6566](astral-sh/uv#6566))
-   Use separate types to represent raw vs. resolver markers ([#&#8203;6646](astral-sh/uv#6646))
-   Parse wheels `WHEEL` and `METADATA` files as email messages ([#&#8203;6616](astral-sh/uv#6616))
-   Support unquoted hrefs in `--find-links` and other HTML sources ([#&#8203;6622](astral-sh/uv#6622))
-   Don't canonicalize paths to user requirements ([#&#8203;6560](astral-sh/uv#6560))

##### Documentation

-   Add FastAPI guide to overview ([#&#8203;6603](astral-sh/uv#6603))
-   Add docs for disabling build isolation with `uv sync` ([#&#8203;6607](astral-sh/uv#6607))
-   Add example of reading script from stdin using echo ([#&#8203;6567](astral-sh/uv#6567))
-   Add tip to use intermediate layers in Docker builds ([#&#8203;6650](astral-sh/uv#6650))
-   Clarify need to include `pyproject.toml` with `--no-install-project` ([#&#8203;6581](astral-sh/uv#6581))
-   Move `WORKDIR` directive in Docker examples ([#&#8203;6652](astral-sh/uv#6652))
-   Remove duplicate `WORKDIR` directive in Docker example ([#&#8203;6651](astral-sh/uv#6651))

### [`v0.3.3`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#033)

[Compare Source](astral-sh/uv@0.3.2...0.3.3)

##### Enhancements

-   Add `uv sync --no-install-project` to skip installation of the project ([#&#8203;6538](astral-sh/uv#6538))
-   Add `uv sync --no-install-workspace` to skip installation of all workspace members ([#&#8203;6539](astral-sh/uv#6539))
-   Add `uv sync --no-install-package` to skip installation of specific packages ([#&#8203;6540](astral-sh/uv#6540))
-   Show previous version in self update message ([#&#8203;6473](astral-sh/uv#6473))

##### CLI

-   Add `--no-project` alias for `uv python pin --no-workspace` ([#&#8203;6514](astral-sh/uv#6514))
-   Ignore `.python-version` files in `uv venv` with `--no-config` ([#&#8203;6513](astral-sh/uv#6513))
-   Include virtual environment interpreters in `uv python find` ([#&#8203;6521](astral-sh/uv#6521))
-   Respect `-` as stdin channel for `uv run` ([#&#8203;6481](astral-sh/uv#6481))
-   Revert changes to pyproject.toml when sync fails duing `uv add` ([#&#8203;6526](astral-sh/uv#6526))

##### Configuration

-   Add `UV_COMPILE_BYTECODE` environment variable ([#&#8203;6530](astral-sh/uv#6530))

##### Bug fixes

-   Set `VIRTUAL_ENV` for `uv run` invocations ([#&#8203;6543](astral-sh/uv#6543))
-   Ignore errors in workspace discovery with `--no-project` ([#&#8203;6554](astral-sh/uv#6554))

##### Documentation

-   Add documentation for `uv python find` ([#&#8203;6527](astral-sh/uv#6527))
-   Add uv tool install example in Docker ([#&#8203;6547](astral-sh/uv#6547))
-   Document why we do lower bounds ([#&#8203;6516](astral-sh/uv#6516))
-   Fix to miss string termination in PowerShell commands for shell autocompletion documentation ([#&#8203;6491](astral-sh/uv#6491))
-   Fix incorrect workspace members keyword ([#&#8203;6502](astral-sh/uv#6502))
-   Use proper environment variables for Windows ([#&#8203;6433](astral-sh/uv#6433))
-   Improve caveat in `uvx` note ([#&#8203;6546](astral-sh/uv#6546))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
@Coruscant11 Coruscant11 mentioned this pull request Sep 28, 2024
Merged
charliermarsh pushed a commit that referenced this pull request Sep 29, 2024
@Coruscant11
Correctly trims values during wheel WHEEL file parsing (#7770)
ada6b36 
## Summary

My last changes (#6616) used by mistake == instead of !=.
:disappointed_relieved: Making values currently never trimmed despite
what we wanted.
Values should now be trimmed if needed.

Also removes the trim of the header name, because if a header contains
spaces, the header will be skipped by the mailparse crate in the first
place.

## Test Plan
- A unit test has been added to validate that we correctly trim values.
- A unit test has been added to validate the header names containing
spaces are skipped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@konstin konstin konstin approved these changes

@charliermarsh charliermarsh charliermarsh approved these changes

Assignees

@charliermarsh charliermarsh

Labels
bug Something isn't working compatibility Compatibility with a specification or another tool
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Some packages can not be installed with uv contrary to pip, like ziglang
3 participants
@Coruscant11 @charliermarsh @konstin