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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

113 changes: 113 additions & 0 deletions crates/distribution-types/src/buildable.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
use std::path::Path;

use url::Url;

use uv_normalize::PackageName;

use crate::{GitSourceDist, Name, PathSourceDist, SourceDist};

/// 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>),
}

impl BuildableSource<'_> {
/// Return the [`PackageName`] of the source, if available.
pub fn name(&self) -> Option<&PackageName> {
match self {
Self::Dist(dist) => Some(dist.name()),
Self::Url(_) => None,
}
}

/// Return the [`BuildableSource`] as a [`SourceDist`], if it is a distribution.
pub fn as_dist(&self) -> Option<&SourceDist> {
match self {
Self::Dist(dist) => Some(dist),
Self::Url(_) => None,
}
}
}

impl std::fmt::Display for BuildableSource<'_> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Self::Dist(dist) => write!(f, "{dist}"),
Self::Url(url) => write!(f, "{url}"),
}
}
}

/// A reference to a source distribution defined by a URL.
#[derive(Debug, Clone, Copy)]
pub enum SourceUrl<'a> {
Direct(DirectSourceUrl<'a>),
Git(GitSourceUrl<'a>),
Path(PathSourceUrl<'a>),
}

impl std::fmt::Display for SourceUrl<'_> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Self::Direct(url) => write!(f, "{url}"),
Self::Git(url) => write!(f, "{url}"),
Self::Path(url) => write!(f, "{url}"),
}
}
}

#[derive(Debug, Clone, Copy)]
pub struct DirectSourceUrl<'a> {
pub url: &'a Url,
}

impl std::fmt::Display for DirectSourceUrl<'_> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{url}", url = self.url)
}
}

#[derive(Debug, Clone, Copy)]
pub struct GitSourceUrl<'a> {
pub url: &'a Url,
}

impl std::fmt::Display for GitSourceUrl<'_> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{url}", url = self.url)
}
}

impl<'a> From<&'a GitSourceDist> for GitSourceUrl<'a> {
fn from(dist: &'a GitSourceDist) -> Self {
Self { url: &dist.url }
}
}

#[derive(Debug, Clone, Copy)]
pub struct PathSourceUrl<'a> {
pub url: &'a Url,
pub path: &'a Path,
}

impl std::fmt::Display for PathSourceUrl<'_> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{url}", url = self.url)
}
}

impl<'a> From<&'a PathSourceDist> for PathSourceUrl<'a> {
fn from(dist: &'a PathSourceDist) -> Self {
Self {
url: &dist.url,
path: &dist.path,
}
}
}
2 changes: 2 additions & 0 deletions crates/distribution-types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ use pep508_rs::{Scheme, VerbatimUrl};
use uv_normalize::PackageName;

pub use crate::any::*;
pub use crate::buildable::*;
pub use crate::cached::*;
pub use crate::direct_url::*;
pub use crate::editable::*;
Expand All @@ -58,6 +59,7 @@ pub use crate::resolution::*;
pub use crate::traits::*;

mod any;
mod buildable;
mod cached;
mod direct_url;
mod editable;
Expand Down
2 changes: 2 additions & 0 deletions crates/uv-cache/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ workspace = true
[dependencies]
cache-key = { workspace = true }
distribution-types = { workspace = true }
pypi-types = { workspace = true }
uv-fs = { workspace = true }
uv-normalize = { workspace = true }

Expand All @@ -30,3 +31,4 @@ tempfile = { workspace = true }
tracing = { workspace = true }
url = { workspace = true }
walkdir = { workspace = true }
rmp-serde = { workspace = true }
45 changes: 33 additions & 12 deletions crates/uv-cache/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use tempfile::{tempdir, TempDir};
use tracing::debug;

use distribution_types::InstalledDist;
use pypi_types::Metadata23;
use uv_fs::directories;
use uv_normalize::PackageName;

Expand Down Expand Up @@ -590,7 +591,7 @@ pub enum CacheBucket {
impl CacheBucket {
fn to_str(self) -> &'static str {
match self {
Self::BuiltWheels => "built-wheels-v0",
Self::BuiltWheels => "built-wheels-v1",
Self::FlatIndex => "flat-index-v0",
Self::Git => "git-v0",
Self::Interpreter => "interpreter-v0",
Expand All @@ -604,6 +605,17 @@ impl CacheBucket {
///
/// Returns the number of entries removed from the cache.
fn remove(self, cache: &Cache, name: &PackageName) -> Result<Removal, io::Error> {
/// Returns `true` if the [`Path`] represents a built wheel for the given package.
fn is_match(path: &Path, name: &PackageName) -> bool {
let Ok(metadata) = fs_err::read(path.join("metadata.msgpack")) else {
return false;
};
let Ok(metadata) = rmp_serde::from_slice::<Metadata23>(&metadata) else {
return false;
};
metadata.name == *name
}

let mut summary = Removal::default();
match self {
Self::Wheels => {
Expand Down Expand Up @@ -637,26 +649,35 @@ impl CacheBucket {
summary += rm_rf(directory.join(name.to_string()))?;
}

// For direct URLs, we expect a directory for every index, followed by a
// directory per package (indexed by name).
// For direct URLs, we expect a directory for every URL, followed by a
// directory per version. To determine whether the URL is relevant, we need to
// search for a wheel matching the package name.
let root = cache.bucket(self).join(WheelCacheKind::Url);
for directory in directories(root) {
summary += rm_rf(directory.join(name.to_string()))?;
for url in directories(root) {
if directories(&url).any(|version| is_match(&version, name)) {
summary += rm_rf(url)?;
}
}

// For local dependencies, we expect a directory for every path, followed by a
// directory per package (indexed by name).
// directory per version. To determine whether the path is relevant, we need to
// search for a wheel matching the package name.
let root = cache.bucket(self).join(WheelCacheKind::Path);
for directory in directories(root) {
summary += rm_rf(directory.join(name.to_string()))?;
for path in directories(root) {
if directories(&path).any(|version| is_match(&version, name)) {
summary += rm_rf(path)?;
}
}

// For Git dependencies, we expect a directory for every repository, followed by a
// directory for every SHA, followed by a directory per package (indexed by name).
// directory for every SHA. To determine whether the SHA is relevant, we need to
// search for a wheel matching the package name.
let root = cache.bucket(self).join(WheelCacheKind::Git);
for directory in directories(root) {
for directory in directories(directory) {
summary += rm_rf(directory.join(name.to_string()))?;
for repository in directories(root) {
for sha in directories(repository) {
if is_match(&sha, name) {
summary += rm_rf(sha)?;
}
}
}
}
Expand Down
20 changes: 5 additions & 15 deletions crates/uv-cache/src/wheel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,7 @@ use url::Url;
use cache_key::{digest, CanonicalUrl};
use distribution_types::IndexUrl;

#[allow(unused_imports)] // For rustdoc
use crate::CacheBucket;

/// Cache wheels and their metadata, both from remote wheels and built from source distributions.
///
/// Use [`WheelCache::remote_wheel_dir`] for remote wheel metadata caching and
/// [`WheelCache::built_wheel_dir`] for built source distributions metadata caching.
#[derive(Debug, Clone)]
pub enum WheelCache<'a> {
/// Either PyPI or an alternative index, which we key by index URL.
Expand All @@ -28,7 +22,8 @@ pub enum WheelCache<'a> {
}

impl<'a> WheelCache<'a> {
fn bucket(&self) -> PathBuf {
/// The root directory for a cache bucket.
pub fn root(&self) -> PathBuf {
match self {
WheelCache::Index(IndexUrl::Pypi(_)) => WheelCacheKind::Pypi.root(),
WheelCache::Index(url) => WheelCacheKind::Index
Expand All @@ -47,14 +42,9 @@ impl<'a> WheelCache<'a> {
}
}

/// Metadata of a remote wheel. See [`CacheBucket::Wheels`]
pub fn remote_wheel_dir(&self, package_name: impl AsRef<Path>) -> PathBuf {
self.bucket().join(package_name)
}

/// Metadata of a built source distribution. See [`CacheBucket::BuiltWheels`]
pub fn built_wheel_dir(&self, filename: impl AsRef<Path>) -> PathBuf {
self.bucket().join(filename)
/// A subdirectory in a bucket for wheels for a specific package.
pub fn wheel_dir(&self, package_name: impl AsRef<Path>) -> PathBuf {
self.root().join(package_name)
}
}

Expand Down
4 changes: 2 additions & 2 deletions crates/uv-client/src/registry_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,7 @@ impl RegistryClient {

let cache_entry = self.cache.entry(
CacheBucket::Wheels,
WheelCache::Index(index).remote_wheel_dir(filename.name.as_ref()),
WheelCache::Index(index).wheel_dir(filename.name.as_ref()),
format!("{}.msgpack", filename.stem()),
);
let cache_control = match self.connectivity {
Expand Down Expand Up @@ -464,7 +464,7 @@ impl RegistryClient {
) -> Result<Metadata23, Error> {
let cache_entry = self.cache.entry(
CacheBucket::Wheels,
cache_shard.remote_wheel_dir(filename.name.as_ref()),
cache_shard.wheel_dir(filename.name.as_ref()),
format!("{}.msgpack", filename.stem()),
);
let cache_control = match self.connectivity {
Expand Down
9 changes: 2 additions & 7 deletions crates/uv-dispatch/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -285,20 +285,15 @@ impl<'a> BuildContext for BuildDispatch<'a> {
),
NoBuild::None => {}
NoBuild::Packages(packages) => {
// We can only prevent builds by name for packages with names. For editable
// packages and unnamed requirements, we can't prevent the build.
Comment on lines +288 to +289
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.

if let Some(dist) = dist {
// We can only prevent builds by name for packages with names
// which is unknown before build of editable source distributions
if packages.contains(dist.name()) {
bail!(
"Building source distributions for {} is disabled",
dist.name()
);
}
} else {
debug_assert!(
matches!(build_kind, BuildKind::Editable),
"Only editable builds are exempt from 'no build' checks"
);
}
}
}
Expand Down
19 changes: 12 additions & 7 deletions crates/uv-distribution/src/distribution_database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ use url::Url;

use distribution_filename::WheelFilename;
use distribution_types::{
BuiltDist, DirectGitUrl, Dist, FileLocation, IndexLocations, LocalEditable, Name, SourceDist,
BuildableSource, BuiltDist, DirectGitUrl, Dist, FileLocation, IndexLocations, LocalEditable,
Name, SourceDist,
};
use platform_tags::Tags;
use pypi_types::Metadata23;
Expand Down Expand Up @@ -116,7 +117,7 @@ impl<'a, Context: BuildContext + Send + Sync> DistributionDatabase<'a, Context>
let url = Url::from_file_path(path).expect("path is absolute");
let cache_entry = self.cache.entry(
CacheBucket::Wheels,
WheelCache::Url(&url).remote_wheel_dir(wheel.name().as_ref()),
WheelCache::Url(&url).wheel_dir(wheel.name().as_ref()),
wheel.filename.stem(),
);

Expand Down Expand Up @@ -154,7 +155,7 @@ impl<'a, Context: BuildContext + Send + Sync> DistributionDatabase<'a, Context>
// Create a cache entry for the wheel.
let wheel_entry = self.cache.entry(
CacheBucket::Wheels,
WheelCache::Index(&wheel.index).remote_wheel_dir(wheel.name().as_ref()),
WheelCache::Index(&wheel.index).wheel_dir(wheel.name().as_ref()),
wheel.filename.stem(),
);

Expand Down Expand Up @@ -196,7 +197,7 @@ impl<'a, Context: BuildContext + Send + Sync> DistributionDatabase<'a, Context>
// Create a cache entry for the wheel.
let wheel_entry = self.cache.entry(
CacheBucket::Wheels,
WheelCache::Url(&wheel.url).remote_wheel_dir(wheel.name().as_ref()),
WheelCache::Url(&wheel.url).wheel_dir(wheel.name().as_ref()),
wheel.filename.stem(),
);

Expand Down Expand Up @@ -247,7 +248,7 @@ impl<'a, Context: BuildContext + Send + Sync> DistributionDatabase<'a, Context>

let cache_entry = self.cache.entry(
CacheBucket::Wheels,
WheelCache::Url(&wheel.url).remote_wheel_dir(wheel.name().as_ref()),
WheelCache::Url(&wheel.url).wheel_dir(wheel.name().as_ref()),
wheel.filename.stem(),
);

Expand Down Expand Up @@ -284,7 +285,11 @@ impl<'a, Context: BuildContext + Send + Sync> DistributionDatabase<'a, Context>
let lock = self.locks.acquire(&dist).await;
let _guard = lock.lock().await;

let built_wheel = self.builder.download_and_build(source_dist).boxed().await?;
let built_wheel = self
.builder
.download_and_build(BuildableSource::Dist(source_dist))
.boxed()
.await?;

// If the wheel was unzipped previously, respect it. Source distributions are
// cached under a unique build ID, so unzipped directories are never stale.
Expand Down Expand Up @@ -358,7 +363,7 @@ impl<'a, Context: BuildContext + Send + Sync> DistributionDatabase<'a, Context>

let metadata = self
.builder
.download_and_build_metadata(&source_dist)
.download_and_build_metadata(BuildableSource::Dist(&source_dist))
.boxed()
.await?;
Ok((metadata, precise))
Expand Down
Loading
Loading