Improve error messages for 'file not found' case (#550)

Right now, if you specify a wheel that doesn't exist, you get: `no such
file or directory` with no additional context. Oops!
This commit is contained in:
Charlie Marsh 2023-12-04 17:01:51 -05:00 committed by GitHub
parent 4e05cd5dfd
commit 5fddcc362e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 100 additions and 20 deletions

View file

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

View file

@ -198,7 +198,8 @@ impl Dist {
let path = url
.to_file_path()
.map_err(|()| Error::UrlFilename(url.clone()))?
.canonicalize()?;
.canonicalize()
.map_err(|err| Error::NotFound(url.clone(), err))?;
return if path
.extension()
.is_some_and(|ext| ext.eq_ignore_ascii_case("whl"))

View file

@ -1477,3 +1477,34 @@ fn compile_source_distribution_path_dependency() -> Result<()> {
Ok(())
}
/// Resolve a local path dependency to a non-existent file.
#[test]
fn compile_wheel_path_dependency_missing() -> Result<()> {
let temp_dir = TempDir::new()?;
let cache_dir = TempDir::new()?;
let venv = create_venv_py312(&temp_dir, &cache_dir);
let requirements_in = temp_dir.child("requirements.in");
requirements_in.write_str("flask @ file:///path/to/flask-3.0.0-py3-none-any.whl")?;
// In addition to the standard filters, remove the temporary directory from the snapshot.
let mut filters = INSTA_FILTERS.to_vec();
filters.push((r"file://.*/", "file://[TEMP_DIR]/"));
insta::with_settings!({
filters => filters
}, {
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.arg("pip-compile")
.arg("requirements.in")
.arg("--cache-dir")
.arg(cache_dir.path())
.arg("--exclude-newer")
.arg(EXCLUDE_NEWER)
.env("VIRTUAL_ENV", venv.as_os_str())
.current_dir(&temp_dir));
});
Ok(())
}

View file

@ -6,17 +6,17 @@ info:
- pip-compile
- requirements.in
- "--cache-dir"
- /tmp/.tmpR1rZex
- /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpUI3Kjp
- "--exclude-newer"
- "2023-11-18T12:00:00Z"
env:
VIRTUAL_ENV: /tmp/.tmpy0g4rP/.venv
VIRTUAL_ENV: /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpDZkiLP/.venv
---
success: false
exit_code: 2
----- stdout -----
----- stderr -----
error: Failed to download and build dask @ git+https://github.com/pallets/flask.git@3.0.0
error: Failed to download and build: dask @ git+https://github.com/pallets/flask.git@3.0.0
Caused by: Package metadata name `flask` does not match given name `dask`

View file

@ -0,0 +1,22 @@
---
source: crates/puffin-cli/tests/pip_compile.rs
info:
program: puffin
args:
- pip-compile
- requirements.in
- "--cache-dir"
- /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpI36Udp
- "--exclude-newer"
- "2023-11-18T12:00:00Z"
env:
VIRTUAL_ENV: /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpuH6gF4/.venv
---
success: false
exit_code: 2
----- 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)

View file

@ -125,7 +125,7 @@ impl CachedClient {
serde_json::to_vec(&data_with_cache_policy).map_err(crate::Error::from)?,
)
.await
.map_err(crate::Error::from)?;
.map_err(crate::Error::CacheWrite)?;
Ok(data_with_cache_policy.data)
}
CachedResponse::ModifiedOrNew(res, cache_policy) => {
@ -136,12 +136,12 @@ impl CachedClient {
let data_with_cache_policy = DataWithCachePolicy { data, cache_policy };
fs_err::tokio::create_dir_all(&cache_entry.dir)
.await
.map_err(crate::Error::from)?;
.map_err(crate::Error::CacheWrite)?;
let data =
serde_json::to_vec(&data_with_cache_policy).map_err(crate::Error::from)?;
write_atomic(cache_entry.path(), data)
.await
.map_err(crate::Error::from)?;
.map_err(crate::Error::CacheWrite)?;
Ok(data_with_cache_policy.data)
} else {
Ok(data)

View file

@ -68,7 +68,10 @@ pub enum Error {
Zip(WheelFilename, #[source] ZipError),
#[error("Failed to write to the client cache")]
IO(#[from] io::Error),
CacheWrite(#[source] io::Error),
#[error(transparent)]
Io(#[from] io::Error),
/// An [`io::Error`] with a filename attached
#[error(transparent)]

View file

@ -8,7 +8,7 @@ use futures::TryStreamExt;
use reqwest::{Client, ClientBuilder, Response, StatusCode};
use reqwest_retry::policies::ExponentialBackoff;
use reqwest_retry::RetryTransientMiddleware;
use tempfile::tempfile;
use tempfile::tempfile_in;
use tokio::io::BufWriter;
use tokio_util::compat::FuturesAsyncReadCompatExt;
use tracing::{debug, trace};
@ -318,10 +318,12 @@ impl RegistryClient {
// you host your wheels for some reasons doesn't support range requests
// (tbh we should probably warn here and tell users to get a better registry because
// their current one makes resolution unnecessary slow)
let temp_download = tempfile()?;
let temp_download = tempfile_in(cache_shard.wheel_dir()).map_err(Error::CacheWrite)?;
let mut writer = BufWriter::new(tokio::fs::File::from_std(temp_download));
let mut reader = self.stream_external(url).await?.compat();
tokio::io::copy(&mut reader, &mut writer).await?;
tokio::io::copy(&mut reader, &mut writer)
.await
.map_err(Error::CacheWrite)?;
let reader = writer.into_inner();
Self::metadata_from_async_read(filename, url.to_string(), reader).await

View file

@ -25,7 +25,7 @@ use distribution_types::{GitSourceDist, Identifier, RemoteSource, SourceDist};
use install_wheel_rs::read_dist_info;
use platform_tags::Tags;
use puffin_cache::{digest, write_atomic, CacheBucket, CacheEntry, CanonicalUrl, WheelCache};
use puffin_client::{CachedClient, CachedClientError, DataWithCachePolicy};
use puffin_client::{CachedClient, CachedClientError, DataWithCachePolicy, Error};
use puffin_git::{Fetch, GitSource};
use puffin_normalize::PackageName;
use puffin_traits::BuildContext;
@ -471,11 +471,17 @@ impl<'a, T: BuildContext> SourceDistCachedBuilder<'a, T> {
// Download the source distribution.
let cache_dir = self.build_context.cache().bucket(CacheBucket::BuiltWheels);
fs::create_dir_all(&cache_dir).await?;
let temp_dir = tempfile::tempdir_in(cache_dir)?;
fs::create_dir_all(&cache_dir)
.await
.map_err(Error::CacheWrite)?;
let temp_dir = tempfile::tempdir_in(cache_dir).map_err(Error::CacheWrite)?;
let sdist_file = temp_dir.path().join(source_dist_filename);
let mut writer = tokio::fs::File::create(&sdist_file).await?;
tokio::io::copy(&mut reader, &mut writer).await?;
let mut writer = tokio::fs::File::create(&sdist_file)
.await
.map_err(Error::CacheWrite)?;
tokio::io::copy(&mut reader, &mut writer)
.await
.map_err(Error::CacheWrite)?;
Ok((Some(temp_dir), sdist_file))
}

View file

@ -5,7 +5,7 @@ use pubgrub::report::{DefaultStringReporter, Reporter};
use thiserror::Error;
use url::Url;
use distribution_types::{BuiltDist, SourceDist};
use distribution_types::{BuiltDist, PathBuiltDist, PathSourceDist, SourceDist};
use pep508_rs::Requirement;
use puffin_distribution::DistributionDatabaseError;
use puffin_normalize::PackageName;
@ -54,11 +54,17 @@ pub enum ResolveError {
#[error(transparent)]
DistributionType(#[from] distribution_types::Error),
#[error("Failed to download {0}")]
#[error("Failed to download: {0}")]
Fetch(Box<BuiltDist>, #[source] DistributionDatabaseError),
#[error("Failed to download and build {0}")]
#[error("Failed to download and build: {0}")]
FetchAndBuild(Box<SourceDist>, #[source] DistributionDatabaseError),
#[error("Failed to read: {0}")]
Read(Box<PathBuiltDist>, #[source] DistributionDatabaseError),
#[error("Failed to build: {0}")]
Build(Box<PathSourceDist>, #[source] DistributionDatabaseError),
}
impl<T> From<futures::channel::mpsc::TrySendError<T>> for ResolveError {

View file

@ -17,7 +17,7 @@ use url::Url;
use waitmap::WaitMap;
use distribution_filename::WheelFilename;
use distribution_types::{Dist, Identifier, Metadata, SourceDist, VersionOrUrl};
use distribution_types::{BuiltDist, Dist, Identifier, Metadata, SourceDist, VersionOrUrl};
use pep508_rs::{MarkerEnvironment, Requirement};
use platform_tags::Tags;
use puffin_cache::CanonicalUrl;
@ -610,6 +610,12 @@ impl<'a, Context: BuildContext + Send + Sync> Resolver<'a, Context> {
.get_or_build_wheel_metadata(&dist)
.await
.map_err(|err| match dist.clone() {
Dist::Built(BuiltDist::Path(built_dist)) => {
ResolveError::Read(Box::new(built_dist), err)
}
Dist::Source(SourceDist::Path(source_dist)) => {
ResolveError::Build(Box::new(source_dist), err)
}
Dist::Built(built_dist) => ResolveError::Fetch(Box::new(built_dist), err),
Dist::Source(source_dist) => {
ResolveError::FetchAndBuild(Box::new(source_dist), err)