From 9b24fcd306dc8351e93b2112fede6182364d2ebf Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 18 Jan 2024 22:40:48 -0500 Subject: [PATCH] Remove verbatim URL from path file location (#998) ## Summary I got confused by why `VerbatimUrl` was on `Path`. Since it's directly computed from it, I think we should just compute it as-needed. I think it's also possibly-buggy because the URL is the URL of the _directory_, not the artifact itself, which differs from other distributions. --- crates/distribution-types/src/file.rs | 5 ++--- crates/distribution-types/src/lib.rs | 4 ++-- crates/puffin-client/src/flat_index.rs | 7 ++----- crates/puffin-client/src/registry_client.rs | 2 +- .../src/distribution_database.rs | 5 +++-- crates/puffin-distribution/src/source/mod.rs | 13 +++++++++---- 6 files changed, 19 insertions(+), 17 deletions(-) diff --git a/crates/distribution-types/src/file.rs b/crates/distribution-types/src/file.rs index 0011af614..dfe80d243 100644 --- a/crates/distribution-types/src/file.rs +++ b/crates/distribution-types/src/file.rs @@ -6,7 +6,6 @@ use serde::{Deserialize, Serialize}; use thiserror::Error; use pep440_rs::{VersionSpecifiers, VersionSpecifiersParseError}; -use pep508_rs::VerbatimUrl; use pypi_types::{BaseUrl, DistInfoMetadata, Hashes, Yanked}; /// Error converting [`pypi_types::File`] to [`distribution_type::File`]. @@ -59,7 +58,7 @@ pub enum FileLocation { /// Absolute URL. AbsoluteUrl(String), /// Absolute path to a file. - Path(PathBuf, VerbatimUrl), + Path(PathBuf), } impl Display for FileLocation { @@ -67,7 +66,7 @@ impl Display for FileLocation { match self { FileLocation::RelativeUrl(_base, url) => Display::fmt(&url, f), FileLocation::AbsoluteUrl(url) => Display::fmt(&url, f), - FileLocation::Path(path, _url) => Display::fmt(&path.display(), f), + FileLocation::Path(path) => Display::fmt(&path.display(), f), } } } diff --git a/crates/distribution-types/src/lib.rs b/crates/distribution-types/src/lib.rs index bba1cebb8..88946da74 100644 --- a/crates/distribution-types/src/lib.rs +++ b/crates/distribution-types/src/lib.rs @@ -729,7 +729,7 @@ impl Identifier for FileLocation { match self { FileLocation::RelativeUrl(base, url) => (base.as_url(), url.as_str()).distribution_id(), FileLocation::AbsoluteUrl(url) => url.distribution_id(), - FileLocation::Path(path, _) => path.distribution_id(), + FileLocation::Path(path) => path.distribution_id(), } } @@ -737,7 +737,7 @@ impl Identifier for FileLocation { match self { FileLocation::RelativeUrl(base, url) => (base.as_url(), url.as_str()).resource_id(), FileLocation::AbsoluteUrl(url) => url.resource_id(), - FileLocation::Path(path, _) => path.resource_id(), + FileLocation::Path(path) => path.resource_id(), } } } diff --git a/crates/puffin-client/src/flat_index.rs b/crates/puffin-client/src/flat_index.rs index 732fd2f85..b223c2f09 100644 --- a/crates/puffin-client/src/flat_index.rs +++ b/crates/puffin-client/src/flat_index.rs @@ -14,7 +14,6 @@ use distribution_types::{ RegistryBuiltDist, RegistrySourceDist, SourceDist, }; use pep440_rs::Version; -use pep508_rs::VerbatimUrl; use platform_tags::Tags; use puffin_cache::{Cache, CacheBucket}; use puffin_normalize::PackageName; @@ -142,11 +141,9 @@ impl<'a> FlatIndexClient<'a> { fn read_from_directory(path: &PathBuf) -> Result, std::io::Error> { // Absolute paths are required for the URL conversion. let path = fs_err::canonicalize(path)?; - let url = Url::from_directory_path(&path).expect("URL is already absolute"); - let url = VerbatimUrl::unknown(url); let mut dists = Vec::new(); - for entry in fs_err::read_dir(&path)? { + for entry in fs_err::read_dir(path)? { let entry = entry?; let metadata = entry.metadata()?; if !metadata.is_file() { @@ -168,7 +165,7 @@ impl<'a> FlatIndexClient<'a> { requires_python: None, size: None, upload_time: None, - url: FileLocation::Path(entry.path().to_path_buf(), url.clone()), + url: FileLocation::Path(entry.path().to_path_buf()), yanked: None, }; diff --git a/crates/puffin-client/src/registry_client.rs b/crates/puffin-client/src/registry_client.rs index 837b5af0c..faf05e562 100644 --- a/crates/puffin-client/src/registry_client.rs +++ b/crates/puffin-client/src/registry_client.rs @@ -235,7 +235,7 @@ impl RegistryClient { self.wheel_metadata_registry(&wheel.index, &wheel.file, &url) .await? } - FileLocation::Path(path, _url) => { + FileLocation::Path(path) => { let reader = fs_err::tokio::File::open(&path).await?; read_metadata_async(&wheel.filename, built_dist.to_string(), reader).await? } diff --git a/crates/puffin-distribution/src/distribution_database.rs b/crates/puffin-distribution/src/distribution_database.rs index 21530c6d5..75d5c36d4 100644 --- a/crates/puffin-distribution/src/distribution_database.rs +++ b/crates/puffin-distribution/src/distribution_database.rs @@ -116,10 +116,11 @@ impl<'a, Context: BuildContext + Send + Sync> DistributionDatabase<'a, Context> .map_err(|err| DistributionDatabaseError::Url(url.clone(), err))?, FileLocation::AbsoluteUrl(url) => Url::parse(url) .map_err(|err| DistributionDatabaseError::Url(url.clone(), err))?, - FileLocation::Path(path, url) => { + FileLocation::Path(path) => { + 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).remote_wheel_dir(wheel.name().as_ref()), wheel.filename.stem(), ); diff --git a/crates/puffin-distribution/src/source/mod.rs b/crates/puffin-distribution/src/source/mod.rs index 4ccdb67e2..3d065b04c 100644 --- a/crates/puffin-distribution/src/source/mod.rs +++ b/crates/puffin-distribution/src/source/mod.rs @@ -20,6 +20,7 @@ use distribution_types::{ Name, PathSourceDist, RemoteSource, SourceDist, }; use install_wheel_rs::read_dist_info; +use pep508_rs::VerbatimUrl; use platform_tags::Tags; use puffin_cache::{CacheBucket, CacheEntry, CacheShard, CachedByTimestamp, WheelCache}; use puffin_client::{CachedClient, CachedClientError, DataWithCachePolicy}; @@ -104,10 +105,12 @@ impl<'a, T: BuildContext> SourceDistCachedBuilder<'a, T> { .map_err(|err| SourceDistError::UrlParse(url.clone(), err))?, FileLocation::AbsoluteUrl(url) => Url::parse(url) .map_err(|err| SourceDistError::UrlParse(url.clone(), err))?, - FileLocation::Path(path, url) => { + FileLocation::Path(path) => { let path_source_dist = PathSourceDist { name: registry_source_dist.filename.name.clone(), - url: url.clone(), + url: VerbatimUrl::unknown( + Url::from_file_path(path).expect("path is absolute"), + ), path: path.clone(), editable: false, }; @@ -177,10 +180,12 @@ impl<'a, T: BuildContext> SourceDistCachedBuilder<'a, T> { .map_err(|err| SourceDistError::UrlParse(url.clone(), err))?, FileLocation::AbsoluteUrl(url) => Url::parse(url) .map_err(|err| SourceDistError::UrlParse(url.clone(), err))?, - FileLocation::Path(path, url) => { + FileLocation::Path(path) => { let path_source_dist = PathSourceDist { name: registry_source_dist.filename.name.clone(), - url: url.clone(), + url: VerbatimUrl::unknown( + Url::from_file_path(path).expect("path is absolute"), + ), path: path.clone(), editable: false, };