From fde488ec6dc444561ae353f979d87c8ae87261fb Mon Sep 17 00:00:00 2001 From: Peter Kolloch Date: Wed, 21 Feb 2024 18:31:35 +0700 Subject: feat(tvix/nix-compat): Use `StorePath` in `Output` https: //b.tvl.fyi/issues/264 Change-Id: Icb09be9643245cc68d09f01d7723af2d44d6bd1a Reviewed-on: https://cl.tvl.fyi/c/depot/+/11001 Autosubmit: Peter Kolloch Reviewed-by: flokli Tested-by: BuildkiteCI --- tvix/glue/src/builtins/derivation.rs | 4 +- tvix/glue/src/known_paths.rs | 8 +-- tvix/glue/src/tvix_build.rs | 2 +- tvix/glue/src/tvix_store_io.rs | 17 +++---- tvix/nix-compat/src/derivation/errors.rs | 2 + tvix/nix-compat/src/derivation/mod.rs | 12 +++-- tvix/nix-compat/src/derivation/output.rs | 57 ++++++++++++--------- tvix/nix-compat/src/derivation/parser.rs | 77 ++++++++++++++++++----------- tvix/nix-compat/src/derivation/tests/mod.rs | 17 +++++-- tvix/nix-compat/src/derivation/validate.rs | 2 +- tvix/nix-compat/src/derivation/write.rs | 3 +- 11 files changed, 119 insertions(+), 82 deletions(-) (limited to 'tvix') diff --git a/tvix/glue/src/builtins/derivation.rs b/tvix/glue/src/builtins/derivation.rs index 87ae8bb946bd..b597d20211e9 100644 --- a/tvix/glue/src/builtins/derivation.rs +++ b/tvix/glue/src/builtins/derivation.rs @@ -116,7 +116,7 @@ fn handle_fixed_output( drv.outputs.insert( "out".to_string(), Output { - path: "".to_string(), + path: None, ca_hash: match hash_mode_str.as_deref() { None | Some("flat") => Some(nixhash::CAHash::Flat(nixhash)), Some("recursive") => Some(nixhash::CAHash::Nar(nixhash)), @@ -486,7 +486,7 @@ pub(crate) mod derivation_builtins { ( name.clone(), ( - output.path, + output.path.unwrap().to_absolute_path(), Some( NixContextElement::Single { name, diff --git a/tvix/glue/src/known_paths.rs b/tvix/glue/src/known_paths.rs index bac7e34a7ec4..13f86fae0ea4 100644 --- a/tvix/glue/src/known_paths.rs +++ b/tvix/glue/src/known_paths.rs @@ -74,14 +74,8 @@ impl KnownPaths { // For all output paths, update our lookup table. // We only write into the lookup table once. for output in drv.outputs.values() { - // We assume derivations to be passed validated, so ignoring rest - // and expecting parsing is ok. - // TODO: b/264 - let (output_path, _rest) = - StorePath::from_absolute_path_full(&output.path).expect("parse output path"); - self.outputs_to_drvpath - .entry(output_path) + .entry(output.path.as_ref().expect("missing store path").clone()) .or_insert(drv_path.to_owned()); } diff --git a/tvix/glue/src/tvix_build.rs b/tvix/glue/src/tvix_build.rs index dc48987bd9d2..e9eb1725ef3e 100644 --- a/tvix/glue/src/tvix_build.rs +++ b/tvix/glue/src/tvix_build.rs @@ -52,7 +52,7 @@ pub(crate) fn derivation_to_build_request( let mut output_paths: Vec = derivation .outputs .values() - .map(|e| e.path[1..].to_owned()) + .map(|e| e.path_str()[1..].to_owned()) .collect(); // Sort the outputs. We can use sort_unstable, as these are unique strings. diff --git a/tvix/glue/src/tvix_store_io.rs b/tvix/glue/src/tvix_store_io.rs index 296a369e29c1..333b04b170c7 100644 --- a/tvix/glue/src/tvix_store_io.rs +++ b/tvix/glue/src/tvix_store_io.rs @@ -4,10 +4,7 @@ use async_recursion::async_recursion; use bytes::Bytes; use futures::Stream; use futures::{StreamExt, TryStreamExt}; -use nix_compat::{ - nixhash::CAHash, - store_path::{StorePath, StorePathRef}, -}; +use nix_compat::{nixhash::CAHash, store_path::StorePath}; use std::{ cell::RefCell, collections::BTreeSet, @@ -153,16 +150,14 @@ impl TvixStoreIO { let output_paths: Vec = output_names .iter() .map(|output_name| { - let output_path = &input_drv + input_drv .outputs .get(output_name) .expect("missing output_name") - .path; - - // since Derivation is validated, we this can be parsed. - StorePathRef::from_absolute_path(output_path.as_bytes()) - .expect("invalid output path") - .to_owned() + .path + .as_ref() + .expect("missing output path") + .clone() }) .collect(); // For each output, ask for the castore node. 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, #[serde(flatten)] pub ca_hash: Option, // 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::(&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 { + 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 = 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 = 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 NomResult<&[u8], BTreeSet> { 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> { Ok((i, input_sources)) } +fn string_to_store_path( + i: &[u8], + path_str: String, +) -> Result>> { + #[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) => ( -- cgit 1.4.1