Improve error handling

pyproject.toml

@@ -3,6 +3,7 @@ name = "virtpy"
 version = "0.1.0"
 description = ""
 authors = ["Emerentius <emerentius@arcor.de>"]
+package-mode = false
 
 [tool.poetry.dependencies]
 python = "^3.9"

src/internal_store.rs

@@ -189,8 +189,12 @@ fn all_virtpy_backings(
         .read_dir()
         .wrap_err("failed to read virtpy dir")?
         .filter_map(|virtpy| {
-            let virtpy = virtpy.unwrap();
-            if !(virtpy.file_type().unwrap().is_dir()) {
+            let virtpy = virtpy.expect("failed to read virtpy dir");
+            if !(virtpy
+                .file_type()
+                .expect("file type of virtpy dir should be readable")
+                .is_dir())
+            {
                 return None;
             }
             let path = virtpy.utf8_path();
@@ -260,7 +264,8 @@ impl StoreDependencies {
         let mut virtpy_dists = VirtpyDists::new();
         let mut dist_virtpys: DistVirtpys = ctx
             .proj_dirs
-            .installed_distributions()
+            .installed_distributions()?
+            .into_iter()
             .map(|dist| (dist, <_>::default()))
             .collect();
         let mut dist_files = DistFiles::new();
@@ -288,13 +293,14 @@ impl StoreDependencies {
                 continue;
             };
             let distributions: HashSet<_> = distributions_used(&backing)
-                .collect::<Result<_>>()
                 .wrap_err_with(|| {
                     format!(
                         "can't read packages used by {}",
                         virtpy_link_location(&virtpy_path).unwrap_or(virtpy_path)
                     )
-                })?;
+                })?
+                .into_iter()
+                .collect();
             for dist in &distributions {
                 dist_virtpys
                     .entry(dist.clone())
@@ -451,11 +457,10 @@ pub(crate) fn print_stats(
     Ok(())
 }
 
-fn distributions_used(
-    virtpy_dirs: &VirtpyBacking,
-) -> impl Iterator<Item = Result<StoredDistribution>> {
+fn distributions_used(virtpy_dirs: &VirtpyBacking) -> Result<Vec<StoredDistribution>> {
     virtpy_dirs
-        .dist_infos()
+        .dist_infos()?
+        .into_iter()
         .filter(|dist_info_path| {
             // The intention is that only we ourselves install packages into
             // our venvs but some other tools may just see the venv structure
@@ -486,6 +491,7 @@ fn distributions_used(
             //     .map_or(true, |installer| installer.trim() == "virtpy")
         })
         .map(stored_distribution_of_installed_dist)
+        .collect()
 }
 
 pub(crate) fn stored_distribution_of_installed_dist(
@@ -497,7 +503,11 @@ pub(crate) fn stored_distribution_of_installed_dist(
 fn _stored_distribution_of_installed_dist(dist_info_path: &Path) -> Result<StoredDistribution> {
     let hash_path = dist_info_path.join(crate::DIST_HASH_FILE);
     let hash = fs_err::read_to_string(hash_path).wrap_err("failed to get distribution hash")?;
-    let (name, version) = package_info_from_dist_info_dirname(dist_info_path.file_name().unwrap());
+    let (name, version) = package_info_from_dist_info_dirname(
+        dist_info_path
+            .file_name()
+            .expect("dist_info_path should have a dirname"),
+    );
 
     Ok(StoredDistribution {
         distribution: Distribution {
@@ -598,16 +608,19 @@ impl StoredDistribution {
         // but it's only called once per package when installing it into
         // a new virtpy right now, so it doesn't matter.
         let path_in_record = PathBuf::from(self.distribution.dist_info_name()).join(file);
-        let record = WheelRecord::from_file(record_path).unwrap();
+        let record = WheelRecord::from_file(record_path).expect("wheel record should be readable");
         record
             .files
             .into_iter()
             .find(|entry| entry.path == path_in_record)
             .map(|entry| ctx.proj_dirs.package_file(&entry.hash))
     }
 
-    pub(crate) fn entrypoints(&self, ctx: &Ctx) -> Option<Vec<EntryPoint>> {
-        crate::python::entrypoints(&self.dist_info_file(ctx, "entry_points.txt")?)
+    pub(crate) fn entrypoints(&self, ctx: &Ctx) -> Result<Vec<EntryPoint>> {
+        self.dist_info_file(ctx, "entry_points.txt")
+            .map_or(Ok(vec![]), |entry_points| {
+                crate::python::entrypoints(&entry_points)
+            })
     }
 
     // Returns the directory where the RECORD of this distribution is stored.
@@ -633,8 +646,7 @@ impl StoredDistribution {
     // for the legacy pip installed distributions it is just the entrypoints.
     pub(crate) fn executable_names(&self, ctx: &Ctx) -> eyre::Result<Vec<String>> {
         let entrypoint_exes = self
-            .entrypoints(ctx)
-            .unwrap_or_default()
+            .entrypoints(ctx)?
             .into_iter()
             .map(|ep| ep.name)
             .collect::<Vec<_>>();

src/main.rs

@@ -1,3 +1,4 @@
+#![cfg_attr(not(test), deny(clippy::unwrap_used))]
 use camino::{Utf8Path, Utf8PathBuf};
 use clap::{ArgAction, Parser, Subcommand};
 use eyre::bail;
@@ -319,22 +320,22 @@ impl ProjectDirs {
         self.installations().join(format!("{package}.virtpy"))
     }
 
-    fn installed_distributions(&self) -> impl Iterator<Item = StoredDistribution> + '_ {
+    fn installed_distributions(&self) -> Result<Vec<StoredDistribution>> {
         self.records()
-            .read_dir()
-            .into_iter()
-            .flatten()
-            .map(|e| e.unwrap())
-            .map(|dist_info_entry| StoredDistribution {
-                distribution: python::Distribution::from_store_name(
-                    dist_info_entry
-                        .path()
-                        .file_name()
-                        .unwrap()
-                        .to_str()
-                        .unwrap(),
-                ),
+            .read_dir()?
+            .map(|dist_info_entry| {
+                Ok(StoredDistribution {
+                    distribution: python::Distribution::from_store_name(
+                        dist_info_entry?
+                            .path()
+                            .file_name()
+                            .expect("record file must have file name")
+                            .to_str()
+                            .expect("record path must be utf8"),
+                    ),
+                })
             })
+            .collect()
     }
 
     // Using a directory in our data directory for temporary files ensures
@@ -354,7 +355,7 @@ fn package_info_from_dist_info_dirname(dirname: &str) -> (&str, &str) {
         r"^([a-zA-Z_\.][a-zA-Z0-9_\.]*)-(\d*!.*|\d*\..*)\.dist-info$",
         dirname
     )
-    .unwrap();
+    .unwrap_or_else(|| panic!("directory name doesn't match expected pattern: {}", dirname));
     (distrib_name, version)
 }
 
@@ -364,7 +365,7 @@ fn path_to_virtpy(path_override: &Option<PathBuf>) -> &Path {
         .unwrap_or_else(|| DEFAULT_VIRTPY_PATH.as_ref())
 }
 
-fn shim_info(ctx: &Ctx) -> Result<ShimInfo> {
+fn shim_info(ctx: &Ctx) -> Result<ShimInfo<'_>> {
     Ok(ShimInfo {
         proj_dirs: &ctx.proj_dirs,
         virtpy_exe: PathBuf::try_from(
@@ -540,7 +541,7 @@ fn main() -> Result<()> {
             println!("{}", Virtpy::from_existing(&virtpy)?.global_python()?);
         }
         Command::InternalUseOnly(InternalUseOnly::ListPackages { virtpy }) => {
-            let packages = Virtpy::from_existing(&virtpy)?.installed_distributions_metadata();
+            let packages = Virtpy::from_existing(&virtpy)?.installed_distributions_metadata()?;
             let (successes, _failures): (Vec<_>, Vec<_>) = packages.into_iter().partition_result();
 
             // Print table of package name and version.
@@ -597,10 +598,11 @@ fn main() -> Result<()> {
                 .proj_dirs
                 .virtpys()
                 .read_dir()?
-                .map(|entry| entry.unwrap())
+                .map(|entry| entry.expect("virtpy dir should be readable"))
                 .filter(|entry| entry.path().join(CENTRAL_METADATA).exists())
                 .map(|entry| entry.path().join(CENTRAL_METADATA).join(LINK_LOCATION))
-                .map(|path| fs_err::read_to_string(path).unwrap())
+                // CHECKME: Is this guaranteed by construction
+                .map(|path| fs_err::read_to_string(path).expect("link location should exist"))
                 .sorted()
                 .collect_vec();
             for path in link_locations {
@@ -805,7 +807,7 @@ fn python_path(virtpy: &Path) -> PathBuf {
 }
 
 fn dist_info_matches_package(dist_info: &Path, package: &str) -> bool {
-    let entry_name = dist_info.file_name().unwrap();
+    let entry_name = dist_info.file_name().expect("should be a dir path");
     let (distrib_name, _version) = package_info_from_dist_info_dirname(entry_name);
     distrib_name == package
 }

src/prelude.rs

@@ -13,7 +13,7 @@ pub trait ToUtf8Path<'a> {
 
 impl<'a> ToUtf8Path<'a> for &'a std::path::Path {
     fn to_utf8_path(self) -> &'a Utf8Path {
-        self.try_to_utf8_path().unwrap()
+        self.try_to_utf8_path().expect("path should be utf8")
     }
 
     fn try_to_utf8_path(self) -> eyre::Result<&'a Utf8Path> {
@@ -32,7 +32,7 @@ impl IntoUtf8Pathbuf for std::path::PathBuf {
     }
 
     fn into_utf8_pathbuf(self) -> Utf8PathBuf {
-        self.try_into_utf8_pathbuf().unwrap()
+        self.try_into_utf8_pathbuf().expect("path should be utf8")
     }
 }
 
@@ -44,7 +44,7 @@ pub trait DirEntryExt {
 
 impl DirEntryExt for fs_err::DirEntry {
     fn utf8_path(&self) -> Utf8PathBuf {
-        self.try_utf8_path().unwrap()
+        self.try_utf8_path().expect("path should be utf8")
     }
 
     fn try_utf8_path(&self) -> eyre::Result<Utf8PathBuf> {
@@ -58,7 +58,7 @@ impl DirEntryExt for fs_err::DirEntry {
 
 impl DirEntryExt for std::fs::DirEntry {
     fn utf8_path(&self) -> Utf8PathBuf {
-        self.try_utf8_path().unwrap()
+        self.try_utf8_path().expect("path should be utf8")
     }
 
     fn try_utf8_path(&self) -> eyre::Result<Utf8PathBuf> {
@@ -77,7 +77,7 @@ pub trait TempDirExt<'a> {
 
 impl<'a> TempDirExt<'a> for &'a tempdir::TempDir {
     fn utf8_path(self) -> &'a Utf8Path {
-        self.try_utf8_path().unwrap()
+        self.try_utf8_path().expect("path should be utf8")
     }
 
     fn try_utf8_path(self) -> eyre::Result<&'a Utf8Path> {

src/python.rs

@@ -80,7 +80,11 @@ impl FileHash {
 
     // files in the repository are named after their hash, so we can just use the filename
     pub(crate) fn from_filename(path: &Path) -> Self {
-        Self(path.file_name().unwrap().to_owned())
+        Self(
+            path.file_name()
+                .expect("path should have filename")
+                .to_owned(),
+        )
     }
 
     pub(crate) fn from_reader(reader: impl std::io::Read) -> Self {
@@ -138,16 +142,26 @@ pub(crate) struct EntryPoint {
 
 impl EntryPoint {
     // construct from entry_points ini entry
-    pub(crate) fn new(key: &str, value: &str) -> Self {
-        let mut it = value.split(':');
-        let module = it.next().unwrap().to_owned();
-        let qualname = it.next().unwrap().to_owned();
+    // Example of typical console scripts from
+    // https://packaging.python.org/en/latest/specifications/entry-points/#file-format
+    //
+    // [console_scripts]
+    // foo = foomod:main
+    // # One which depends on extras:
+    // foobar = foomod:main_bar [bar,baz]
+    pub(crate) fn new(key: &str, value: &str) -> Result<Self> {
+        // TODO: support extras
+        let (_, module, qualname, _extras) = lazy_regex::regex_captures!(
+            r"([^:\s]+)\s*:\s*([^\s]+)(?: \[\s*([^\]]*)\s*\]\s*)?",
+            value
+        )
+        .ok_or_else(|| eyre!("console script for {key} couldn't be parsed: {value}"))?;
 
-        EntryPoint {
+        Ok(EntryPoint {
             name: key.to_owned(),
-            module,
-            qualname,
-        }
+            module: module.to_owned(),
+            qualname: qualname.to_owned(),
+        })
     }
 
     // without shebang
@@ -215,7 +229,7 @@ fn _generate_windows_executable(
     static LAUNCHER_CODE: &[u8] = include_bytes!("../windows_exe_wrappers/t64.exe");
     let mut zip_writer = zip::ZipWriter::new(std::io::Cursor::new(Vec::<u8>::new()));
     zip_writer.start_file("__main__.py", zip::write::FileOptions::default())?;
-    write!(&mut zip_writer, "{code}").unwrap();
+    write!(&mut zip_writer, "{code}").expect("failed to zip code for executable");
     let mut wrapper = LAUNCHER_CODE.to_vec();
     wrapper.extend(shebang.as_bytes());
     wrapper.extend(b".exe");
@@ -266,7 +280,8 @@ pub(crate) struct Distribution {
 impl Distribution {
     pub(crate) fn from_store_name(store_name: &str) -> Self {
         let (_, name, version, hash) =
-            lazy_regex::regex_captures!(r"([^,]+),([^,]+),([^,]+)", store_name).unwrap();
+            lazy_regex::regex_captures!(r"([^,]+),([^,]+),([^,]+)", store_name)
+                .expect("failed to get Distribution data from internal store file name");
 
         Self {
             name: name.to_owned(),
@@ -321,24 +336,24 @@ impl Distribution {
     }
 }
 
-pub(crate) fn entrypoints(path: &Path) -> Option<Vec<EntryPoint>> {
+pub(crate) fn entrypoints(path: &Path) -> Result<Vec<EntryPoint>> {
     let ini = ini::Ini::load_from_file(path);
 
     match ini {
-        Err(ini::Error::Io(err)) if is_not_found(&err) => return None,
+        Err(ini::Error::Io(err)) if is_not_found(&err) => return Ok(vec![]),
         _ => (),
     };
-    let ini = ini.unwrap();
-
-    let entrypoints = ini
-        .section(Some("console_scripts"))
-        .map_or(vec![], |console_scripts| {
-            console_scripts
-                .iter()
-                .map(|(key, val)| EntryPoint::new(key, val))
-                .collect()
-        });
-    Some(entrypoints)
+    let ini = ini?;
+
+    let entrypoints =
+        ini.section(Some("console_scripts"))
+            .map_or(Ok(vec![]), |console_scripts| {
+                console_scripts
+                    .iter()
+                    .map(|(key, val)| EntryPoint::new(key, val))
+                    .collect()
+            })?;
+    Ok(entrypoints)
 }
 
 fn hash_of_file_sha256_base64(path: &Path) -> Result<String> {
@@ -369,7 +384,7 @@ fn _hash_of_file_sha256(path: &Path) -> Result<impl AsRef<[u8]>> {
 fn _hash_of_reader_sha256(mut reader: impl std::io::Read) -> impl AsRef<[u8]> {
     use sha2::Digest;
     let mut hasher = sha2::Sha256::new();
-    std::io::copy(&mut reader, &mut hasher).unwrap();
+    std::io::copy(&mut reader, &mut hasher).expect("hashing can't fail");
     hasher.finalize()
 }
 

src/python/detection.rs

@@ -1,4 +1,4 @@
-use eyre::{bail, eyre, WrapErr};
+use eyre::{ensure, eyre, WrapErr};
 use itertools::Itertools;
 use std::path::PathBuf as StdPathBuf;
 
@@ -12,17 +12,16 @@ pub(crate) fn detect(python: &str) -> Result<PathBuf> {
     // If `python` is definitely a path, use it, if it exists.
     // For a path like 'foo/bar', .ancestors() returns "foo/bar", "foo", ""
     if path.is_absolute() || path.ancestors().take(3).count() == 3 {
-        if path.exists() {
-            return Ok(path.to_owned());
-        } else {
-            bail!("python not found at {path}");
-        }
+        ensure!(path.exists(), "python not found at {path}");
+        return Ok(path.to_owned());
     }
 
     let version_pattern = lazy_regex::regex!(r"^(\d)(\.(\d+))?$");
     if let Some(captures) = version_pattern.captures(python) {
-        let major = captures[1].parse().unwrap();
-        let minor = captures.get(3).map(|n| n.as_str().parse().unwrap());
+        let major = captures[1].parse().expect("major version parse failure");
+        let minor = captures
+            .get(3)
+            .map(|n| n.as_str().parse().expect("minor version parse failure"));
 
         return find_python_by_version(major, minor);
     }