Skip to content

Commit

Permalink
Use a dedicated error type for puffin-distribution (#466)
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh authored Nov 20, 2023
1 parent 342fc62 commit 8decb29
Show file tree
Hide file tree
Showing 11 changed files with 52 additions and 22 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

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

13 changes: 8 additions & 5 deletions crates/install-wheel-rs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ pub enum Error {
Pep440,
#[error("Invalid direct_url.json")]
DirectUrlJson(#[from] serde_json::Error),
#[error("No .dist-info directory found")]
MissingDistInfo,
#[error("Multiple .dist-info directories found: {0}")]
MultipleDistInfo(String),
}

impl Error {
Expand All @@ -84,7 +88,7 @@ impl Error {
pub fn find_dist_info<'a, T: Copy>(
filename: &WheelFilename,
files: impl Iterator<Item = (T, &'a str)>,
) -> Result<(T, &'a str), String> {
) -> Result<(T, &'a str), Error> {
let metadatas: Vec<_> = files
.filter_map(|(payload, path)| {
let (dist_info_dir, file) = path.split_once('/')?;
Expand All @@ -102,17 +106,16 @@ pub fn find_dist_info<'a, T: Copy>(
.collect();
let (payload, dist_info_dir) = match metadatas[..] {
[] => {
return Err("no .dist-info directory".to_string());
return Err(Error::MissingDistInfo);
}
[(payload, path)] => (payload, path),
_ => {
return Err(format!(
"multiple .dist-info directories: {}",
return Err(Error::MultipleDistInfo(
metadatas
.into_iter()
.map(|(_, dist_info_dir)| dist_info_dir.to_string())
.collect::<Vec<_>>()
.join(", ")
.join(", "),
));
}
};
Expand Down
3 changes: 1 addition & 2 deletions crates/install-wheel-rs/src/wheel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -930,8 +930,7 @@ pub fn install_wheel(
ZipArchive::new(reader).map_err(|err| Error::from_zip_error("(index)".to_string(), err))?;

debug!(name = name.as_ref(), "Getting wheel metadata");
let dist_info_prefix = find_dist_info(filename, archive.file_names().map(|name| (name, name)))
.map_err(Error::InvalidWheel)?
let dist_info_prefix = find_dist_info(filename, archive.file_names().map(|name| (name, name)))?
.1
.to_string();
let (name, _version) = read_metadata(&dist_info_prefix, &mut archive)?;
Expand Down
3 changes: 1 addition & 2 deletions crates/puffin-client/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -301,8 +301,7 @@ impl RegistryClient {
.iter()
.enumerate()
.filter_map(|(idx, e)| Some((idx, e.entry().filename().as_str().ok()?))),
)
.map_err(|err| Error::InvalidDistInfo(filename.clone(), err))?;
)?;

// Read the contents of the METADATA file
let mut contents = Vec::new();
Expand Down
3 changes: 3 additions & 0 deletions crates/puffin-client/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ pub enum Error {
#[error(transparent)]
UrlParseError(#[from] url::ParseError),

#[error(transparent)]
InstallWheel(#[from] install_wheel_rs::Error),

#[error("{0} isn't available locally, but making network requests to registries was banned.")]
NoIndex(String),

Expand Down
3 changes: 1 addition & 2 deletions crates/puffin-client/src/remote_metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,7 @@ pub(crate) async fn wheel_metadata_from_remote_zip(
.iter()
.enumerate()
.filter_map(|(idx, e)| Some(((idx, e), e.entry().filename().as_str().ok()?))),
)
.map_err(|err| Error::InvalidDistInfo(filename.clone(), err))?;
)?;

let offset = metadata_entry.header_offset();
let size = metadata_entry.entry().compressed_size()
Expand Down
1 change: 1 addition & 0 deletions crates/puffin-distribution/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ bytesize = { workspace = true }
fs-err = { workspace = true }
rayon = { workspace = true }
tempfile = { workspace = true }
thiserror = { workspace = true }
tokio = { workspace = true }
tokio-util = { workspace = true, features = ["compat"] }
tracing = { workspace = true }
Expand Down
22 changes: 14 additions & 8 deletions crates/puffin-distribution/src/download.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::path::PathBuf;
use std::str::FromStr;

use anyhow::{format_err, Context, Result};
use anyhow::{Context, Result};
use tempfile::TempDir;
use zip::ZipArchive;

Expand All @@ -10,6 +10,8 @@ use distribution_types::{Dist, RemoteSource};
use install_wheel_rs::find_dist_info;
use pypi_types::Metadata21;

use crate::error::Error;

/// A downloaded wheel that's stored in-memory.
#[derive(Debug)]
pub struct InMemoryWheel {
Expand Down Expand Up @@ -86,12 +88,14 @@ impl std::fmt::Display for SourceDistDownload {

impl InMemoryWheel {
/// Read the [`Metadata21`] from a wheel.
pub fn read_dist_info(&self) -> Result<Metadata21> {
pub fn read_dist_info(&self) -> Result<Metadata21, Error> {
let mut archive = ZipArchive::new(std::io::Cursor::new(&self.buffer))?;
let filename = self.filename()?;
let filename = self
.filename()
.map_err(|err| Error::FilenameParse(self.dist.to_string(), err))?;
let dist_info_dir =
find_dist_info(&filename, archive.file_names().map(|name| (name, name)))
.map_err(|err| format_err!("Invalid wheel {filename}: {err}"))?
.map_err(|err| Error::DistInfo(self.dist.to_string(), err))?
.1;
let dist_info =
std::io::read_to_string(archive.by_name(&format!("{dist_info_dir}/METADATA"))?)?;
Expand All @@ -101,12 +105,14 @@ impl InMemoryWheel {

impl DiskWheel {
/// Read the [`Metadata21`] from a wheel.
pub fn read_dist_info(&self) -> Result<Metadata21> {
pub fn read_dist_info(&self) -> Result<Metadata21, Error> {
let mut archive = ZipArchive::new(fs_err::File::open(&self.path)?)?;
let filename = self.filename()?;
let filename = self
.filename()
.map_err(|err| Error::FilenameParse(self.dist.to_string(), err))?;
let dist_info_dir =
find_dist_info(&filename, archive.file_names().map(|name| (name, name)))
.map_err(|err| format_err!("Invalid wheel {filename}: {err}"))?
.map_err(|err| Error::DistInfo(self.dist.to_string(), err))?
.1;
let dist_info =
std::io::read_to_string(archive.by_name(&format!("{dist_info_dir}/METADATA"))?)?;
Expand All @@ -116,7 +122,7 @@ impl DiskWheel {

impl WheelDownload {
/// Read the [`Metadata21`] from a wheel.
pub fn read_dist_info(&self) -> Result<Metadata21> {
pub fn read_dist_info(&self) -> Result<Metadata21, Error> {
match self {
WheelDownload::InMemory(wheel) => wheel.read_dist_info(),
WheelDownload::Disk(wheel) => wheel.read_dist_info(),
Expand Down
13 changes: 13 additions & 0 deletions crates/puffin-distribution/src/error.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#[derive(thiserror::Error, Debug)]
pub enum Error {
#[error(transparent)]
IO(#[from] std::io::Error),
#[error(transparent)]
PypiTypes(#[from] pypi_types::Error),
#[error(transparent)]
Zip(#[from] zip::result::ZipError),
#[error("Unable to read .dist-info directory for: {0}")]
DistInfo(String, #[source] install_wheel_rs::Error),
#[error("Unable to parse wheel filename for: {0}")]
FilenameParse(String, #[source] anyhow::Error),
}
11 changes: 8 additions & 3 deletions crates/puffin-distribution/src/fetcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use puffin_git::{GitSource, GitUrl};
use puffin_traits::BuildContext;
use pypi_types::Metadata21;

use crate::error::Error;
use crate::reporter::Facade;
use crate::{DiskWheel, Download, InMemoryWheel, Reporter, SourceDistDownload, WheelDownload};

Expand Down Expand Up @@ -52,7 +53,7 @@ impl<'a> Fetcher<'a> {
}

/// Return the [`Metadata21`] for a distribution, if it exists in the cache.
pub fn find_metadata(&self, dist: &Dist, tags: &Tags) -> Result<Option<Metadata21>> {
pub fn find_metadata(&self, dist: &Dist, tags: &Tags) -> Result<Option<Metadata21>, Error> {
self.find_in_cache(dist, tags)
.map(|wheel| wheel.read_dist_info())
.transpose()
Expand All @@ -77,10 +78,14 @@ impl<'a> Fetcher<'a> {
// Fetch the distribution, then read the metadata (for built distributions), or build
// the distribution and _then_ read the metadata (for source distributions).
dist => match self.fetch_dist(dist, client).await? {
Download::Wheel(wheel) => wheel.read_dist_info(),
Download::Wheel(wheel) => {
let metadata = wheel.read_dist_info()?;
Ok(metadata)
}
Download::SourceDist(sdist) => {
let wheel = self.build_sdist(sdist, build_context).await?;
wheel.read_dist_info()
let metadata = wheel.read_dist_info()?;
Ok(metadata)
}
},
}
Expand Down
1 change: 1 addition & 0 deletions crates/puffin-distribution/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ pub use crate::reporter::Reporter;
pub use crate::unzip::Unzip;

mod download;
mod error;
mod fetcher;
mod reporter;
mod unzip;
Expand Down

0 comments on commit 8decb29

Please sign in to comment.