From d8619f668a0e46c04c72f13b631fc11da4d21023 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 12 Feb 2024 22:41:00 -0500 Subject: [PATCH] Surface errors for offline `--find-links` URLs (#1271) ## Summary Ensures that if the user passes `--no-index` with `--find-links`, and we're unable to access the HTML page, we show an appropriate hint. --- crates/puffin-client/src/flat_index.rs | 123 +++++++++++++----- .../puffin-resolver/src/resolver/provider.rs | 2 + crates/puffin/tests/pip_compile.rs | 8 +- 3 files changed, 95 insertions(+), 38 deletions(-) diff --git a/crates/puffin-client/src/flat_index.rs b/crates/puffin-client/src/flat_index.rs index 9483bdb73..51cace38b 100644 --- a/crates/puffin-client/src/flat_index.rs +++ b/crates/puffin-client/src/flat_index.rs @@ -32,7 +32,48 @@ pub enum FlatIndexError { FindLinksUrl(Url, #[source] Error), } -type FlatIndexEntry = (DistFilename, File, IndexUrl); +#[derive(Debug, Default, Clone)] +pub struct FlatIndexEntries { + /// The list of `--find-links` entries. + entries: Vec<(DistFilename, File, IndexUrl)>, + /// Whether any `--find-links` entries could not be resolved due to a lack of network + /// connectivity. + offline: bool, +} + +impl FlatIndexEntries { + /// Create a [`FlatIndexEntries`] from a list of `--find-links` entries. + fn from_entries(entries: Vec<(DistFilename, File, IndexUrl)>) -> Self { + Self { + entries, + offline: false, + } + } + + /// Create a [`FlatIndexEntries`] to represent an offline `--find-links` entry. + fn offline() -> Self { + Self { + entries: Vec::new(), + offline: true, + } + } + + /// Extend this list of `--find-links` entries with another list. + fn extend(&mut self, other: Self) { + self.entries.extend(other.entries); + self.offline |= other.offline; + } + + /// Return the number of `--find-links` entries. + fn len(&self) -> usize { + self.entries.len() + } + + /// Return `true` if there are no `--find-links` entries. + fn is_empty(&self) -> bool { + self.entries.is_empty() + } +} /// A client for reading distributions from `--find-links` entries (either local directories or /// remote HTML indexes). @@ -53,7 +94,7 @@ impl<'a> FlatIndexClient<'a> { pub async fn fetch( &self, indexes: impl Iterator, - ) -> Result, FlatIndexError> { + ) -> Result { let mut fetches = futures::stream::iter(indexes) .map(|index| async move { let entries = match index { @@ -74,11 +115,11 @@ impl<'a> FlatIndexClient<'a> { index ); } - Ok::, FlatIndexError>(entries) + Ok::(entries) }) .buffered(16); - let mut results = Vec::new(); + let mut results = FlatIndexEntries::default(); while let Some(entries) = fetches.next().await.transpose()? { results.extend(entries); } @@ -86,7 +127,7 @@ impl<'a> FlatIndexClient<'a> { } /// Read a flat remote index from a `--find-links` URL. - async fn read_from_url(&self, url: &Url) -> Result, Error> { + async fn read_from_url(&self, url: &Url) -> Result { let cache_entry = self.cache.entry( CacheBucket::FlatIndex, "html", @@ -142,28 +183,29 @@ impl<'a> FlatIndexClient<'a> { parse_simple_response, ) .await; - let files = match response { - Ok(files) => files, - Err(CachedClientError::Client(err)) if matches!(err.kind(), ErrorKind::Offline(_)) => { - warn!("Remote `--find-links` entry was not available in the cache: {url}"); - vec![] + match response { + Ok(files) => { + let files = files + .into_iter() + .filter_map(|file| { + Some(( + DistFilename::try_from_normalized_filename(&file.filename)?, + file, + IndexUrl::Url(url.clone()), + )) + }) + .collect(); + Ok(FlatIndexEntries::from_entries(files)) } - Err(err) => return Err(err.into()), - }; - Ok(files - .into_iter() - .filter_map(|file| { - Some(( - DistFilename::try_from_normalized_filename(&file.filename)?, - file, - IndexUrl::Url(url.clone()), - )) - }) - .collect()) + Err(CachedClientError::Client(err)) if matches!(err.kind(), ErrorKind::Offline(_)) => { + Ok(FlatIndexEntries::offline()) + } + Err(err) => Err(err.into()), + } } /// Read a flat remote index from a `--find-links` directory. - fn read_from_directory(path: &PathBuf) -> Result, std::io::Error> { + fn read_from_directory(path: &PathBuf) -> Result { // Absolute paths are required for the URL conversion. let path = fs_err::canonicalize(path)?; @@ -203,28 +245,36 @@ impl<'a> FlatIndexClient<'a> { }; dists.push((filename, file, IndexUrl::Pypi)); } - Ok(dists) + Ok(FlatIndexEntries::from_entries(dists)) } } /// A set of [`PrioritizedDistribution`] from a `--find-links` entry, indexed by [`PackageName`] /// and [`Version`]. #[derive(Debug, Clone, Default)] -pub struct FlatIndex(FxHashMap); +pub struct FlatIndex { + /// The list of [`FlatDistributions`] from the `--find-links` entries, indexed by package name. + index: FxHashMap, + /// Whether any `--find-links` entries could not be resolved due to a lack of network + /// connectivity. + offline: bool, +} impl FlatIndex { /// Collect all files from a `--find-links` target into a [`FlatIndex`]. #[instrument(skip_all)] - pub fn from_entries(entries: Vec, tags: &Tags) -> Self { - let mut flat_index = FxHashMap::default(); - + pub fn from_entries(entries: FlatIndexEntries, tags: &Tags) -> Self { // Collect compatible distributions. - for (filename, file, index) in entries { - let distributions = flat_index.entry(filename.name().clone()).or_default(); - Self::add_file(distributions, file, filename, tags, index); + let mut index = FxHashMap::default(); + for (filename, file, url) in entries.entries { + let distributions = index.entry(filename.name().clone()).or_default(); + Self::add_file(distributions, file, filename, tags, url); } - Self(flat_index) + // Collect offline entries. + let offline = entries.offline; + + Self { index, offline } } fn add_file( @@ -277,7 +327,12 @@ impl FlatIndex { /// Get the [`FlatDistributions`] for the given package name. pub fn get(&self, package_name: &PackageName) -> Option<&FlatDistributions> { - self.0.get(package_name) + self.index.get(package_name) + } + + /// Returns `true` if there are any offline `--find-links` entries. + pub fn offline(&self) -> bool { + self.offline } } @@ -297,8 +352,8 @@ impl FlatDistributions { } impl IntoIterator for FlatDistributions { - type IntoIter = std::collections::btree_map::IntoIter; type Item = (Version, PrioritizedDistribution); + type IntoIter = std::collections::btree_map::IntoIter; fn into_iter(self) -> Self::IntoIter { self.0.into_iter() diff --git a/crates/puffin-resolver/src/resolver/provider.rs b/crates/puffin-resolver/src/resolver/provider.rs index 986ea0ae7..8bff0f1b1 100644 --- a/crates/puffin-resolver/src/resolver/provider.rs +++ b/crates/puffin-resolver/src/resolver/provider.rs @@ -158,6 +158,8 @@ impl<'a, Context: BuildContext + Send + Sync> ResolverProvider puffin_client::ErrorKind::NoIndex(_) => { if let Some(flat_index) = self.flat_index.get(package_name).cloned() { Ok(VersionsResponse::Found(VersionMap::from(flat_index))) + } else if self.flat_index.offline() { + Ok(VersionsResponse::Offline) } else { Ok(VersionsResponse::NoIndex) } diff --git a/crates/puffin/tests/pip_compile.rs b/crates/puffin/tests/pip_compile.rs index 6adb0b72a..67efce25b 100644 --- a/crates/puffin/tests/pip_compile.rs +++ b/crates/puffin/tests/pip_compile.rs @@ -3235,8 +3235,6 @@ fn offline_find_links() -> Result<()> { ); // Resolve with `--offline`, `--find-links`, and `--no-index`. - // TODO(charlie): This should indicate that the network was disabled, but we don't "know" that - // the `--find-links` lookup failed. puffin_snapshot!(context.compile() .arg("requirements.in") .arg("--find-links") @@ -3249,8 +3247,10 @@ fn offline_find_links() -> Result<()> { ----- stderr ----- × No solution found when resolving dependencies: - ╰─▶ Because tqdm was not found in the provided package locations and you - require tqdm, we can conclude that the requirements are unsatisfiable. + ╰─▶ Because tqdm was not found in the cache and you require tqdm, we can + conclude that the requirements are unsatisfiable. + + hint: Packages were unavailable because the network was disabled "### );