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.

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

Enable PEP 517 builds for unnamed requirements #2600

Merged
merged 5 commits into from
Mar 22, 2024
Merged

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Mar 21, 2024

Summary

This PR enables the source distribution database to be used with unnamed requirements (i.e., URLs without a package name). The (significant) upside here is that we can now use PEP 517 hooks to resolve unnamed requirement metadata and reuse any computation in the cache.

The changes to crates/uv-distribution/src/source/mod.rs are quite extensive, but mostly mechanical. The core idea is that we introduce a new BuildableSource abstraction, which can either be a distribution, or an unnamed URL:

/// A reference to a source that can be built into a built distribution.
///
/// This can either be a distribution (e.g., a package on a registry) or a direct URL.
///
/// Distributions can _also_ point to URLs in lieu of a registry; however, the primary distinction
/// here is that a distribution will always include a package name, while a URL will not.
#[derive(Debug, Clone, Copy)]
pub enum BuildableSource<'a> {
    Dist(&'a SourceDist),
    Url(SourceUrl<'a>),
}

All the methods on the source distribution database now accept BuildableSource. BuildableSource has a name() method, but it returns Option<&PackageName>, and everything is required to work with and without a package name.

The main drawback of this approach (which isn't a terrible one) is that we can no longer include the package name in the cache. (We do continue to use the package name for registry-based distributions, since those always have a name.). The package name was included in the cache route for two reasons: (1) it's nice for debugging; and (2) we use it to power uv cache clean flask, to identify the entries that are relevant for Flask.

To solve this, I changed the uv cache clean code to look one level deeper. So, when we want to determine whether to remove the cache entry for a given URL, we now look into the directory to see if there are any wheels that match the package name. This isn't as nice, but it does work (and we have test coverage for it -- all passing).

I also considered removing the package name from the cache routes for non-registry wheels, for consistency... But, it would require a cache bump, and it didn't feel important enough to merit that.

@charliermarsh charliermarsh added the enhancement New feature or request label Mar 21, 2024
@charliermarsh charliermarsh marked this pull request as ready for review March 21, 2024 22:21
@charliermarsh
Copy link
Member Author

This doesn't actually leverage these improvements anywhere, it just enables them.

Comment on lines +288 to +289
// We can only prevent builds by name for packages with names. For editable
// packages and unnamed requirements, we can't prevent the build.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm.. this is an interesting caveat. We should probably mention this in the --only-binary docs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I don’t really see any way around it. It was also already true for editables, interestingly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could default to “false” here instead of true. That might be safer? At least for non-editables.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That actually seems better since you can always add a package name to fix it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL that pip install does not enforce --no-binary for direct URL requirements. For example, pip install "anyio @ https://files.pythonhosted.org/packages/db/4d/3970183622f0330d3c23d9b8a5f52e365e50381fd484d08e3285104333d3/anyio-4.3.0.tar.gz" --only-binary anyio --no-cache --force-reinstall works fine.

@charliermarsh charliermarsh merged commit 5d7d7dc into main Mar 22, 2024
31 checks passed
@charliermarsh charliermarsh deleted the charlie/bare-viii branch March 22, 2024 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants