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.
This commit is contained in:
Charlie Marsh 2024-01-18 22:40:48 -05:00 committed by GitHub
parent 6af4eb7a45
commit 9b24fcd306
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 19 additions and 17 deletions

View file

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

View file

@ -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(),
}
}
}

View file

@ -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<Vec<FlatIndexEntry>, 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,
};

View file

@ -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?
}

View file

@ -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(),
);

View file

@ -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,
};