From af4a3b9b4eb1d89012f6c9ccf2ae8899c9926626 Mon Sep 17 00:00:00 2001 From: CosmicHorror Date: Fri, 24 Nov 2023 11:57:45 -0700 Subject: [PATCH] Error cleanup (#46) * `ParseErrorInner` should **not** be part of the public API * `LibaryFolders` -> `LibraryFolders` * Make `Error` non-exhaustive * Add path to io error * Add path to parse error * Remove erroneous print * Fixup error for missing app installation --- src/error.rs | 56 +++++++++++++++++++++++++++++++++---------- src/lib.rs | 5 ++++ src/libraryfolders.rs | 16 +++++-------- src/shortcut.rs | 17 +++++++++---- src/steamapp.rs | 46 ++++++++++++++++++++++------------- 5 files changed, 97 insertions(+), 43 deletions(-) diff --git a/src/error.rs b/src/error.rs index dd3996a..f80f99e 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,35 +1,55 @@ -use std::fmt; +use std::{ + fmt, + path::{Path, PathBuf}, +}; pub type Result = std::result::Result; #[derive(Debug)] +#[non_exhaustive] pub enum Error { FailedLocatingSteamDir, - // TODO: make more specific and associate with a path? - Io(std::io::Error), - // TODO: associate with a path + Io { + inner: std::io::Error, + path: PathBuf, + }, Parse { kind: ParseErrorKind, error: ParseError, + path: PathBuf, }, MissingExpectedApp { app_id: u32, }, + MissingAppInstall { + app_id: u32, + path: PathBuf, + }, } impl fmt::Display for Error { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Self::FailedLocatingSteamDir => f.write_str("Failed locating the steam dir"), - Self::Io(err) => write!(f, "Encountered an I/O error: {}", err), - Self::Parse { kind, error } => write!( + Self::Io { inner: err, path } => { + write!(f, "Encountered an I/O error: {} at {}", err, path.display()) + } + Self::Parse { kind, error, path } => write!( f, - "Failed parsing VDF file. File kind: {:?}, Error: {}", - kind, error + "Failed parsing VDF file. File kind: {:?}, Error: {} at {}", + kind, + error, + path.display(), ), Self::MissingExpectedApp { app_id } => { write!(f, "Missing expected app with id: {}", app_id) } + Self::MissingAppInstall { app_id, path } => write!( + f, + "Missing expected app installation with id: {} at {}", + app_id, + path.display(), + ), } } } @@ -37,16 +57,26 @@ impl fmt::Display for Error { impl std::error::Error for Error {} impl Error { - pub(crate) fn parse(kind: ParseErrorKind, error: ParseError) -> Self { - Self::Parse { kind, error } + pub(crate) fn io(io: std::io::Error, path: &Path) -> Self { + Self::Io { + inner: io, + path: path.to_owned(), + } + } + + pub(crate) fn parse(kind: ParseErrorKind, error: ParseError, path: &Path) -> Self { + Self::Parse { + kind, + error, + path: path.to_owned(), + } } } #[derive(Copy, Clone, Debug)] #[non_exhaustive] pub enum ParseErrorKind { - // FIXME(cosmic): this is misspelled ;-; - LibaryFolders, + LibraryFolders, SteamApp, Shortcut, } @@ -66,7 +96,7 @@ impl fmt::Display for ParseError { } #[derive(Debug)] -pub enum ParseErrorInner { +pub(crate) enum ParseErrorInner { Parse(keyvalues_parser::error::Error), Serde(keyvalues_serde::error::Error), UnexpectedStructure, diff --git a/src/lib.rs b/src/lib.rs index 9174492..807277a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -115,6 +115,11 @@ //! } //! ``` +#![warn( + // We're a library after all + clippy::print_stderr, clippy::print_stdout +)] + #[cfg(not(any(target_os = "windows", target_os = "macos", target_os = "linux")))] compile_error!("Unsupported operating system!"); diff --git a/src/libraryfolders.rs b/src/libraryfolders.rs index c6720ef..be36186 100644 --- a/src/libraryfolders.rs +++ b/src/libraryfolders.rs @@ -40,13 +40,13 @@ use keyvalues_parser::Vdf; /// } /// ``` pub fn parse_library_folders(path: &Path) -> Result { - let parse_error = |err| Error::parse(ParseErrorKind::LibaryFolders, err); + let parse_error = |err| Error::parse(ParseErrorKind::LibraryFolders, err, path); if !path.is_file() { return Err(parse_error(ParseError::missing())); } - let contents = fs::read_to_string(path).map_err(Error::Io)?; + let contents = fs::read_to_string(path).map_err(|io| Error::io(io, path))?; let value = Vdf::parse(&contents) .map_err(|err| parse_error(ParseError::from_parser(err)))? .value; @@ -63,12 +63,7 @@ pub fn parse_library_folders(path: &Path) -> Result { .and_then(|obj| obj.get("path")) .and_then(|values| values.first()) .and_then(|value| value.get_str()) - .ok_or_else(|| { - Error::parse( - ParseErrorKind::LibaryFolders, - ParseError::unexpected_structure(), - ) - }) + .ok_or_else(|| parse_error(ParseError::unexpected_structure())) .map(PathBuf::from) }) .collect::>()?; @@ -107,8 +102,9 @@ impl Library { // Read the manifest files at the library to get an up-to-date list of apps since the // values in `libraryfolders.vdf` may be stale let mut apps = Vec::new(); - for entry in fs::read_dir(path.join("steamapps")).map_err(Error::Io)? { - let entry = entry.map_err(Error::Io)?; + let steamapps = path.join("steamapps"); + for entry in fs::read_dir(&steamapps).map_err(|io| Error::io(io, &steamapps))? { + let entry = entry.map_err(|io| Error::io(io, &steamapps))?; if let Some(id) = entry .file_name() .to_str() diff --git a/src/shortcut.rs b/src/shortcut.rs index 9f22596..a30e77b 100644 --- a/src/shortcut.rs +++ b/src/shortcut.rs @@ -1,6 +1,11 @@ //! **WARN:** This is all hacky and should be replaced with proper binary VDF parsing -use std::{fs, io, iter::Peekable, path::Path, slice::Iter}; +use std::{ + fs, io, + iter::Peekable, + path::{Path, PathBuf}, + slice::Iter, +}; use crate::{error::ParseErrorKind, Error, ParseError, Result}; @@ -38,6 +43,7 @@ impl Shortcut { } pub struct ShortcutIter { + dir: PathBuf, read_dir: fs::ReadDir, pending: std::vec::IntoIter, } @@ -49,11 +55,13 @@ impl ShortcutIter { return Err(Error::parse( ParseErrorKind::Shortcut, ParseError::missing(), + &user_data, )); } - let read_dir = fs::read_dir(user_data).map_err(Error::Io)?; + let read_dir = fs::read_dir(&user_data).map_err(|io| Error::io(io, &user_data))?; Ok(Self { + dir: user_data, read_dir, pending: Vec::new().into_iter(), }) @@ -83,6 +91,7 @@ impl Iterator for ShortcutIter { break Err(Error::parse( ParseErrorKind::Shortcut, ParseError::unexpected_structure(), + &shortcuts_path, )); } } @@ -91,12 +100,12 @@ impl Iterator for ShortcutIter { if err.kind() == io::ErrorKind::NotFound { continue; } else { - break Err(Error::Io(err)); + break Err(Error::io(err, &shortcuts_path)); } } } } - Err(err) => break Err(Error::Io(err)), + Err(err) => break Err(Error::io(err, &self.dir)), } }; diff --git a/src/steamapp.rs b/src/steamapp.rs index 2c36a52..9067c84 100644 --- a/src/steamapp.rs +++ b/src/steamapp.rs @@ -77,21 +77,33 @@ pub struct SteamApp { impl SteamApp { pub(crate) fn new(library_path: &Path, manifest: &Path) -> Result { - let contents = fs::read_to_string(manifest).map_err(Error::Io)?; - let app = Self::from_manifest_str(library_path, &contents)?; + let contents = fs::read_to_string(manifest).map_err(|io| Error::io(io, manifest))?; + let internal = keyvalues_serde::from_str(&contents).map_err(|err| { + Error::parse( + ParseErrorKind::SteamApp, + ParseError::from_serde(err), + manifest, + ) + })?; + let app = Self::from_internal_steam_app(internal, library_path); // Check if the installation path exists and is a valid directory - // TODO: lint against printing - println!("{:?}", app.path); + // TODO: this one check really shapes a lot of the API (in terms of how the data for the + // `SteamApp` is resolved. Maybe move this to something like + // ```rust + // library.resolve_install_dir(&app)?; + // ``` if app.path.is_dir() { Ok(app) } else { - // TODO: app id here - Err(Error::MissingExpectedApp { app_id: 1 }) + Err(Error::MissingAppInstall { + app_id: app.app_id, + path: app.path, + }) } } - pub(crate) fn from_manifest_str(library_path: &Path, manifest: &str) -> Result { + pub(crate) fn from_internal_steam_app(internal: InternalSteamApp, library_path: &Path) -> Self { let InternalSteamApp { app_id, universe, @@ -121,10 +133,7 @@ impl SteamApp { mounted_config, install_scripts, shared_depots, - } = keyvalues_serde::from_str(manifest).map_err(|err| Error::Parse { - kind: ParseErrorKind::SteamApp, - error: ParseError::from_serde(err), - })?; + } = internal; let path = library_path .join("steamapps") @@ -148,7 +157,7 @@ impl SteamApp { let full_validate_before_next_update = full_validate_before_next_update.unwrap_or_default(); let full_validate_after_next_update = full_validate_after_next_update.unwrap_or_default(); - Ok(Self { + Self { app_id, universe, launcher_path, @@ -177,7 +186,7 @@ impl SteamApp { mounted_config, install_scripts, shared_depots, - }) + } } } @@ -348,7 +357,7 @@ pub struct Depot { } #[derive(Debug, Deserialize)] -struct InternalSteamApp { +pub(crate) struct InternalSteamApp { #[serde(rename = "appid")] app_id: u32, #[serde(rename = "installdir")] @@ -410,10 +419,15 @@ struct InternalSteamApp { mod tests { use super::*; + fn app_from_manifest_str(s: &str, library_path: &Path) -> SteamApp { + let internal: InternalSteamApp = keyvalues_serde::from_str(s).unwrap(); + SteamApp::from_internal_steam_app(internal, library_path) + } + #[test] fn sanity() { let manifest = include_str!("../tests/assets/appmanifest_230410.acf"); - let app = SteamApp::from_manifest_str(Path::new("C:\\redact\\me"), manifest).unwrap(); + let app = app_from_manifest_str(manifest, Path::new("C:\\redact\\me")); // Redact the path because the path separator used is not cross-platform insta::assert_ron_snapshot!(app, { ".path" => "[path]" }); } @@ -421,7 +435,7 @@ mod tests { #[test] fn more_sanity() { let manifest = include_str!("../tests/assets/appmanifest_599140.acf"); - let app = SteamApp::from_manifest_str(Path::new("/redact/me"), manifest).unwrap(); + let app = app_from_manifest_str(manifest, Path::new("/redact/me")); insta::assert_ron_snapshot!(app, { ".path" => "[path]" }); } }