Refactor out all panics. Use anyhow instead.

Author: tombh

Cargo.lock

@@ -14,8 +14,9 @@ exclude = [
 resolver = "2"
 
 [workspace.dependencies]
+anyhow = "1.0.94"
 clap = { version = "4.4.8", features = ["derive"] }
-chrono = { version = "0.4.38", default-features = false }
+chrono = { version = "0.4.38", default-features = false, features = ["std"] }
 directories = "5.0.1"
 env_home = "0.1.0"
 env_logger = "0.10"
@@ -51,9 +52,4 @@ pattern_type_mismatch = { level = "allow", priority = 1 }
 print_stdout = { level = "allow", priority = 1 }
 std_instead_of_alloc = { level = "allow", priority = 1 }
 
-# TODO: Try to not depend on allowing these lints
-unwrap_used = { level = "allow", priority = 1 }
-get_unwrap = { level = "allow", priority = 1 }
-expect_used = { level = "allow", priority = 1 }
-panic = { level = "allow", priority = 1 }
 

Cargo.toml

@@ -0,0 +1,2 @@
+allow-unwrap-in-tests = true
+allow-panic-in-tests = true

clippy.toml

@@ -9,6 +9,7 @@ keywords = ["gpu", "compiler"]
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
 [dependencies]
+anyhow.workspace = true
 spirv-builder-cli = { path = "../spirv-builder-cli", default-features = false }
 clap.workspace = true
 directories.workspace = true

crates/cargo-gpu/Cargo.toml

@@ -2,6 +2,7 @@
 
 use std::io::Write as _;
 
+use anyhow::Context as _;
 use clap::Parser;
 use spirv_builder_cli::{Linkage, ShaderModule};
 
@@ -33,45 +34,43 @@ pub struct Build {
 
 impl Build {
     /// Entrypoint
-    pub fn run(&mut self) {
-        let (dylib_path, spirv_builder_cli_path) = self.install.run();
+    pub fn run(&mut self) -> anyhow::Result<()> {
+        let (dylib_path, spirv_builder_cli_path) = self.install.run()?;
 
         // Ensure the shader output dir exists
         log::debug!("ensuring output-dir '{}' exists", self.output_dir.display());
-        std::fs::create_dir_all(&self.output_dir).unwrap();
-        self.output_dir = self.output_dir.canonicalize().unwrap();
+        std::fs::create_dir_all(&self.output_dir)?;
+        self.output_dir = self.output_dir.canonicalize()?;
 
         // Ensure the shader crate exists
-        self.install.shader_crate = self.install.shader_crate.canonicalize().unwrap();
-        assert!(
+        self.install.shader_crate = self.install.shader_crate.canonicalize()?;
+        anyhow::ensure!(
             self.install.shader_crate.exists(),
             "shader crate '{}' does not exist. (Current dir is '{}')",
             self.install.shader_crate.display(),
-            std::env::current_dir().unwrap().display()
+            std::env::current_dir()?.display()
         );
 
         let spirv_builder_args = spirv_builder_cli::Args {
             dylib_path,
             shader_crate: self.install.shader_crate.clone(),
             shader_target: self.shader_target.clone(),
-            path_to_target_spec: target_spec_dir().join(format!("{}.json", self.shader_target)),
+            path_to_target_spec: target_spec_dir()?.join(format!("{}.json", self.shader_target)),
             no_default_features: self.no_default_features,
             features: self.features.clone(),
             output_dir: self.output_dir.clone(),
         };
 
-        // UNWRAP: safe because we know this always serializes
-        let arg = serde_json::to_string_pretty(&spirv_builder_args).unwrap();
+        let arg = serde_json::to_string_pretty(&spirv_builder_args)?;
         log::info!("using spirv-builder-cli arg: {arg}");
 
         // Call spirv-builder-cli to compile the shaders.
         let output = std::process::Command::new(spirv_builder_cli_path)
             .arg(arg)
             .stdout(std::process::Stdio::inherit())
             .stderr(std::process::Stdio::inherit())
-            .output()
-            .unwrap();
-        assert!(output.status.success(), "build failed");
+            .output()?;
+        anyhow::ensure!(output.status.success(), "build failed");
 
         let spirv_manifest = self.output_dir.join("spirv-manifest.json");
         if spirv_manifest.is_file() {
@@ -81,51 +80,52 @@ impl Build {
             );
         } else {
             log::error!("missing raw manifest '{}'", spirv_manifest.display());
-            panic!("missing raw manifest");
+            anyhow::bail!("missing raw manifest");
         }
 
         let shaders: Vec<ShaderModule> =
-            serde_json::from_reader(std::fs::File::open(&spirv_manifest).unwrap()).unwrap();
+            serde_json::from_reader(std::fs::File::open(&spirv_manifest)?)?;
 
-        let mut linkage: Vec<_> = shaders
+        let mut linkage: Vec<Linkage> = shaders
             .into_iter()
             .map(
                 |ShaderModule {
                      entry,
                      path: filepath,
-                 }| {
+                 }|
+                 -> anyhow::Result<Linkage> {
                     use relative_path::PathExt as _;
-                    let path = self.output_dir.join(filepath.file_name().unwrap());
-                    std::fs::copy(&filepath, &path).unwrap();
-                    let path_relative_to_shader_crate = path
-                        .relative_to(&self.install.shader_crate)
-                        .unwrap()
-                        .to_path("");
-                    Linkage::new(entry, path_relative_to_shader_crate)
+                    let path = self.output_dir.join(
+                        filepath
+                            .file_name()
+                            .context("Couldn't parse file name from shader module path")?,
+                    );
+                    std::fs::copy(&filepath, &path)?;
+                    let path_relative_to_shader_crate =
+                        path.relative_to(&self.install.shader_crate)?.to_path("");
+                    Ok(Linkage::new(entry, path_relative_to_shader_crate))
                 },
             )
-            .collect();
+            .collect::<anyhow::Result<Vec<Linkage>>>()?;
 
         // Write the shader manifest json file
         let manifest_path = self.output_dir.join("manifest.json");
         // Sort the contents so the output is deterministic
         linkage.sort();
         // UNWRAP: safe because we know this always serializes
-        let json = serde_json::to_string_pretty(&linkage).unwrap();
-        let mut file = std::fs::File::create(&manifest_path).unwrap_or_else(|error| {
-            log::error!(
-                "could not create shader manifest file '{}': {error}",
+        let json = serde_json::to_string_pretty(&linkage)?;
+        let mut file = std::fs::File::create(&manifest_path).with_context(|| {
+            format!(
+                "could not create shader manifest file '{}'",
                 manifest_path.display(),
-            );
-            panic!("{error}")
-        });
-        file.write_all(json.as_bytes()).unwrap_or_else(|error| {
-            log::error!(
-                "could not write shader manifest file '{}': {error}",
+            )
+        })?;
+        file.write_all(json.as_bytes()).with_context(|| {
+            format!(
+                "could not write shader manifest file '{}'",
                 manifest_path.display(),
-            );
-            panic!("{error}")
-        });
+            )
+        })?;
 
         log::info!("wrote manifest to '{}'", manifest_path.display());
 
@@ -134,8 +134,10 @@ impl Build {
                 "removing spirv-manifest.json file '{}'",
                 spirv_manifest.display()
             );
-            std::fs::remove_file(spirv_manifest).unwrap();
+            std::fs::remove_file(spirv_manifest)?;
         }
+
+        Ok(())
     }
 }
 

crates/cargo-gpu/src/build.rs

@@ -1,6 +1,8 @@
 //! Install a dedicated per-shader crate that has the `rust-gpu` compiler in it.
 use std::io::Write as _;
 
+use anyhow::Context as _;
+
 use crate::{cache_dir, spirv_cli::SpirvCli, spirv_source::SpirvSource, target_spec_dir};
 
 /// These are the files needed to create the dedicated, per-shader `rust-gpu` builder create.
@@ -120,7 +122,7 @@ pub struct Install {
 
 impl Install {
     /// Returns a [`SpirvCLI`] instance, responsible for ensuring the right version of the `spirv-builder-cli` crate.
-    fn spirv_cli(&self, shader_crate_path: &std::path::PathBuf) -> SpirvCli {
+    fn spirv_cli(&self, shader_crate_path: &std::path::PathBuf) -> anyhow::Result<SpirvCli> {
         SpirvCli::new(
             shader_crate_path,
             self.spirv_builder_source.clone(),
@@ -130,20 +132,21 @@ impl Install {
     }
 
     /// Create the `spirv-builder-cli` crate.
-    fn write_source_files(&self) {
-        let spirv_cli = self.spirv_cli(&self.shader_crate);
-        let checkout = spirv_cli.cached_checkout_path();
-        std::fs::create_dir_all(checkout.join("src")).unwrap();
+    fn write_source_files(&self) -> anyhow::Result<()> {
+        let spirv_cli = self.spirv_cli(&self.shader_crate)?;
+        let checkout = spirv_cli.cached_checkout_path()?;
+        std::fs::create_dir_all(checkout.join("src"))?;
         for (filename, contents) in SPIRV_BUILDER_FILES {
             log::debug!("writing {filename}");
             let path = checkout.join(filename);
-            let mut file = std::fs::File::create(&path).unwrap();
+            let mut file = std::fs::File::create(&path)?;
             let mut replaced_contents = contents.replace("${CHANNEL}", &spirv_cli.channel);
             if filename == &"Cargo.toml" {
                 replaced_contents = Self::update_cargo_toml(&replaced_contents, &spirv_cli.source);
             }
-            file.write_all(replaced_contents.as_bytes()).unwrap();
+            file.write_all(replaced_contents.as_bytes())?;
         }
+        Ok(())
     }
 
     /// Update  the `Cargo.toml` file in the `spirv-builder-cli` crate so that it contains
@@ -176,33 +179,30 @@ impl Install {
     }
 
     /// Add the target spec files to the crate.
-    fn write_target_spec_files(&self) {
+    fn write_target_spec_files(&self) -> anyhow::Result<()> {
         for (filename, contents) in TARGET_SPECS {
-            let path = target_spec_dir().join(filename);
+            let path = target_spec_dir()?.join(filename);
             if !path.is_file() || self.force_spirv_cli_rebuild {
-                let mut file = std::fs::File::create(&path).unwrap();
-                file.write_all(contents.as_bytes()).unwrap();
+                let mut file = std::fs::File::create(&path)?;
+                file.write_all(contents.as_bytes())?;
             }
         }
+        Ok(())
     }
 
     /// Install the binary pair and return the paths, (dylib, cli).
-    pub fn run(&self) -> (std::path::PathBuf, std::path::PathBuf) {
+    pub fn run(&self) -> anyhow::Result<(std::path::PathBuf, std::path::PathBuf)> {
         // Ensure the cache dir exists
-        let cache_dir = cache_dir();
+        let cache_dir = cache_dir()?;
         log::info!("cache directory is '{}'", cache_dir.display());
-        std::fs::create_dir_all(&cache_dir).unwrap_or_else(|error| {
-            log::error!(
-                "could not create cache directory '{}': {error}",
-                cache_dir.display()
-            );
-            panic!("could not create cache dir");
-        });
+        std::fs::create_dir_all(&cache_dir).with_context(|| {
+            format!("could not create cache directory '{}'", cache_dir.display())
+        })?;
 
-        let spirv_version = self.spirv_cli(&self.shader_crate);
-        spirv_version.ensure_toolchain_and_components_exist();
+        let spirv_version = self.spirv_cli(&self.shader_crate)?;
+        spirv_version.ensure_toolchain_and_components_exist()?;
 
-        let checkout = spirv_version.cached_checkout_path();
+        let checkout = spirv_version.cached_checkout_path()?;
         let release = checkout.join("target").join("release");
 
         let dylib_filename = format!(
@@ -227,8 +227,8 @@ impl Install {
                 "writing spirv-builder-cli source files into '{}'",
                 checkout.display()
             );
-            self.write_source_files();
-            self.write_target_spec_files();
+            self.write_source_files()?;
+            self.write_target_spec_files()?;
 
             let mut command = std::process::Command::new("cargo");
             command
@@ -239,24 +239,23 @@ impl Install {
 
             command.args([
                 "--features",
-                &Self::get_required_spirv_builder_version(spirv_version.date),
+                &Self::get_required_spirv_builder_version(spirv_version.date)?,
             ]);
 
             log::debug!("building artifacts with `{:?}`", command);
 
             let output = command
                 .stdout(std::process::Stdio::inherit())
                 .stderr(std::process::Stdio::inherit())
-                .output()
-                .unwrap();
-            assert!(output.status.success(), "...build error!");
+                .output()?;
+            anyhow::ensure!(output.status.success(), "...build error!");
 
             if dylib_path.is_file() {
                 log::info!("successfully built {}", dylib_path.display());
-                std::fs::rename(&dylib_path, &dest_dylib_path).unwrap();
+                std::fs::rename(&dylib_path, &dest_dylib_path)?;
             } else {
                 log::error!("could not find {}", dylib_path.display());
-                panic!("spirv-builder-cli build failed");
+                anyhow::bail!("spirv-builder-cli build failed");
             }
 
             let cli_path = if cfg!(target_os = "windows") {
@@ -266,18 +265,18 @@ impl Install {
             };
             if cli_path.is_file() {
                 log::info!("successfully built {}", cli_path.display());
-                std::fs::rename(&cli_path, &dest_cli_path).unwrap();
+                std::fs::rename(&cli_path, &dest_cli_path)?;
             } else {
                 log::error!("could not find {}", cli_path.display());
                 log::debug!("contents of '{}':", release.display());
-                for maybe_entry in std::fs::read_dir(&release).unwrap() {
-                    let entry = maybe_entry.unwrap();
+                for maybe_entry in std::fs::read_dir(&release)? {
+                    let entry = maybe_entry?;
                     log::debug!("{}", entry.file_name().to_string_lossy());
                 }
-                panic!("spirv-builder-cli build failed");
+                anyhow::bail!("spirv-builder-cli build failed");
             }
         }
-        (dest_dylib_path, dest_cli_path)
+        Ok((dest_dylib_path, dest_cli_path))
     }
 
     /// The `spirv-builder` crate from the main `rust-gpu` repo hasn't always been setup to
@@ -287,15 +286,15 @@ impl Install {
     /// TODO:
     ///   * Warn the user that certain `cargo-gpu` features aren't available when building with
     ///     older versions of `spirv-builder`, eg setting the target spec.
-    fn get_required_spirv_builder_version(date: chrono::NaiveDate) -> String {
+    fn get_required_spirv_builder_version(date: chrono::NaiveDate) -> anyhow::Result<String> {
         let parse_date = chrono::NaiveDate::parse_from_str;
-        let pre_cli_date = parse_date("2024-04-24", "%Y-%m-%d").unwrap();
+        let pre_cli_date = parse_date("2024-04-24", "%Y-%m-%d")?;
 
-        if date < pre_cli_date {
+        Ok(if date < pre_cli_date {
             "spirv-builder-pre-cli"
         } else {
             "spirv-builder-0_10"
         }
-        .into()
+        .into())
     }
 }