Avoid silently dropping errors in directory enumeration (#11890)

## Summary

Right now, _all_ errors are dropped here, which seems wrong. We should
only return an empty iterator if the directory doesn't exist.
This commit is contained in:
Charlie Marsh 2025-03-02 18:39:17 -08:00 committed by GitHub
parent 5ec9be0585
commit 04f20169db
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
9 changed files with 62 additions and 41 deletions

View file

@ -1027,14 +1027,14 @@ impl CacheBucket {
// For alternate indices, we expect a directory for every index (under an `index`
// subdirectory), followed by a directory per package (indexed by name).
let root = cache.bucket(self).join(WheelCacheKind::Index);
for directory in directories(root) {
for directory in directories(root)? {
summary += rm_rf(directory.join(name.to_string()))?;
}
// For direct URLs, we expect a directory for every URL, followed by a
// directory per package (indexed by name).
let root = cache.bucket(self).join(WheelCacheKind::Url);
for directory in directories(root) {
for directory in directories(root)? {
summary += rm_rf(directory.join(name.to_string()))?;
}
}
@ -1046,7 +1046,7 @@ impl CacheBucket {
// For alternate indices, we expect a directory for every index (under an `index`
// subdirectory), followed by a directory per package (indexed by name).
let root = cache.bucket(self).join(WheelCacheKind::Index);
for directory in directories(root) {
for directory in directories(root)? {
summary += rm_rf(directory.join(name.to_string()))?;
}
@ -1054,8 +1054,8 @@ impl CacheBucket {
// directory per version. To determine whether the URL is relevant, we need to
// search for a wheel matching the package name.
let root = cache.bucket(self).join(WheelCacheKind::Url);
for url in directories(root) {
if directories(&url).any(|version| is_match(&version, name)) {
for url in directories(root)? {
if directories(&url)?.any(|version| is_match(&version, name)) {
summary += rm_rf(url)?;
}
}
@ -1064,8 +1064,8 @@ impl CacheBucket {
// directory per version. To determine whether the path is relevant, we need to
// search for a wheel matching the package name.
let root = cache.bucket(self).join(WheelCacheKind::Path);
for path in directories(root) {
if directories(&path).any(|version| is_match(&version, name)) {
for path in directories(root)? {
if directories(&path)?.any(|version| is_match(&version, name)) {
summary += rm_rf(path)?;
}
}
@ -1074,8 +1074,8 @@ impl CacheBucket {
// directory for every SHA. To determine whether the SHA is relevant, we need to
// search for a wheel matching the package name.
let root = cache.bucket(self).join(WheelCacheKind::Git);
for repository in directories(root) {
for sha in directories(repository) {
for repository in directories(root)? {
for sha in directories(repository)? {
if is_match(&sha, name) {
summary += rm_rf(sha)?;
}
@ -1090,7 +1090,7 @@ impl CacheBucket {
// For alternate indices, we expect a directory for every index (under an `index`
// subdirectory), followed by a directory per package (indexed by name).
let root = cache.bucket(self).join(WheelCacheKind::Index);
for directory in directories(root) {
for directory in directories(root)? {
summary += rm_rf(directory.join(format!("{name}.rkyv")))?;
}
}

View file

@ -203,7 +203,7 @@ impl<'a> BuiltWheelIndex<'a> {
let mut candidate: Option<CachedWheel> = None;
// Unzipped wheels are stored as symlinks into the archive directory.
for wheel_dir in uv_fs::entries(shard) {
for wheel_dir in uv_fs::entries(shard).ok().into_iter().flatten() {
// Ignore any `.lock` files.
if wheel_dir
.extension()

View file

@ -103,7 +103,7 @@ impl<'a> RegistryWheelIndex<'a> {
// For registry wheels, the cache structure is: `<index>/<package-name>/<wheel>.http`
// or `<index>/<package-name>/<version>/<wheel>.rev`.
for file in files(&wheel_dir) {
for file in files(&wheel_dir).ok().into_iter().flatten() {
match index.url() {
// Add files from remote registries.
IndexUrl::Pypi(_) | IndexUrl::Url(_) => {
@ -170,7 +170,7 @@ impl<'a> RegistryWheelIndex<'a> {
);
// For registry source distributions, the cache structure is: `<index>/<package-name>/<version>/`.
for shard in directories(&cache_shard) {
for shard in directories(&cache_shard).ok().into_iter().flatten() {
let cache_shard = cache_shard.shard(shard);
// Read the revision from the cache.
@ -205,7 +205,7 @@ impl<'a> RegistryWheelIndex<'a> {
cache_shard.shard(cache_digest(build_configuration))
};
for wheel_dir in uv_fs::entries(cache_shard) {
for wheel_dir in uv_fs::entries(cache_shard).ok().into_iter().flatten() {
// Ignore any `.lock` files.
if wheel_dir
.extension()

View file

@ -28,16 +28,19 @@ pub(crate) struct BuiltWheelMetadata {
impl BuiltWheelMetadata {
/// Find a compatible wheel in the cache.
pub(crate) fn find_in_cache(tags: &Tags, cache_shard: &CacheShard) -> Option<Self> {
for file in files(cache_shard) {
pub(crate) fn find_in_cache(
tags: &Tags,
cache_shard: &CacheShard,
) -> Result<Option<Self>, std::io::Error> {
for file in files(cache_shard)? {
if let Some(metadata) = Self::from_path(file, cache_shard) {
// Validate that the wheel is compatible with the target platform.
if metadata.filename.is_compatible(tags) {
return Some(metadata);
return Ok(Some(metadata));
}
}
}
None
Ok(None)
}
/// Try to parse a distribution from a cached directory name (like `typing-extensions-4.8.0-py3-none-any.whl`).

View file

@ -425,6 +425,8 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {
// If the cache contains a compatible wheel, return it.
if let Some(built_wheel) = BuiltWheelMetadata::find_in_cache(tags, &cache_shard)
.ok()
.flatten()
.filter(|built_wheel| built_wheel.matches(source.name(), source.version()))
{
return Ok(built_wheel.with_hashes(revision.into_hashes()));
@ -795,6 +797,8 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {
// If the cache contains a compatible wheel, return it.
if let Some(built_wheel) = BuiltWheelMetadata::find_in_cache(tags, &cache_shard)
.ok()
.flatten()
.filter(|built_wheel| built_wheel.matches(source.name(), source.version()))
{
return Ok(built_wheel);
@ -1097,6 +1101,8 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {
// If the cache contains a compatible wheel, return it.
if let Some(built_wheel) = BuiltWheelMetadata::find_in_cache(tags, &cache_shard)
.ok()
.flatten()
.filter(|built_wheel| built_wheel.matches(source.name(), source.version()))
{
return Ok(built_wheel);
@ -1474,6 +1480,8 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {
// If the cache contains a compatible wheel, return it.
if let Some(built_wheel) = BuiltWheelMetadata::find_in_cache(tags, &cache_shard)
.ok()
.flatten()
.filter(|built_wheel| built_wheel.matches(source.name(), source.version()))
{
return Ok(built_wheel);

View file

@ -1,4 +1,5 @@
use std::fmt::Display;
use std::io;
use std::path::{Path, PathBuf};
use fs2::FileExt;
@ -516,60 +517,69 @@ pub fn persist_with_retry_sync(
/// Iterate over the subdirectories of a directory.
///
/// If the directory does not exist, returns an empty iterator.
pub fn directories(path: impl AsRef<Path>) -> impl Iterator<Item = PathBuf> {
path.as_ref()
.read_dir()
.ok()
pub fn directories(path: impl AsRef<Path>) -> Result<impl Iterator<Item = PathBuf>, io::Error> {
let entries = match path.as_ref().read_dir() {
Ok(entries) => Some(entries),
Err(err) if err.kind() == io::ErrorKind::NotFound => None,
Err(err) => return Err(err),
};
Ok(entries
.into_iter()
.flatten()
.filter_map(|entry| match entry {
Ok(entry) => Some(entry),
Err(err) => {
warn!("Failed to read entry: {}", err);
warn!("Failed to read entry: {err}");
None
}
})
.filter(|entry| entry.file_type().is_ok_and(|file_type| file_type.is_dir()))
.map(|entry| entry.path())
.map(|entry| entry.path()))
}
/// Iterate over the entries in a directory.
///
/// If the directory does not exist, returns an empty iterator.
pub fn entries(path: impl AsRef<Path>) -> impl Iterator<Item = PathBuf> {
path.as_ref()
.read_dir()
.ok()
pub fn entries(path: impl AsRef<Path>) -> Result<impl Iterator<Item = PathBuf>, io::Error> {
let entries = match path.as_ref().read_dir() {
Ok(entries) => Some(entries),
Err(err) if err.kind() == io::ErrorKind::NotFound => None,
Err(err) => return Err(err),
};
Ok(entries
.into_iter()
.flatten()
.filter_map(|entry| match entry {
Ok(entry) => Some(entry),
Err(err) => {
warn!("Failed to read entry: {}", err);
warn!("Failed to read entry: {err}");
None
}
})
.map(|entry| entry.path())
.map(|entry| entry.path()))
}
/// Iterate over the files in a directory.
///
/// If the directory does not exist, returns an empty iterator.
pub fn files(path: impl AsRef<Path>) -> impl Iterator<Item = PathBuf> {
path.as_ref()
.read_dir()
.ok()
pub fn files(path: impl AsRef<Path>) -> Result<impl Iterator<Item = PathBuf>, io::Error> {
let entries = match path.as_ref().read_dir() {
Ok(entries) => Some(entries),
Err(err) if err.kind() == io::ErrorKind::NotFound => None,
Err(err) => return Err(err),
};
Ok(entries
.into_iter()
.flatten()
.filter_map(|entry| match entry {
Ok(entry) => Some(entry),
Err(err) => {
warn!("Failed to read entry: {}", err);
warn!("Failed to read entry: {err}");
None
}
})
.filter(|entry| entry.file_type().is_ok_and(|file_type| file_type.is_file()))
.map(|entry| entry.path())
.map(|entry| entry.path()))
}
/// Returns `true` if a path is a temporary file or directory.

View file

@ -97,7 +97,7 @@ impl InstalledTools {
#[allow(clippy::type_complexity)]
pub fn tools(&self) -> Result<Vec<(PackageName, Result<Tool, Error>)>, Error> {
let mut tools = Vec::new();
for directory in uv_fs::directories(self.root()) {
for directory in uv_fs::directories(self.root())? {
let name = directory.file_name().unwrap().to_string_lossy().to_string();
let name = PackageName::from_str(&name)?;
let path = directory.join("uv-receipt.toml");

View file

@ -37,7 +37,7 @@ pub(crate) async fn uninstall(
do_uninstall(&installations, targets, all, printer, preview).await?;
// Clean up any empty directories.
if uv_fs::directories(installations.root()).all(|path| uv_fs::is_temporary(&path)) {
if uv_fs::directories(installations.root())?.all(|path| uv_fs::is_temporary(&path)) {
fs_err::tokio::remove_dir_all(&installations.root()).await?;
if let Some(top_level) = installations.root().parent() {
@ -48,7 +48,7 @@ pub(crate) async fn uninstall(
Err(err) => return Err(err.into()),
}
if uv_fs::directories(top_level).all(|path| uv_fs::is_temporary(&path)) {
if uv_fs::directories(top_level)?.all(|path| uv_fs::is_temporary(&path)) {
fs_err::tokio::remove_dir_all(top_level).await?;
}
}

View file

@ -34,12 +34,12 @@ pub(crate) async fn uninstall(name: Vec<PackageName>, printer: Printer) -> Resul
do_uninstall(&installed_tools, name, printer).await?;
// Clean up any empty directories.
if uv_fs::directories(installed_tools.root()).all(|path| uv_fs::is_temporary(&path)) {
if uv_fs::directories(installed_tools.root())?.all(|path| uv_fs::is_temporary(&path)) {
fs_err::tokio::remove_dir_all(&installed_tools.root())
.await
.ignore_currently_being_deleted()?;
if let Some(parent) = installed_tools.root().parent() {
if uv_fs::directories(parent).all(|path| uv_fs::is_temporary(&path)) {
if uv_fs::directories(parent)?.all(|path| uv_fs::is_temporary(&path)) {
fs_err::tokio::remove_dir_all(parent)
.await
.ignore_currently_being_deleted()?;