feat(tvix/nix-compat): Use `StorePath` in `Output` r/7585

https: //b.tvl.fyi/issues/264
Change-Id: Icb09be9643245cc68d09f01d7723af2d44d6bd1a
Reviewed-on: https://cl.tvl.fyi/c/depot/+/11001
Autosubmit: Peter Kolloch <info@eigenvalue.net>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI

7 files changed, 109 insertions, 61 deletions

diff --git a/tvix/nix-compat/src/derivation/errors.rs b/tvix/nix-compat/src/derivation/errors.rs
index 7dcc3a5d0042..452231f19db2 100644
--- a/tvix/nix-compat/src/derivation/errors.rs
+++ b/tvix/nix-compat/src/derivation/errors.rs
@@ -53,6 +53,8 @@ pub enum DerivationError {
 pub enum OutputError {
     #[error("Invalid output path {0}: {1}")]
     InvalidOutputPath(String, store_path::Error),
+    #[error("Missing output path")]
+    MissingOutputPath,
     #[error("Invalid CAHash: {:?}", .0)]
     InvalidCAHash(CAHash),
 }
diff --git a/tvix/nix-compat/src/derivation/mod.rs b/tvix/nix-compat/src/derivation/mod.rs
index 2e4178b872f6..7c5afdbdcf2b 100644
--- a/tvix/nix-compat/src/derivation/mod.rs
+++ b/tvix/nix-compat/src/derivation/mod.rs
@@ -161,7 +161,13 @@ impl Derivation {
                 "fixed:out:{}{}:{}",
                 ca_kind_prefix(ca_hash),
                 ca_hash.digest().to_nix_hex_string(),
-                out_output.path
+                out_output
+                    .path
+                    .as_ref()
+                    .map(StorePath::to_absolute_path)
+                    .as_ref()
+                    .map(|s| s as &str)
+                    .unwrap_or(""),
             ))
             .finalize()
             .into(),
@@ -239,7 +245,7 @@ impl Derivation {
             // Assert that outputs are not yet populated, to avoid using this function wrongly.
             // We don't also go over self.environment, but it's a sufficient
             // footgun prevention mechanism.
-            assert!(output.path.is_empty());
+            assert!(output.path.is_none());
 
             let path_name = output_path_name(name, output_name);
 
@@ -258,7 +264,7 @@ impl Derivation {
                 })?
             };
 
-            output.path = abs_store_path.to_absolute_path();
+            output.path = Some(abs_store_path.to_owned());
             self.environment.insert(
                 output_name.to_string(),
                 abs_store_path.to_absolute_path().into(),
diff --git a/tvix/nix-compat/src/derivation/output.rs b/tvix/nix-compat/src/derivation/output.rs
index 521ea3dedc9c..e2a8282c7686 100644
--- a/tvix/nix-compat/src/derivation/output.rs
+++ b/tvix/nix-compat/src/derivation/output.rs
@@ -1,14 +1,16 @@
-use crate::derivation::OutputError;
 use crate::nixhash::CAHash;
 use crate::store_path::StorePathRef;
+use crate::{derivation::OutputError, store_path::StorePath};
+use serde::de::Unexpected;
 use serde::{Deserialize, Serialize};
 use serde_json::Map;
+use std::borrow::Cow;
 
 /// References the derivation output.
 #[derive(Clone, Debug, Default, Eq, PartialEq, Serialize)]
 pub struct Output {
     /// Store path of build result.
-    pub path: String,
+    pub path: Option<StorePath>,
 
     #[serde(flatten)]
     pub ca_hash: Option<CAHash>, // we can only represent a subset here.
@@ -20,18 +22,21 @@ impl<'de> Deserialize<'de> for Output {
         D: serde::Deserializer<'de>,
     {
         let fields = Map::deserialize(deserializer)?;
+        let path: &str = fields
+            .get("path")
+            .ok_or(serde::de::Error::missing_field(
+                "`path` is missing but required for outputs",
+            ))?
+            .as_str()
+            .ok_or(serde::de::Error::invalid_type(
+                serde::de::Unexpected::Other("certainly not a string"),
+                &"a string",
+            ))?;
+
+        let path = StorePathRef::from_absolute_path(path.as_bytes())
+            .map_err(|_| serde::de::Error::invalid_value(Unexpected::Str(path), &"StorePath"))?;
         Ok(Self {
-            path: fields
-                .get("path")
-                .ok_or(serde::de::Error::missing_field(
-                    "`path` is missing but required for outputs",
-                ))?
-                .as_str()
-                .ok_or(serde::de::Error::invalid_type(
-                    serde::de::Unexpected::Other("certainly not a string"),
-                    &"a string",
-                ))?
-                .to_owned(),
+            path: Some(path.to_owned()),
             ca_hash: CAHash::from_map::<D>(&fields)?,
         })
     }
@@ -42,6 +47,14 @@ impl Output {
         self.ca_hash.is_some()
     }
 
+    /// The output path as a string -- use `""` to indicate an unset output path.
+    pub fn path_str(&self) -> Cow<str> {
+        match &self.path {
+            None => Cow::Borrowed(""),
+            Some(path) => Cow::Owned(path.to_absolute_path()),
+        }
+    }
+
     pub fn validate(&self, validate_output_paths: bool) -> Result<(), OutputError> {
         if let Some(fixed_output_hash) = &self.ca_hash {
             match fixed_output_hash {
@@ -52,10 +65,8 @@ impl Output {
             }
         }
 
-        if validate_output_paths {
-            if let Err(e) = StorePathRef::from_absolute_path(self.path.as_bytes()) {
-                return Err(OutputError::InvalidOutputPath(self.path.to_string(), e));
-            }
+        if validate_output_paths && self.path.is_none() {
+            return Err(OutputError::MissingOutputPath);
         }
         Ok(())
     }
@@ -67,7 +78,7 @@ impl Output {
 fn deserialize_valid_input_addressed_output() {
     let json_bytes = r#"
     {
-      "path": "/nix/store/blablabla"
+      "path": "/nix/store/00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432"
     }"#;
     let output: Output = serde_json::from_str(json_bytes).expect("must parse");
 
@@ -80,7 +91,7 @@ fn deserialize_valid_input_addressed_output() {
 fn deserialize_valid_fixed_output() {
     let json_bytes = r#"
     {
-        "path": "/nix/store/blablablabla",
+        "path": "/nix/store/00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432",
         "hash": "08813cbee9903c62be4c5027726a418a300da4500b2d369d3af9286f4815ceba",
         "hashAlgo": "r:sha256"
     }"#;
@@ -95,7 +106,7 @@ fn deserialize_valid_fixed_output() {
 fn deserialize_with_error_invalid_hash_encoding_fixed_output() {
     let json_bytes = r#"
     {
-        "path": "/nix/store/blablablabla",
+        "path": "/nix/store/00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432",
         "hash": "IAMNOTVALIDNIXBASE32",
         "hashAlgo": "r:sha256"
     }"#;
@@ -110,7 +121,7 @@ fn deserialize_with_error_invalid_hash_encoding_fixed_output() {
 fn deserialize_with_error_invalid_hash_algo_fixed_output() {
     let json_bytes = r#"
     {
-        "path": "/nix/store/blablablabla",
+        "path": "/nix/store/00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432",
         "hash": "08813cbee9903c62be4c5027726a418a300da4500b2d369d3af9286f4815ceba",
         "hashAlgo": "r:sha1024"
     }"#;
@@ -125,7 +136,7 @@ fn deserialize_with_error_invalid_hash_algo_fixed_output() {
 fn deserialize_with_error_missing_hash_algo_fixed_output() {
     let json_bytes = r#"
     {
-        "path": "/nix/store/blablablabla",
+        "path": "/nix/store/00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432",
         "hash": "08813cbee9903c62be4c5027726a418a300da4500b2d369d3af9286f4815ceba",
     }"#;
     let output: Result<Output, _> = serde_json::from_str(json_bytes);
@@ -139,7 +150,7 @@ fn deserialize_with_error_missing_hash_algo_fixed_output() {
 fn deserialize_with_error_missing_hash_fixed_output() {
     let json_bytes = r#"
     {
-        "path": "/nix/store/blablablabla",
+        "path": "/nix/store/00bgd045z0d4icpbc2yyz4gx48ak44la-net-tools-1.60_p20170221182432",
         "hashAlgo": "r:sha1024"
     }"#;
     let output: Result<Output, _> = serde_json::from_str(json_bytes);
diff --git a/tvix/nix-compat/src/derivation/parser.rs b/tvix/nix-compat/src/derivation/parser.rs
index 9f5a5f090bf9..3f291c2040c3 100644
--- a/tvix/nix-compat/src/derivation/parser.rs
+++ b/tvix/nix-compat/src/derivation/parser.rs
@@ -97,7 +97,13 @@ fn parse_output(i: &[u8]) -> NomResult<&[u8], (String, Output)> {
                     Ok(hash_with_mode) => Ok((
                         output_name,
                         Output {
-                            path: output_path,
+                            // TODO: Check if allowing empty paths here actually makes sense
+                            //       or we should make this code stricter.
+                            path: if output_path.is_empty() {
+                                None
+                            } else {
+                                Some(string_to_store_path(i, output_path)?)
+                            },
                             ca_hash: hash_with_mode,
                         },
                     )),
@@ -164,21 +170,7 @@ fn parse_input_derivations(i: &[u8]) -> NomResult<&[u8], BTreeMap<StorePath, BTr
             new_output_names.insert(output_name);
         }
 
-        #[cfg(debug_assertions)]
-        let input_derivation_str = input_derivation.clone();
-
-        let input_derivation: StorePath =
-            StorePathRef::from_absolute_path(input_derivation.as_bytes())
-                .map_err(|e: store_path::Error| {
-                    nom::Err::Failure(NomError {
-                        input: i,
-                        code: e.into(),
-                    })
-                })?
-                .to_owned();
-
-        #[cfg(debug_assertions)]
-        assert_eq!(input_derivation_str, input_derivation.to_absolute_path());
+        let input_derivation: StorePath = string_to_store_path(i, input_derivation)?;
 
         input_derivations.insert(input_derivation, new_output_names);
     }
@@ -191,14 +183,7 @@ fn parse_input_sources(i: &[u8]) -> NomResult<&[u8], BTreeSet<StorePath>> {
 
     let mut input_sources: BTreeSet<_> = BTreeSet::new();
     for input_source in input_sources_lst.into_iter() {
-        let input_source: StorePath = StorePathRef::from_absolute_path(input_source.as_bytes())
-            .map_err(|e: store_path::Error| {
-                nom::Err::Failure(NomError {
-                    input: i,
-                    code: e.into(),
-                })
-            })?
-            .to_owned();
+        let input_source: StorePath = string_to_store_path(i, input_source)?;
         if input_sources.contains(&input_source) {
             return Err(nom::Err::Failure(NomError {
                 input: i,
@@ -212,6 +197,28 @@ fn parse_input_sources(i: &[u8]) -> NomResult<&[u8], BTreeSet<StorePath>> {
     Ok((i, input_sources))
 }
 
+fn string_to_store_path(
+    i: &[u8],
+    path_str: String,
+) -> Result<StorePath, nom::Err<NomError<&[u8]>>> {
+    #[cfg(debug_assertions)]
+    let path_str2 = path_str.clone();
+
+    let path: StorePath = StorePathRef::from_absolute_path(path_str.as_bytes())
+        .map_err(|e: store_path::Error| {
+            nom::Err::Failure(NomError {
+                input: i,
+                code: e.into(),
+            })
+        })?
+        .to_owned();
+
+    #[cfg(debug_assertions)]
+    assert_eq!(path_str2, path.to_absolute_path());
+
+    Ok(path)
+}
+
 pub fn parse_derivation(i: &[u8]) -> NomResult<&[u8], Derivation> {
     use nom::Parser;
     preceded(
@@ -343,15 +350,24 @@ mod tests {
             b.insert(
                 "lib".to_string(),
                 Output {
-                    path: "/nix/store/2vixb94v0hy2xc6p7mbnxxcyc095yyia-has-multi-out-lib"
-                        .to_string(),
+                    path: Some(
+                        StorePath::from_bytes(
+                            b"2vixb94v0hy2xc6p7mbnxxcyc095yyia-has-multi-out-lib",
+                        )
+                        .unwrap(),
+                    ),
                     ca_hash: None,
                 },
             );
             b.insert(
                 "out".to_string(),
                 Output {
-                    path: "/nix/store/55lwldka5nyxa08wnvlizyqw02ihy8ic-has-multi-out".to_string(),
+                    path: Some(
+                        StorePath::from_bytes(
+                            b"55lwldka5nyxa08wnvlizyqw02ihy8ic-has-multi-out".as_bytes(),
+                        )
+                        .unwrap(),
+                    ),
                     ca_hash: None,
                 },
             );
@@ -506,14 +522,17 @@ mod tests {
     #[test_case(
         br#"("out","/nix/store/5vyvcwah9l9kf07d52rcgdk70g2f4y13-foo","","")"#,
         ("out".to_string(), Output {
-            path: "/nix/store/5vyvcwah9l9kf07d52rcgdk70g2f4y13-foo".to_string(),
+            path: Some(
+                StorePathRef::from_absolute_path("/nix/store/5vyvcwah9l9kf07d52rcgdk70g2f4y13-foo".as_bytes()).unwrap().to_owned()),
             ca_hash: None
         }); "simple"
     )]
     #[test_case(
         br#"("out","/nix/store/4q0pg5zpfmznxscq3avycvf9xdvx50n3-bar","r:sha256","08813cbee9903c62be4c5027726a418a300da4500b2d369d3af9286f4815ceba")"#,
         ("out".to_string(), Output {
-            path: "/nix/store/4q0pg5zpfmznxscq3avycvf9xdvx50n3-bar".to_string(),
+            path: Some(
+                StorePathRef::from_absolute_path(
+                "/nix/store/4q0pg5zpfmznxscq3avycvf9xdvx50n3-bar".as_bytes()).unwrap().to_owned()),
             ca_hash: Some(from_algo_and_mode_and_digest("r:sha256",
                    data_encoding::HEXLOWER.decode(b"08813cbee9903c62be4c5027726a418a300da4500b2d369d3af9286f4815ceba").unwrap()            ).unwrap()),
         }); "fod"
diff --git a/tvix/nix-compat/src/derivation/tests/mod.rs b/tvix/nix-compat/src/derivation/tests/mod.rs
index fcfea2047e96..2bf09265bcda 100644
--- a/tvix/nix-compat/src/derivation/tests/mod.rs
+++ b/tvix/nix-compat/src/derivation/tests/mod.rs
@@ -170,7 +170,7 @@ fn derivation_with_trimmed_output_paths(derivation: &Derivation) -> Derivation {
         trimmed_outputs.insert(
             output_name.to_string(),
             Output {
-                path: "".to_string(),
+                path: None,
                 ..output.clone()
             },
         );
@@ -314,7 +314,7 @@ fn output_path_construction() {
     bar_drv.outputs.insert(
         "out".to_string(),
         Output {
-            path: "".to_string(), // will be calculated
+            path: None, // will be calculated
             ca_hash: Some(crate::nixhash::CAHash::Nar(
                 crate::nixhash::from_algo_and_digest(
                     crate::nixhash::HashAlgo::Sha256,
@@ -365,7 +365,16 @@ fn output_path_construction() {
 
     // assemble foo env
     let foo_env = &mut foo_drv.environment;
-    foo_env.insert("bar".to_string(), bar_output_path.to_owned().into());
+    // foo_env.insert("bar".to_string(), StorePathRef:: bar_output_path.to_owned().try_into().unwrap());
+    foo_env.insert(
+        "bar".to_string(),
+        bar_output_path
+            .as_ref()
+            .unwrap()
+            .to_absolute_path()
+            .as_bytes()
+            .into(),
+    );
     foo_env.insert("builder".to_string(), ":".into());
     foo_env.insert("name".to_string(), "foo".into());
     foo_env.insert("out".to_string(), "".into()); // will be calculated
@@ -375,7 +384,7 @@ fn output_path_construction() {
     foo_drv.outputs.insert(
         "out".to_string(),
         Output {
-            path: "".to_string(), // will be calculated
+            path: None, // will be calculated
             ca_hash: None,
         },
     );
diff --git a/tvix/nix-compat/src/derivation/validate.rs b/tvix/nix-compat/src/derivation/validate.rs
index af7dff2fdc6c..e7b24d84eed9 100644
--- a/tvix/nix-compat/src/derivation/validate.rs
+++ b/tvix/nix-compat/src/derivation/validate.rs
@@ -123,7 +123,7 @@ mod test {
         outputs.insert(
             "out".to_string(),
             Output {
-                path: "".to_string(),
+                path: None,
                 ca_hash: Some(CAHash::Text([0; 32])), // This is disallowed
             },
         );
diff --git a/tvix/nix-compat/src/derivation/write.rs b/tvix/nix-compat/src/derivation/write.rs
index 04810e736af6..a603c7a19d4e 100644
--- a/tvix/nix-compat/src/derivation/write.rs
+++ b/tvix/nix-compat/src/derivation/write.rs
@@ -132,7 +132,8 @@ pub(crate) fn write_outputs(
 
         write_char(writer, PAREN_OPEN)?;
 
-        let mut elements: Vec<&str> = vec![output_name, &output.path];
+        let path_str = output.path_str();
+        let mut elements: Vec<&str> = vec![output_name, &path_str];
 
         let (mode_and_algo, digest) = match &output.ca_hash {
             Some(ca_hash) => (