From d9cc9dbf88d0bfe4449e69dc8115235da4d70fc0 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 20 Jan 2024 12:59:18 -0500 Subject: [PATCH] Improve error message when editable requirement doesn't exist (#1024) Making these a lot clearer in the common case by reducing the depth of the error. --- crates/distribution-types/src/error.rs | 4 ++-- crates/distribution-types/src/lib.rs | 13 ++++++++++--- crates/puffin-build/src/lib.rs | 12 +++++++++++- crates/puffin-distribution/src/source/error.rs | 3 +++ crates/puffin-distribution/src/source/mod.rs | 4 ++-- crates/puffin-installer/src/downloader.rs | 6 +++--- crates/puffin/tests/pip_compile.rs | 10 +++------- 7 files changed, 34 insertions(+), 18 deletions(-) diff --git a/crates/distribution-types/src/error.rs b/crates/distribution-types/src/error.rs index 8ddcba0cb..c461acef8 100644 --- a/crates/distribution-types/src/error.rs +++ b/crates/distribution-types/src/error.rs @@ -11,6 +11,6 @@ pub enum Error { #[error("Unable to extract filename from URL: {0}")] UrlFilename(Url), - #[error("Unable to locate distribution at: {0}")] - NotFound(Url, #[source] std::io::Error), + #[error("Distribution not found at: {0}")] + NotFound(Url), } diff --git a/crates/distribution-types/src/lib.rs b/crates/distribution-types/src/lib.rs index 2ca6fb4d4..c04eab4bd 100644 --- a/crates/distribution-types/src/lib.rs +++ b/crates/distribution-types/src/lib.rs @@ -228,12 +228,19 @@ impl Dist { } if url.scheme().eq_ignore_ascii_case("file") { - // Store the canonicalized path. - let path = url + // Store the canonicalized path, which also serves to validate that it exists. + let path = match url .to_file_path() .map_err(|()| Error::UrlFilename(url.to_url()))? .canonicalize() - .map_err(|err| Error::NotFound(url.to_url(), err))?; + { + Ok(path) => path, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => { + return Err(Error::NotFound(url.to_url())); + } + Err(err) => return Err(err.into()), + }; + return if path .extension() .is_some_and(|ext| ext.eq_ignore_ascii_case("whl")) diff --git a/crates/puffin-build/src/lib.rs b/crates/puffin-build/src/lib.rs index 70359c23b..7f0a06866 100644 --- a/crates/puffin-build/src/lib.rs +++ b/crates/puffin-build/src/lib.rs @@ -69,6 +69,8 @@ pub enum Error { EditableSetupPy, #[error("Failed to install requirements from {0}")] RequirementsInstall(&'static str, #[source] anyhow::Error), + #[error("Source distribution not found at: {0}")] + NotFound(PathBuf), #[error("Failed to create temporary virtual environment")] Gourgeist(#[from] gourgeist::Error), #[error("Failed to run {0}")] @@ -280,7 +282,15 @@ impl SourceBuild { ) -> Result { let temp_dir = tempdir()?; - let source_root = if fs::metadata(source)?.is_dir() { + let metadata = match fs::metadata(source) { + Ok(metadata) => metadata, + Err(err) if err.kind() == io::ErrorKind::NotFound => { + return Err(Error::NotFound(source.to_path_buf())); + } + Err(err) => return Err(err.into()), + }; + + let source_root = if metadata.is_dir() { source.to_path_buf() } else { debug!("Unpacking for build: {}", source.display()); diff --git a/crates/puffin-distribution/src/source/error.rs b/crates/puffin-distribution/src/source/error.rs index 4d9747f42..2f2c4e814 100644 --- a/crates/puffin-distribution/src/source/error.rs +++ b/crates/puffin-distribution/src/source/error.rs @@ -3,6 +3,7 @@ use tokio::task::JoinError; use zip::result::ZipError; use distribution_filename::WheelFilenameError; + use puffin_normalize::PackageName; /// The caller is responsible for adding the source dist information to the error chain @@ -32,6 +33,8 @@ pub enum SourceDistError { // Build error #[error("Failed to build: {0}")] Build(String, #[source] anyhow::Error), + #[error("Failed to build editable: {0}")] + BuildEditable(String, #[source] anyhow::Error), #[error("Built wheel has an invalid filename")] WheelFilename(#[from] WheelFilenameError), #[error("Package metadata name `{metadata}` does not match given name `{given}`")] diff --git a/crates/puffin-distribution/src/source/mod.rs b/crates/puffin-distribution/src/source/mod.rs index e4c1b8576..14f28a69a 100644 --- a/crates/puffin-distribution/src/source/mod.rs +++ b/crates/puffin-distribution/src/source/mod.rs @@ -992,10 +992,10 @@ impl<'a, T: BuildContext> SourceDistCachedBuilder<'a, T> { BuildKind::Editable, ) .await - .map_err(|err| SourceDistError::Build(editable.to_string(), err))? + .map_err(|err| SourceDistError::BuildEditable(editable.to_string(), err))? .wheel(editable_wheel_dir) .await - .map_err(|err| SourceDistError::Build(editable.to_string(), err))?; + .map_err(|err| SourceDistError::BuildEditable(editable.to_string(), err))?; let filename = WheelFilename::from_str(&disk_filename)?; // We finally have the name of the package and can construct the dist let dist = Dist::Source(SourceDist::Path(PathSourceDist { diff --git a/crates/puffin-installer/src/downloader.rs b/crates/puffin-installer/src/downloader.rs index a842cdca6..5bf10e383 100644 --- a/crates/puffin-installer/src/downloader.rs +++ b/crates/puffin-installer/src/downloader.rs @@ -25,8 +25,8 @@ pub enum Error { /// Should not occur; only seen when another task panicked. #[error("The task executor is broken, did some other task panic?")] Join(#[from] JoinError), - #[error("Failed to build editable: {0}")] - Editable(LocalEditable, #[source] DistributionDatabaseError), + #[error(transparent)] + Editable(#[from] DistributionDatabaseError), #[error("Unzip failed in another thread: {0}")] Thread(String), } @@ -121,7 +121,7 @@ impl<'a, Context: BuildContext + Send + Sync> Downloader<'a, Context> { .database .build_wheel_editable(&editable, editable_wheel_dir) .await - .map_err(|err| Error::Editable(editable.clone(), err))?; + .map_err(Error::Editable)?; let cached_dist = Self::unzip_wheel(local_wheel).await?; if let Some(task_id) = task_id { if let Some(reporter) = &self.reporter { diff --git a/crates/puffin/tests/pip_compile.rs b/crates/puffin/tests/pip_compile.rs index 5ee0acbc1..5e4504eea 100644 --- a/crates/puffin/tests/pip_compile.rs +++ b/crates/puffin/tests/pip_compile.rs @@ -2208,8 +2208,7 @@ fn compile_wheel_path_dependency_missing() -> Result<()> { ----- stdout ----- ----- stderr ----- - error: Unable to locate distribution at: file://[TEMP_DIR]/flask-3.0.0-py3-none-any.whl - Caused by: No such file or directory (os error 2) + error: Distribution not found at: file://[TEMP_DIR]/flask-3.0.0-py3-none-any.whl "###); }); @@ -3449,8 +3448,7 @@ fn missing_path_requirement() -> Result<()> { ----- stdout ----- ----- stderr ----- - error: Unable to locate distribution at: file:///tmp/django-3.2.8.tar.gz - Caused by: No such file or directory (os error 2) + error: Distribution not found at: file:///tmp/django-3.2.8.tar.gz "###); }); @@ -3491,9 +3489,7 @@ fn missing_editable_requirement() -> Result<()> { ----- stderr ----- error: Failed to build editables Caused by: Failed to build editable: file://[TEMP_DIR]/django-3.2.8.tar.gz - Caused by: Failed to build: file://[TEMP_DIR]/django-3.2.8.tar.gz - Caused by: failed to query metadata of file `file://[TEMP_DIR]/django-3.2.8.tar.gz` - Caused by: No such file or directory (os error 2) + Caused by: Source distribution not found at: file://[TEMP_DIR]/django-3.2.8.tar.gz "###); });