refactor(tvix/derivation): expose proper ValidateDerivationError r/5668

Use proper errors, instead of anyhow.

Change-Id: I6db14c72a6319b389b0136aac7b84f50a30fb366
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7847
Autosubmit: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>

7 files changed, 109 insertions, 30 deletions

diff --git a/tvix/Cargo.lock b/tvix/Cargo.lock
index 37e1a0f139..36c9cef50f 100644
--- a/tvix/Cargo.lock
+++ b/tvix/Cargo.lock
@@ -2288,6 +2288,7 @@ dependencies = [
  "sha2",
  "test-case",
  "test-generator",
+ "thiserror",
  "tvix-store-bin",
 ]
 
diff --git a/tvix/Cargo.nix b/tvix/Cargo.nix
index 5fca533d1c..a27fe822f2 100644
--- a/tvix/Cargo.nix
+++ b/tvix/Cargo.nix
@@ -6791,6 +6791,10 @@ rec {
             packageId = "sha2";
           }
           {
+            name = "thiserror";
+            packageId = "thiserror";
+          }
+          {
             name = "tvix-store-bin";
             packageId = "tvix-store-bin";
           }
diff --git a/tvix/derivation/Cargo.toml b/tvix/derivation/Cargo.toml
index 2d5ea74bdc..12afcf92c1 100644
--- a/tvix/derivation/Cargo.toml
+++ b/tvix/derivation/Cargo.toml
@@ -10,6 +10,7 @@ anyhow = "1.0.68"
 glob = "0.3.0"
 serde = { version = "1.0", features = ["derive"] }
 sha2 = "0.10.6"
+thiserror = "1.0.38"
 tvix-store-bin = { path = "../store" }
 
 [dev-dependencies.test-generator]
diff --git a/tvix/derivation/src/errors.rs b/tvix/derivation/src/errors.rs
new file mode 100644
index 0000000000..a43162c146
--- /dev/null
+++ b/tvix/derivation/src/errors.rs
@@ -0,0 +1,44 @@
+use thiserror::Error;
+use tvix_store::store_path::ParseStorePathError;
+
+/// Errors that can occur during the validation of Derivation structs.
+#[derive(Debug, Error)]
+pub enum ValidateDerivationError {
+    // outputs
+    #[error("No outputs defined.")]
+    NoOutputs(),
+    #[error("Invalid output name: {0}.")]
+    InvalidOutputName(String),
+    #[error("Encountered fixed-output derivation, but more than 1 output in total.")]
+    MoreThanOneOutputButFixed(),
+    #[error("Invalid output name for fixed-output derivation: {0}.")]
+    InvalidOutputNameForFixed(String),
+    #[error("Unable to parse path of output {0}: {1}.")]
+    InvalidOutputPath(String, ParseStorePathError),
+
+    // input derivation
+    #[error("Unable to parse input derivation path {0}: {1}.")]
+    InvalidInputDerivationPath(String, ParseStorePathError),
+    #[error("Input Derivation {0} doesn't end with .drv.")]
+    InvalidInputDerivationPrefix(String),
+    #[error("Input Derivation {0} output names are empty.")]
+    EmptyInputDerivationOutputNames(String),
+    #[error("Input Derivation {0} output name {1} is invalid.")]
+    InvalidInputDerivationOutputName(String, String),
+
+    // input sources
+    #[error("Unable to parse input sources path {0}: {1}.")]
+    InvalidInputSourcesPath(String, ParseStorePathError),
+
+    // platform
+    #[error("Invalid platform field: {0}")]
+    InvalidPlatform(String),
+
+    // builder
+    #[error("Invalid builder field: {0}")]
+    InvalidBuilder(String),
+
+    // environment
+    #[error("Invalid environment key {0}")]
+    InvalidEnvironmentKey(String),
+}
diff --git a/tvix/derivation/src/lib.rs b/tvix/derivation/src/lib.rs
index 0729ba0a6c..00a8e45022 100644
--- a/tvix/derivation/src/lib.rs
+++ b/tvix/derivation/src/lib.rs
@@ -1,4 +1,5 @@
 mod derivation;
+mod errors;
 mod nix_hash;
 mod output;
 mod string_escape;
@@ -11,4 +12,5 @@ mod tests;
 // Public API of the crate.
 
 pub use derivation::Derivation;
+pub use errors::ValidateDerivationError;
 pub use output::{Hash, Output};
diff --git a/tvix/derivation/src/output.rs b/tvix/derivation/src/output.rs
index 1236cd989f..cd77c00f2f 100644
--- a/tvix/derivation/src/output.rs
+++ b/tvix/derivation/src/output.rs
@@ -1,5 +1,7 @@
 use serde::{Deserialize, Serialize};
-use tvix_store::store_path::StorePath;
+use tvix_store::store_path::{ParseStorePathError, StorePath};
+
+use crate::ValidateDerivationError;
 
 #[derive(Clone, Debug, Default, Eq, PartialEq, Serialize, Deserialize)]
 pub struct Output {
@@ -22,8 +24,10 @@ impl Output {
         self.hash.is_some()
     }
 
-    pub fn validate(&self) -> anyhow::Result<()> {
-        StorePath::from_absolute_path(&self.path)?;
+    pub fn validate(&self) -> Result<(), ParseStorePathError> {
+        if let Err(e) = StorePath::from_absolute_path(&self.path) {
+            return Err(e);
+        }
         Ok(())
     }
 }
diff --git a/tvix/derivation/src/validate.rs b/tvix/derivation/src/validate.rs
index e225bd2fdb..2ff23d5204 100644
--- a/tvix/derivation/src/validate.rs
+++ b/tvix/derivation/src/validate.rs
@@ -1,85 +1,108 @@
-use crate::{derivation::Derivation, write::DOT_FILE_EXT};
-use anyhow::bail;
+use crate::{derivation::Derivation, write::DOT_FILE_EXT, ValidateDerivationError};
 use tvix_store::store_path::StorePath;
 
 impl Derivation {
     /// validate ensures a Derivation struct is properly populated,
-    /// and returns an error if not.
-    /// TODO(flokli): make this proper errors
-    pub fn validate(&self) -> anyhow::Result<()> {
+    /// and returns a [ValidateDerivationError] if not.
+    pub fn validate(&self) -> Result<(), ValidateDerivationError> {
         // Ensure the number of outputs is > 1
         if self.outputs.is_empty() {
-            bail!("0 outputs");
+            return Err(ValidateDerivationError::NoOutputs());
         }
 
         // Validate all outputs
         for (output_name, output) in &self.outputs {
             if output_name.is_empty() {
-                bail!("output_name from outputs may not be empty")
+                return Err(ValidateDerivationError::InvalidOutputName(
+                    output_name.to_string(),
+                ));
             }
 
             if output.is_fixed() {
                 if self.outputs.len() != 1 {
-                    bail!("encountered fixed-output, but there's more than 1 output in total");
+                    return Err(ValidateDerivationError::MoreThanOneOutputButFixed());
                 }
                 if output_name != "out" {
-                    bail!("the fixed-output output name must be called 'out'");
+                    return Err(ValidateDerivationError::InvalidOutputNameForFixed(
+                        output_name.to_string(),
+                    ));
                 }
 
                 break;
             }
 
-            output.validate()?;
+            if let Err(e) = output.validate() {
+                return Err(ValidateDerivationError::InvalidOutputPath(
+                    output_name.to_string(),
+                    e,
+                ));
+            };
         }
 
         // Validate all input_derivations
         for (input_derivation_path, output_names) in &self.input_derivations {
             // Validate input_derivation_path
-            StorePath::from_absolute_path(input_derivation_path)?;
+            if let Err(e) = StorePath::from_absolute_path(input_derivation_path) {
+                return Err(ValidateDerivationError::InvalidInputDerivationPath(
+                    input_derivation_path.to_string(),
+                    e,
+                ));
+            }
+
             if !input_derivation_path.ends_with(DOT_FILE_EXT) {
-                bail!(
-                    "derivation {} does not end with .drv",
-                    input_derivation_path
-                );
+                return Err(ValidateDerivationError::InvalidInputDerivationPrefix(
+                    input_derivation_path.to_string(),
+                ));
             }
 
             if output_names.is_empty() {
-                bail!(
-                    "output_names list for {} may not be empty",
-                    input_derivation_path
-                );
+                return Err(ValidateDerivationError::EmptyInputDerivationOutputNames(
+                    input_derivation_path.to_string(),
+                ));
             }
 
             for output_name in output_names.iter() {
                 if output_name.is_empty() {
-                    bail!(
-                        "output name entry for {} may not be empty",
-                        input_derivation_path
-                    )
+                    return Err(ValidateDerivationError::InvalidInputDerivationOutputName(
+                        input_derivation_path.to_string(),
+                        output_name.to_string(),
+                    ));
                 }
+                // TODO: do we need to apply more name validation here?
             }
         }
 
         // Validate all input_sources
         for input_source in self.input_sources.iter() {
-            StorePath::from_absolute_path(input_source)?;
+            if let Err(e) = StorePath::from_absolute_path(input_source) {
+                return Err(ValidateDerivationError::InvalidInputSourcesPath(
+                    input_source.to_string(),
+                    e,
+                ));
+            }
         }
 
         // validate platform
         if self.system.is_empty() {
-            bail!("required attribute 'platform' missing");
+            return Err(ValidateDerivationError::InvalidPlatform(
+                self.system.to_string(),
+            ));
         }
 
         // validate builder
         if self.builder.is_empty() {
-            bail!("required attribute 'builder' missing");
+            return Err(ValidateDerivationError::InvalidBuilder(
+                self.builder.to_string(),
+            ));
         }
 
         // validate env, none of the keys may be empty.
         // We skip the `name` validation seen in go-nix.
         for k in self.environment.keys() {
             if k.is_empty() {
-                bail!("found empty environment variable key");
+                return Err(ValidateDerivationError::InvalidEnvironmentKey(
+                    k.to_string(),
+                ));
             }
         }