refactor(tvix/nix-compat): move build_store_path out of derivation

This doesn't have anything to do with ATerms, we just happen to be using
the aterm representation of a Derivation as contents.

Moving this into store_path/utils.rs makes these things much cleaner -
Have a build_store_path_from_references function, and a
build_store_path_from_fingerprint helper function that makes use of it.

build_store_path_from_references is invoked from the derivation module
which can be used to calculate the derivation path.

In the derivation module, we also invoke
build_store_path_from_fingerprint during the output path calculation.

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

tvix/cli/src/derivation.rs

@@ -1,6 +1,6 @@
 //! Implements `builtins.derivation`, the core of what makes Nix build packages.
 use nix_compat::derivation::Derivation;
-use nix_compat::{hash_placeholder, nixhash};
+use nix_compat::nixhash;
 use std::cell::RefCell;
 use std::collections::{btree_map, BTreeSet};
 use std::rc::Rc;
@@ -202,6 +202,7 @@ async fn strong_coerce_to_string(co: &GenCo, val: Value) -> Result<String, Error
 #[builtins(state = "Rc<RefCell<KnownPaths>>")]
 mod derivation_builtins {
     use super::*;
+    use nix_compat::store_path::hash_placeholder;
     use tvix_eval::generators::Gen;
 
     #[builtin("placeholder")]
@@ -411,10 +412,16 @@ mod derivation_builtins {
 
         // TODO: fail on derivation references (only "plain" is allowed here)
 
-        let path =
+        let path = nix_compat::store_path::build_store_path_from_references(
-            nix_compat::derivation::path_with_references(name.as_str(), content.as_str(), refs)
+            name.as_str(),
-                .map_err(Error::InvalidDerivation)?
+            content.as_str(),
-                .to_absolute_path();
+            refs,
+        )
+        .map_err(|_e| {
+            nix_compat::derivation::DerivationError::InvalidOutputName(name.as_str().to_string())
+        })
+        .map_err(Error::InvalidDerivation)?
+        .to_absolute_path();
 
         state.borrow_mut().plain(&path);
 
@@ -428,7 +435,7 @@ pub use derivation_builtins::builtins as derivation_builtins;
 
 #[cfg(test)]
 mod tests {
-    use super::*;
+    use nix_compat::store_path::hash_placeholder;
     use tvix_eval::observer::NoOpObserver;
 
     // TODO: These tests are commented out because we do not have

tvix/nix-compat/src/derivation/errors.rs

@@ -1,4 +1,4 @@
-use crate::{nixbase32::Nixbase32DecodeError, store_path::ParseStorePathError};
+use crate::{nixbase32::Nixbase32DecodeError, store_path};
 use thiserror::Error;
 
 /// Errors that can occur during the validation of Derivation structs.
@@ -17,7 +17,7 @@ pub enum DerivationError {
     InvalidOutput(String, OutputError),
     // input derivation
     #[error("unable to parse input derivation path {0}: {1}")]
-    InvalidInputDerivationPath(String, ParseStorePathError),
+    InvalidInputDerivationPath(String, store_path::Error),
     #[error("input derivation {0} doesn't end with .drv")]
     InvalidInputDerivationPrefix(String),
     #[error("input derivation {0} output names are empty")]
@@ -27,7 +27,7 @@ pub enum DerivationError {
 
     // input sources
     #[error("unable to parse input sources path {0}: {1}")]
-    InvalidInputSourcesPath(String, ParseStorePathError),
+    InvalidInputSourcesPath(String, store_path::Error),
 
     // platform
     #[error("invalid platform field: {0}")]
@@ -46,8 +46,8 @@ pub enum DerivationError {
 // [crate::derivation::Output] of a [crate::derivation::Derviation].
 #[derive(Debug, Error, PartialEq)]
 pub enum OutputError {
-    #[error("Invalid ouput path {0}: {1}")]
+    #[error("Invalid output path {0}: {1}")]
-    InvalidOutputPath(String, ParseStorePathError),
+    InvalidOutputPath(String, store_path::Error),
     #[error("Invalid hash encoding: {0}")]
     InvalidHashEncoding(String, Nixbase32DecodeError),
     #[error("Invalid hash algo: {0}")]

tvix/nix-compat/src/derivation/mod.rs

@@ -1,7 +1,8 @@
 use crate::{
     nixhash::HashAlgo,
-    store_path::{self, StorePath},
+    store_path::{
-    texthash::text_hash_string,
+        self, build_store_path_from_fingerprint, build_store_path_from_references, StorePath,
+    },
 };
 use serde::{Deserialize, Serialize};
 use sha2::{Digest, Sha256};
@@ -10,7 +11,6 @@ use std::collections::{BTreeMap, BTreeSet};
 mod errors;
 mod output;
 mod string_escape;
-mod utils;
 mod validate;
 mod write;
 
@@ -21,7 +21,6 @@ mod tests;
 pub use crate::nixhash::{NixHash, NixHashWithMode};
 pub use errors::{DerivationError, OutputError};
 pub use output::Output;
-pub use utils::path_with_references;
 
 #[derive(Clone, Debug, Default, Eq, PartialEq, Serialize, Deserialize)]
 pub struct Derivation {
@@ -77,12 +76,12 @@ impl Derivation {
         buffer
     }
 
-    /// Returns the drv path of a Derivation struct.
+    /// Returns the drv path of a [Derivation] struct.
     ///
-    /// The drv path is calculated by calculating the [text_hash_string], using
+    /// The drv path is calculated by invoking [build_store_path_from_references], using
-    /// the `name` with a `.drv` suffix as name, all d.InputDerivations and d.InputSources as references,
+    /// the `name` with a `.drv` suffix as name, all [Derivation::input_sources] and
-    /// and the ATerm representation of the Derivation as contents.
+    /// keys of [Derivation::input_derivations] as references, and the ATerm string of
-    /// The text_hash_string is then passed to the build_store_path function.
+    /// the [Derivation] as content.
     pub fn calculate_derivation_path(&self, name: &str) -> Result<StorePath, DerivationError> {
         // append .drv to the name
         let name = &format!("{}.drv", name);
@@ -97,9 +96,8 @@ impl Derivation {
             inputs
         };
 
-        let text_hash_str = &text_hash_string(name, self.to_aterm_string(), references);
+        build_store_path_from_references(name, self.to_aterm_string(), references)
-
+            .map_err(|_e| DerivationError::InvalidOutputName(name.to_string()))
-        utils::build_store_path(text_hash_str, name)
     }
 
     /// Returns the FOD digest, if the derivation is fixed-output, or None if
@@ -249,8 +247,10 @@ impl Derivation {
                 store_path::STORE_DIR,
                 output_path_name,
             ));
-            let abs_store_path =
+
-                utils::build_store_path(&fp, &output_path_name)?.to_absolute_path();
+            let abs_store_path = build_store_path_from_fingerprint(&output_path_name, &fp)
+                .map_err(|_e| DerivationError::InvalidOutputName(output_path_name.to_string()))?
+                .to_absolute_path();
 
             output.path = abs_store_path.clone();
             self.environment

tvix/nix-compat/src/derivation/tests/mod.rs

@@ -1,7 +1,7 @@
 use crate::derivation::output::Output;
 use crate::derivation::Derivation;
 use crate::nixhash::NixHash;
-use crate::store_path::StorePath;
+use crate::store_path::{build_store_path_from_references, StorePath};
 use std::collections::BTreeSet;
 use std::fs::File;
 use std::io::Read;
@@ -313,40 +313,3 @@ fn output_path_construction() {
             .expect("must succeed")
     );
 }
-
-#[test]
-fn path_with_zero_references() {
-    // This hash should match `builtins.toFile`, e.g.:
-    //
-    // nix-repl> builtins.toFile "foo" "bar"
-    // "/nix/store/vxjiwkjkn7x4079qvh1jkl5pn05j2aw0-foo"
-
-    let store_path = crate::derivation::path_with_references("foo", "bar", Vec::<String>::new())
-        .expect("path_with_references() should succeed");
-
-    assert_eq!(
-        store_path.to_absolute_path().as_str(),
-        "/nix/store/vxjiwkjkn7x4079qvh1jkl5pn05j2aw0-foo"
-    );
-}
-
-#[test]
-fn path_with_non_zero_references() {
-    // This hash should match:
-    //
-    // nix-repl> builtins.toFile "baz" "${builtins.toFile "foo" "bar"}"
-    // "/nix/store/5xd714cbfnkz02h2vbsj4fm03x3f15nf-baz"
-
-    let inner = crate::derivation::path_with_references("foo", "bar", Vec::<String>::new())
-        .expect("path_with_references() should succeed");
-    let inner_path = inner.to_absolute_path();
-
-    let outer =
-        crate::derivation::path_with_references("baz", &inner_path, vec![inner_path.as_str()])
-            .expect("path_with_references() should succeed");
-
-    assert_eq!(
-        outer.to_absolute_path().as_str(),
-        "/nix/store/5xd714cbfnkz02h2vbsj4fm03x3f15nf-baz"
-    );
-}

tvix/nix-compat/src/derivation/utils.rs

@@ -1,54 +0,0 @@
-use crate::derivation::DerivationError;
-use crate::nixbase32;
-use crate::store_path::StorePath;
-use crate::texthash::text_hash_string;
-use sha2::{Digest, Sha256};
-
-/// compress_hash takes an arbitrarily long sequence of bytes (usually
-/// a hash digest), and returns a sequence of bytes of length
-/// output_size.
-///
-/// It's calculated by rotating through the bytes in the output buffer
-/// (zero- initialized), and XOR'ing with each byte of the passed
-/// input. It consumes 1 byte at a time, and XOR's it with the current
-/// value in the output buffer.
-///
-/// This mimics equivalent functionality in C++ Nix.
-fn compress_hash(input: &[u8], output_size: usize) -> Vec<u8> {
-    let mut output: Vec<u8> = vec![0; output_size];
-
-    for (ii, ch) in input.iter().enumerate() {
-        output[ii % output_size] ^= ch;
-    }
-
-    output
-}
-
-/// This returns a store path, either of a derivation or a regular output.
-/// The string is hashed with sha256, its digest is compressed to 20 bytes, and
-/// nixbase32-encoded (32 characters)
-pub(super) fn build_store_path(
-    fingerprint: &str,
-    name: &str,
-) -> Result<StorePath, DerivationError> {
-    let digest = {
-        let hasher = Sha256::new_with_prefix(fingerprint);
-        hasher.finalize()
-    };
-    let compressed = compress_hash(&digest, 20);
-    StorePath::from_string(format!("{}-{}", nixbase32::encode(&compressed), name,).as_str())
-        .map_err(|_e| DerivationError::InvalidOutputName(name.to_string()))
-    // Constructing the StorePath can only fail if the passed output name was
-    // invalid, so map errors to a [DerivationError::InvalidOutputName].
-}
-
-/// Build a store path for a literal text file in the store that may
-/// contain references.
-pub fn path_with_references<S: AsRef<str>, I: IntoIterator<Item = S>, C: AsRef<[u8]>>(
-    name: &str,
-    content: C,
-    references: I,
-) -> Result<StorePath, DerivationError> {
-    let text_hash_str = text_hash_string(name, content, references);
-    build_store_path(&text_hash_str, name)
-}

tvix/nix-compat/src/lib.rs

@@ -1,5 +1,3 @@
-use sha2::{Digest, Sha256};
-
 pub mod derivation;
 pub mod nar;
 pub mod nixbase32;
@@ -8,19 +6,3 @@ mod nixhash_algos;
 mod nixhash_with_mode;
 pub mod store_path;
 mod texthash;
-
-/// Nix placeholders (i.e. values returned by `builtins.placeholder`)
-/// are used to populate outputs with paths that must be
-/// string-replaced with the actual placeholders later, at runtime.
-///
-/// The actual placeholder is basically just a SHA256 hash encoded in
-/// cppnix format.
-pub fn hash_placeholder(name: &str) -> String {
-    let digest = {
-        let mut hasher = Sha256::new();
-        hasher.update(format!("nix-output:{}", name));
-        hasher.finalize()
-    };
-
-    format!("/{}", nixbase32::encode(&digest))
-}

tvix/nix-compat/src/store_path/mod.rs

@@ -2,6 +2,13 @@ use crate::nixbase32::{self, Nixbase32DecodeError};
 use std::fmt;
 use thiserror::Error;
 
+mod utils;
+
+pub use utils::{
+    build_store_path_from_fingerprint, build_store_path_from_references, compress_hash,
+    hash_placeholder,
+};
+
 pub const DIGEST_SIZE: usize = 20;
 // lazy_static doesn't allow us to call NIXBASE32.encode_len(), so we ran it
 // manually and have an assert in the tests.
@@ -14,7 +21,7 @@ pub const STORE_DIR_WITH_SLASH: &str = "/nix/store/";
 
 /// Errors that can occur during the validation of name characters.
 #[derive(Debug, PartialEq, Eq, Error)]
-pub enum ParseStorePathError {
+pub enum Error {
     #[error("Dash is missing between hash and name")]
     MissingDash(),
     #[error("Hash encoding is invalid: {0}")]
@@ -42,25 +49,23 @@ pub struct StorePath {
 }
 
 impl StorePath {
-    pub fn from_string(s: &str) -> Result<StorePath, ParseStorePathError> {
+    pub fn from_string(s: &str) -> Result<StorePath, Error> {
         // the whole string needs to be at least:
         //
         // - 32 characters (encoded hash)
         // - 1 dash
         // - 1 character for the name
         if s.len() < ENCODED_DIGEST_SIZE + 2 {
-            return Err(ParseStorePathError::InvalidName("".to_string()));
+            return Err(Error::InvalidName("".to_string()));
         }
 
         let digest = match nixbase32::decode(s[..ENCODED_DIGEST_SIZE].as_bytes()) {
             Ok(decoded) => decoded,
-            Err(decoder_error) => {
+            Err(decoder_error) => return Err(Error::InvalidHashEncoding(decoder_error)),
-                return Err(ParseStorePathError::InvalidHashEncoding(decoder_error))
-            }
         };
 
         if s.as_bytes()[ENCODED_DIGEST_SIZE] != b'-' {
-            return Err(ParseStorePathError::MissingDash());
+            return Err(Error::MissingDash());
         }
 
         StorePath::validate_name(&s[ENCODED_DIGEST_SIZE + 2..])?;
@@ -73,10 +78,10 @@ impl StorePath {
 
     /// Construct a [StorePath] from an absolute store path string.
     /// That is a string starting with the store prefix (/nix/store)
-    pub fn from_absolute_path(s: &str) -> Result<StorePath, ParseStorePathError> {
+    pub fn from_absolute_path(s: &str) -> Result<StorePath, Error> {
         match s.strip_prefix(STORE_DIR_WITH_SLASH) {
             Some(s_stripped) => Self::from_string(s_stripped),
-            None => Err(ParseStorePathError::MissingStoreDir()),
+            None => Err(Error::MissingStoreDir()),
         }
     }
 
@@ -87,7 +92,7 @@ impl StorePath {
     }
 
     /// Checks a given &str to match the restrictions for store path names.
-    pub fn validate_name(s: &str) -> Result<(), ParseStorePathError> {
+    pub fn validate_name(s: &str) -> Result<(), Error> {
         for c in s.chars() {
             if c.is_ascii_alphanumeric()
                 || c == '-'
@@ -100,7 +105,7 @@ impl StorePath {
                 continue;
             }
 
-            return Err(ParseStorePathError::InvalidName(s.to_string()));
+            return Err(Error::InvalidName(s.to_string()));
         }
 
         Ok(())
@@ -118,7 +123,7 @@ mod tests {
     use crate::nixbase32;
     use crate::store_path::{DIGEST_SIZE, ENCODED_DIGEST_SIZE};
 
-    use super::{ParseStorePathError, StorePath};
+    use super::{Error, StorePath};
 
     #[test]
     fn encoded_digest_size() {
@@ -191,7 +196,7 @@ mod tests {
     #[test]
     fn absolute_path_missing_prefix() {
         assert_eq!(
-            ParseStorePathError::MissingStoreDir(),
+            Error::MissingStoreDir(),
             StorePath::from_absolute_path("foobar-123").expect_err("must fail")
         );
     }

tvix/nix-compat/src/store_path/utils.rs

@@ -0,0 +1,118 @@
+use crate::nixbase32;
+use crate::store_path::StorePath;
+use crate::texthash::text_hash_string;
+use sha2::{Digest, Sha256};
+
+use super::Error;
+
+/// compress_hash takes an arbitrarily long sequence of bytes (usually
+/// a hash digest), and returns a sequence of bytes of length
+/// output_size.
+///
+/// It's calculated by rotating through the bytes in the output buffer
+/// (zero- initialized), and XOR'ing with each byte of the passed
+/// input. It consumes 1 byte at a time, and XOR's it with the current
+/// value in the output buffer.
+///
+/// This mimics equivalent functionality in C++ Nix.
+pub fn compress_hash(input: &[u8], output_size: usize) -> Vec<u8> {
+    let mut output: Vec<u8> = vec![0; output_size];
+
+    for (ii, ch) in input.iter().enumerate() {
+        output[ii % output_size] ^= ch;
+    }
+
+    output
+}
+
+/// This builds a store path, by calculating the text_hash_string of either a
+/// derivation or a literal text file that may contain references.
+pub fn build_store_path_from_references<
+    S: AsRef<str>,
+    I: IntoIterator<Item = S>,
+    C: AsRef<[u8]>,
+>(
+    name: &str,
+    content: C,
+    references: I,
+) -> Result<StorePath, Error> {
+    let text_hash_str = text_hash_string(name, content, references);
+    build_store_path_from_fingerprint(name, &text_hash_str)
+}
+
+/// This builds a store path from a fingerprint.
+/// Usually, that function is used from [build_store_path_from_references] and
+/// passed a "text hash string" (starting with "text:" as fingerprint),
+/// but other fingerprints starting with "output:" are also used in Derivation
+/// output path calculation.
+///
+/// The fingerprint is hashed with sha256, its digest is compressed to 20 bytes,
+/// and nixbase32-encoded (32 characters).
+pub fn build_store_path_from_fingerprint(
+    name: &str,
+    fingerprint: &str,
+) -> Result<StorePath, Error> {
+    let digest = {
+        let hasher = Sha256::new_with_prefix(fingerprint);
+        hasher.finalize()
+    };
+    let compressed = compress_hash(&digest, 20);
+    StorePath::from_string(format!("{}-{}", nixbase32::encode(&compressed), name).as_str())
+}
+
+/// Nix placeholders (i.e. values returned by `builtins.placeholder`)
+/// are used to populate outputs with paths that must be
+/// string-replaced with the actual placeholders later, at runtime.
+///
+/// The actual placeholder is basically just a SHA256 hash encoded in
+/// cppnix format.
+pub fn hash_placeholder(name: &str) -> String {
+    let digest = {
+        let mut hasher = Sha256::new();
+        hasher.update(format!("nix-output:{}", name));
+        hasher.finalize()
+    };
+
+    format!("/{}", nixbase32::encode(&digest))
+}
+
+#[cfg(test)]
+mod test {
+    use crate::store_path::build_store_path_from_references;
+
+    #[test]
+    fn build_store_path_with_zero_references() {
+        // This hash should match `builtins.toFile`, e.g.:
+        //
+        // nix-repl> builtins.toFile "foo" "bar"
+        // "/nix/store/vxjiwkjkn7x4079qvh1jkl5pn05j2aw0-foo"
+
+        let store_path = build_store_path_from_references("foo", "bar", Vec::<String>::new())
+            .expect("build_store_path() should succeed");
+
+        assert_eq!(
+            store_path.to_absolute_path().as_str(),
+            "/nix/store/vxjiwkjkn7x4079qvh1jkl5pn05j2aw0-foo"
+        );
+    }
+
+    #[test]
+    fn build_store_path_with_non_zero_references() {
+        // This hash should match:
+        //
+        // nix-repl> builtins.toFile "baz" "${builtins.toFile "foo" "bar"}"
+        // "/nix/store/5xd714cbfnkz02h2vbsj4fm03x3f15nf-baz"
+
+        let inner = build_store_path_from_references("foo", "bar", Vec::<String>::new())
+            .expect("path_with_references() should succeed");
+        let inner_path = inner.to_absolute_path();
+
+        let outer = build_store_path_from_references("baz", &inner_path, vec![inner_path.as_str()])
+            .expect("path_with_references() should succeed");
+
+        assert_eq!(
+            outer.to_absolute_path().as_str(),
+            "/nix/store/5xd714cbfnkz02h2vbsj4fm03x3f15nf-baz"
+        );
+    }
+}

tvix/store/src/proto/mod.rs

@@ -5,7 +5,7 @@ use thiserror::Error;
 
 use prost::Message;
 
-use nix_compat::store_path::{ParseStorePathError, StorePath};
+use nix_compat::store_path::{self, StorePath};
 
 mod grpc_blobservice_wrapper;
 mod grpc_directoryservice_wrapper;
@@ -52,7 +52,7 @@ pub enum ValidatePathInfoError {
 
     /// Invalid node name encountered.
     #[error("Failed to parse {0} as NixPath: {1}")]
-    InvalidNodeName(String, ParseStorePathError),
+    InvalidNodeName(String, store_path::Error),
 
     /// The digest the (root) node refers to has invalid length.
     #[error("Invalid Digest length: {0}")]
@@ -91,7 +91,7 @@ fn validate_digest<E>(digest: &Vec<u8>, err: fn(usize) -> E) -> Result<(), E> {
 /// On error, it returns an error generated from the supplied constructor.
 fn parse_node_name_root<E>(
     name: &str,
-    err: fn(String, ParseStorePathError) -> E,
+    err: fn(String, store_path::Error) -> E,
 ) -> Result<StorePath, E> {
     match StorePath::from_string(name) {
         Ok(np) => Ok(np),

tvix/store/src/proto/tests/pathinfo.rs

@@ -1,6 +1,6 @@
 use crate::proto::{self, Node, PathInfo, ValidatePathInfoError};
 use lazy_static::lazy_static;
-use nix_compat::store_path::{ParseStorePathError, StorePath};
+use nix_compat::store_path::{self, StorePath};
 use test_case::test_case;
 
 lazy_static! {
@@ -66,7 +66,7 @@ fn validate_no_node(
     },
     Err(ValidatePathInfoError::InvalidNodeName(
         "invalid".to_string(),
-        ParseStorePathError::InvalidName("".to_string())
+        store_path::Error::InvalidName("".to_string())
     ));
     "invalid node name"
 )]
@@ -111,7 +111,7 @@ fn validate_directory(
     },
     Err(ValidatePathInfoError::InvalidNodeName(
         "invalid".to_string(),
-        ParseStorePathError::InvalidName("".to_string())
+        store_path::Error::InvalidName("".to_string())
     ));
     "invalid node name"
 )]
@@ -141,7 +141,7 @@ fn validate_file(t_file_node: proto::FileNode, t_result: Result<StorePath, Valid
     },
     Err(ValidatePathInfoError::InvalidNodeName(
         "invalid".to_string(),
-        ParseStorePathError::InvalidName("".to_string())
+        store_path::Error::InvalidName("".to_string())
     ));
     "invalid node name"
 )]