refactor(tvix/cli): absorb construct_output_hash r/5783

This helper function only was created because
populate_output_configuration was hard to test before cl/7939.

With that out of the way, we can pull it in.

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

4 files changed, 116 insertions, 100 deletions

diff --git a/tvix/Cargo.lock b/tvix/Cargo.lock
index b7d478a779..7519a52660 100644
--- a/tvix/Cargo.lock
+++ b/tvix/Cargo.lock
@@ -2614,7 +2614,6 @@ dependencies = [
  "rustyline",
  "smol_str",
  "ssri",
- "test-case",
  "thiserror",
  "tvix-derivation",
  "tvix-eval",
diff --git a/tvix/Cargo.nix b/tvix/Cargo.nix
index 8255a395fc..e0895ac928 100644
--- a/tvix/Cargo.nix
+++ b/tvix/Cargo.nix
@@ -7750,12 +7750,6 @@ rec {
             packageId = "tvix-store-bin";
           }
         ];
-        devDependencies = [
-          {
-            name = "test-case";
-            packageId = "test-case";
-          }
-        ];
 
       };
       "tvix-derivation" = rec {
diff --git a/tvix/cli/Cargo.toml b/tvix/cli/Cargo.toml
index 56d95f7460..42d2102c4b 100644
--- a/tvix/cli/Cargo.toml
+++ b/tvix/cli/Cargo.toml
@@ -19,6 +19,3 @@ aho-corasick = "0.7"
 ssri = "7.0.0"
 data-encoding = "2.3.3"
 thiserror = "1.0.38"
-
-[dev-dependencies]
-test-case = "2.2.2"
diff --git a/tvix/cli/src/derivation.rs b/tvix/cli/src/derivation.rs
index 17211baa1c..3eafc85591 100644
--- a/tvix/cli/src/derivation.rs
+++ b/tvix/cli/src/derivation.rs
@@ -80,6 +80,13 @@ fn populate_inputs<I: IntoIterator<Item = String>>(
     }
 }
 
+/// Populate the output configuration of a derivation based on the
+/// parameters passed to the call, flipping the required
+/// parameters for a fixed-output derivation if necessary.
+///
+/// This function handles all possible combinations of the
+/// parameters, including invalid ones.
+///
 /// Due to the support for SRI hashes, and how these are passed along to
 /// builtins.derivation, outputHash and outputHashAlgo can have values which
 /// need to be further modified before constructing the Derivation struct.
@@ -93,75 +100,6 @@ fn populate_inputs<I: IntoIterator<Item = String>>(
 /// (lowercase) hex encoding of the digest.
 ///
 /// These values are only rewritten for the outputs, not what's passed to env.
-fn construct_output_hash(digest: &str, algo: &str, hash_mode: Option<&str>) -> Result<Hash, Error> {
-    let sri_parsed = digest.parse::<ssri::Integrity>();
-    // SRI strings can embed multiple hashes with different algos, but that's probably not supported
-
-    let (digest, algo): (String, String) = match sri_parsed {
-        Err(e) => {
-            // unable to parse as SRI, but algo not set
-            if algo.is_empty() {
-                // InvalidSRIString doesn't implement PartialEq, but our error does
-                return Err(Error::InvalidSRIString(e.to_string()));
-            }
-
-            // algo is set. Assume the digest is set to some nixbase32.
-            // TODO: more validation here
-
-            (digest.to_string(), algo.to_string())
-        }
-        Ok(sri_parsed) => {
-            // We don't support more than one SRI hash
-            if sri_parsed.hashes.len() != 1 {
-                return Err(Error::UnsupportedSRIMultiple(sri_parsed.hashes.len()).into());
-            }
-
-            // grab the first (and only hash)
-            let sri_parsed_hash = &sri_parsed.hashes[0];
-
-            // ensure the algorithm in the SRI is supported
-            if !(sri_parsed_hash.algorithm == ssri::Algorithm::Sha1
-                || sri_parsed_hash.algorithm == ssri::Algorithm::Sha256
-                || sri_parsed_hash.algorithm == ssri::Algorithm::Sha512)
-            {
-                Error::UnsupportedSRIAlgo(sri_parsed_hash.algorithm.to_string());
-            }
-
-            // if algo is set, it needs to match what the SRI says
-            if !algo.is_empty() && algo != sri_parsed_hash.algorithm.to_string() {
-                return Err(Error::ConflictingSRIHashAlgo(
-                    algo.to_string(),
-                    sri_parsed_hash.algorithm.to_string(),
-                ));
-            }
-
-            // the digest comes base64-encoded. We need to decode, and re-encode as hexlower.
-            match BASE64.decode(sri_parsed_hash.digest.as_bytes()) {
-                Err(e) => return Err(Error::InvalidSRIDigest(e).into()),
-                Ok(sri_digest) => (
-                    data_encoding::HEXLOWER.encode(&sri_digest),
-                    sri_parsed_hash.algorithm.to_string(),
-                ),
-            }
-        }
-    };
-
-    // mutate the algo string a bit more, depending on hashMode
-    let algo = match hash_mode {
-        None | Some("flat") => algo,
-        Some("recursive") => format!("r:{}", algo),
-        Some(other) => return Err(Error::InvalidOutputHashMode(other.to_string()).into()),
-    };
-
-    Ok(Hash { algo, digest })
-}
-
-/// Populate the output configuration of a derivation based on the
-/// parameters passed to the call, flipping the required
-/// parameters for a fixed-output derivation if necessary.
-///
-/// This function handles all possible combinations of the
-/// parameters, including invalid ones.
 fn populate_output_configuration(
     drv: &mut Derivation,
     hash: Option<String>,      // in nix: outputHash
@@ -172,7 +110,71 @@ fn populate_output_configuration(
         (Some(digest), Some(algo), hash_mode) => match drv.outputs.get_mut("out") {
             None => return Err(Error::ConflictingOutputTypes.into()),
             Some(out) => {
-                out.hash = Some(construct_output_hash(&digest, &algo, hash_mode.as_deref())?);
+                let sri_parsed = digest.parse::<ssri::Integrity>();
+                // SRI strings can embed multiple hashes with different algos, but that's probably not supported
+
+                let (digest, algo): (String, String) = match sri_parsed {
+                    Err(e) => {
+                        // unable to parse as SRI, but algo not set
+                        if algo.is_empty() {
+                            // InvalidSRIString doesn't implement PartialEq, but our error does
+                            return Err(Error::InvalidSRIString(e.to_string()).into());
+                        }
+
+                        // algo is set. Assume the digest is set to some nixbase32.
+                        // TODO: more validation here
+
+                        (digest.to_string(), algo.to_string())
+                    }
+                    Ok(sri_parsed) => {
+                        // We don't support more than one SRI hash
+                        if sri_parsed.hashes.len() != 1 {
+                            return Err(
+                                Error::UnsupportedSRIMultiple(sri_parsed.hashes.len()).into()
+                            );
+                        }
+
+                        // grab the first (and only hash)
+                        let sri_parsed_hash = &sri_parsed.hashes[0];
+
+                        // ensure the algorithm in the SRI is supported
+                        if !(sri_parsed_hash.algorithm == ssri::Algorithm::Sha1
+                            || sri_parsed_hash.algorithm == ssri::Algorithm::Sha256
+                            || sri_parsed_hash.algorithm == ssri::Algorithm::Sha512)
+                        {
+                            Error::UnsupportedSRIAlgo(sri_parsed_hash.algorithm.to_string());
+                        }
+
+                        // if algo is set, it needs to match what the SRI says
+                        if !algo.is_empty() && algo != sri_parsed_hash.algorithm.to_string() {
+                            return Err(Error::ConflictingSRIHashAlgo(
+                                algo.to_string(),
+                                sri_parsed_hash.algorithm.to_string(),
+                            )
+                            .into());
+                        }
+
+                        // the digest comes base64-encoded. We need to decode, and re-encode as hexlower.
+                        match BASE64.decode(sri_parsed_hash.digest.as_bytes()) {
+                            Err(e) => return Err(Error::InvalidSRIDigest(e).into()),
+                            Ok(sri_digest) => (
+                                data_encoding::HEXLOWER.encode(&sri_digest),
+                                sri_parsed_hash.algorithm.to_string(),
+                            ),
+                        }
+                    }
+                };
+
+                // mutate the algo string a bit more, depending on hashMode
+                let algo = match hash_mode.as_deref() {
+                    None | Some("flat") => algo,
+                    Some("recursive") => format!("r:{}", algo),
+                    Some(other) => {
+                        return Err(Error::InvalidOutputHashMode(other.to_string()).into())
+                    }
+                };
+
+                out.hash = Some(Hash { algo, digest });
             }
         },
 
@@ -431,7 +433,6 @@ pub use derivation_builtins::builtins as derivation_builtins;
 #[cfg(test)]
 mod tests {
     use super::*;
-    use test_case::test_case;
     use tvix_eval::observer::NoOpObserver;
 
     static mut OBSERVER: NoOpObserver = NoOpObserver {};
@@ -584,6 +585,50 @@ mod tests {
     }
 
     #[test]
+    /// hash_algo set to sha256, but SRI hash passed
+    fn populate_output_config_flat_sri_sha256() {
+        let mut drv = Derivation::default();
+        drv.outputs.insert("out".to_string(), Default::default());
+
+        populate_output_configuration(
+            &mut drv,
+            Some("sha256-swapHA/ZO8QoDPwumMt6s5gf91oYe+oyk4EfRSyJqMg=".into()),
+            Some("sha256".into()),
+            Some("flat".into()),
+        )
+        .expect("populate_output_configuration() should succeed");
+
+        let expected = Hash {
+            algo: "sha256".into(),
+            digest: "b306a91c0fd93bc4280cfc2e98cb7ab3981ff75a187bea3293811f452c89a8c8".into(), // lower hex
+        };
+
+        assert_eq!(drv.outputs["out"].hash, Some(expected));
+    }
+
+    #[test]
+    /// hash_algo set to empty string, SRI hash passed
+    fn populate_output_config_flat_sri() {
+        let mut drv = Derivation::default();
+        drv.outputs.insert("out".to_string(), Default::default());
+
+        populate_output_configuration(
+            &mut drv,
+            Some("sha256-s6JN6XqP28g1uYMxaVAQMLiXcDG8tUs7OsE3QPhGqzA=".into()),
+            Some("".into()),
+            Some("flat".into()),
+        )
+        .expect("populate_output_configuration() should succeed");
+
+        let expected = Hash {
+            algo: "sha256".into(),
+            digest: "b3a24de97a8fdbc835b9833169501030b8977031bcb54b3b3ac13740f846ab30".into(), // lower hex
+        };
+
+        assert_eq!(drv.outputs["out"].hash, Some(expected));
+    }
+
+    #[test]
     fn handle_outputs_parameter() {
         let mut vm = fake_vm();
         let mut drv = Derivation::default();
@@ -631,23 +676,4 @@ mod tests {
             vec!["--foo".to_string(), "42".to_string(), "--bar".to_string()]
         );
     }
-
-    #[test_case(
-        "sha256-swapHA/ZO8QoDPwumMt6s5gf91oYe+oyk4EfRSyJqMg=", "sha256", Some("flat"),
-        Ok(Hash { algo: "sha256".to_string(), digest: "b306a91c0fd93bc4280cfc2e98cb7ab3981ff75a187bea3293811f452c89a8c8".to_string() });
-        "sha256 and SRI"
-    )]
-    #[test_case(
-        "sha256-s6JN6XqP28g1uYMxaVAQMLiXcDG8tUs7OsE3QPhGqzA=", "", Some("flat"),
-        Ok(Hash { algo: "sha256".to_string(), digest: "b3a24de97a8fdbc835b9833169501030b8977031bcb54b3b3ac13740f846ab30".to_string() });
-        "SRI only"
-    )]
-    fn test_construct_output_hash(
-        digest: &str,
-        algo: &str,
-        hash_mode: Option<&str>,
-        result: Result<Hash, Error>,
-    ) {
-        assert_eq!(construct_output_hash(digest, algo, hash_mode), result);
-    }
 }