refactor(tvix/build): remove proto::BuildRequest::validate

Change-Id: I96fa98946bf6aff5eedcb220e2b6b3d90c204eec
Reviewed-on: https://cl.tvl.fyi/c/depot/+/12633
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI

tvix/build/src/buildservice/build_request.rs

@@ -35,7 +35,7 @@ use tvix_castore::{Node, PathComponent};
 ///
 /// As of now, we're okay to accept this, but it prevents uploading an
 /// entirely-non-IFD subgraph of BuildRequests eagerly.
-#[derive(Clone, PartialEq)]
+#[derive(Debug, Clone, PartialEq)]
 pub struct BuildRequest {
     /// The list of all root nodes that should be visible in `inputs_dir` at the
     /// time of the build.
@@ -94,7 +94,7 @@ pub struct BuildRequest {
     pub refscan_needles: Vec<String>,
 }
 
-#[derive(Clone, PartialEq)]
+#[derive(Debug, Clone, PartialEq)]
 pub struct EnvVar {
     /// name of the environment variable. Must not contain =.
     pub key: String,
@@ -105,7 +105,7 @@ pub struct EnvVar {
 /// Constraints can be things like required architecture and minimum amount of memory.
 /// The required input paths are *not* represented in here, because it
 /// wouldn't be hermetic enough - see the comment around inputs too.
-#[derive(Clone, PartialEq, Eq, Hash)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
 pub enum BuildConstraints {
     /// The system that's needed to execute the build.
     /// Must not be empty.
@@ -124,7 +124,7 @@ pub enum BuildConstraints {
     ProvideBinSh,
 }
 
-#[derive(Clone, PartialEq)]
+#[derive(Debug, Clone, PartialEq)]
 pub struct AdditionalFile {
     pub path: PathBuf,
     pub contents: Bytes,

tvix/build/src/proto/mod.rs

@@ -118,90 +118,6 @@ where
     data.tuple_windows().all(|(a, b)| a <= b)
 }
 
-impl BuildRequest {
-    /// Ensures the build request is valid.
-    /// This means, all input nodes need to be valid, paths in lists need to be sorted,
-    /// and all restrictions around paths themselves (relative, clean, …) need
-    // to be fulfilled.
-    pub fn validate(&self) -> Result<(), ValidateBuildRequestError> {
-        // validate names. Make sure they're sorted
-
-        let mut last_name: bytes::Bytes = "".into();
-        for (i, node) in self.inputs.iter().enumerate() {
-            // TODO(flokli): store result somewhere
-            let (name, _node) = node
-                .clone()
-                .into_name_and_node()
-                .map_err(|e| ValidateBuildRequestError::InvalidInputNode(i, e))?;
-
-            if name.as_ref() <= last_name.as_ref() {
-                return Err(ValidateBuildRequestError::InputNodesNotSorted);
-            } else {
-                last_name = name.into()
-            }
-        }
-
-        // validate working_dir
-        if !is_clean_relative_path(&self.working_dir) {
-            Err(ValidateBuildRequestError::InvalidWorkingDir)?;
-        }
-
-        // validate scratch paths
-        for (i, p) in self.scratch_paths.iter().enumerate() {
-            if !is_clean_relative_path(p) {
-                Err(ValidateBuildRequestError::InvalidScratchPath(i))?
-            }
-        }
-        if !is_sorted(self.scratch_paths.iter().map(|e| e.as_bytes())) {
-            Err(ValidateBuildRequestError::ScratchPathsNotSorted)?;
-        }
-
-        // validate inputs_dir
-        if !is_clean_relative_path(&self.inputs_dir) {
-            Err(ValidateBuildRequestError::InvalidInputsDir)?;
-        }
-
-        // validate outputs
-        for (i, p) in self.outputs.iter().enumerate() {
-            if !is_clean_relative_path(p) {
-                Err(ValidateBuildRequestError::InvalidOutputPath(i))?
-            }
-        }
-        if !is_sorted(self.outputs.iter().map(|e| e.as_bytes())) {
-            Err(ValidateBuildRequestError::OutputsNotSorted)?;
-        }
-
-        // validate environment_vars.
-        for (i, e) in self.environment_vars.iter().enumerate() {
-            if e.key.is_empty() || e.key.contains('=') {
-                Err(ValidateBuildRequestError::InvalidEnvVar(i))?
-            }
-        }
-        if !is_sorted(self.environment_vars.iter().map(|e| e.key.as_bytes())) {
-            Err(ValidateBuildRequestError::EnvVarNotSorted)?;
-        }
-
-        // validate build constraints
-        if let Some(constraints) = self.constraints.as_ref() {
-            constraints
-                .validate()
-                .map_err(ValidateBuildRequestError::InvalidBuildConstraints)?;
-        }
-
-        // validate additional_files
-        for (i, additional_file) in self.additional_files.iter().enumerate() {
-            if !is_clean_relative_path(&additional_file.path) {
-                Err(ValidateBuildRequestError::InvalidAdditionalFilePath(i))?
-            }
-        }
-        if !is_sorted(self.additional_files.iter().map(|e| e.path.as_bytes())) {
-            Err(ValidateBuildRequestError::AdditionalFilesNotSorted)?;
-        }
-
-        Ok(())
-    }
-}
-
 impl TryFrom<BuildRequest> for crate::buildservice::BuildRequest {
     type Error = ValidateBuildRequestError;
     fn try_from(value: BuildRequest) -> Result<Self, Self::Error> {
@@ -311,26 +227,6 @@ pub enum ValidateBuildConstraintsError {
     AvailableRoPathsNotSorted,
 }
 
-impl build_request::BuildConstraints {
-    pub fn validate(&self) -> Result<(), ValidateBuildConstraintsError> {
-        // validate system
-        if self.system.is_empty() {
-            Err(ValidateBuildConstraintsError::InvalidSystem)?;
-        }
-        // validate available_ro_paths
-        for (i, p) in self.available_ro_paths.iter().enumerate() {
-            if !is_clean_absolute_path(p) {
-                Err(ValidateBuildConstraintsError::InvalidAvailableRoPaths(i))?
-            }
-        }
-        if !is_sorted(self.available_ro_paths.iter().map(|e| e.as_bytes())) {
-            Err(ValidateBuildConstraintsError::AvailableRoPathsNotSorted)?;
-        }
-
-        Ok(())
-    }
-}
-
 impl From<build_request::EnvVar> for crate::buildservice::EnvVar {
     fn from(value: build_request::EnvVar) -> Self {
         Self {

tvix/glue/src/tvix_build.rs

@@ -6,9 +6,10 @@ use std::collections::BTreeMap;
 use bytes::Bytes;
 use nix_compat::{derivation::Derivation, nixbase32, store_path::StorePath};
 use sha2::{Digest, Sha256};
+use tvix_build::buildservice::BuildRequest;
 use tvix_build::proto::{
+    self,
     build_request::{AdditionalFile, BuildConstraints, EnvVar},
-    BuildRequest,
 };
 use tvix_castore::Node;
 
@@ -43,7 +44,7 @@ pub(crate) fn get_refscan_needles(
         .chain(derivation.input_derivations.keys())
 }
 
-/// Takes a [Derivation] and turns it into a [BuildRequest].
+/// Takes a [Derivation] and turns it into a [proto::BuildRequest].
 /// It assumes the Derivation has been validated.
 /// It needs two lookup functions:
 /// - one translating input sources to a castore node
@@ -53,7 +54,7 @@ pub(crate) fn get_refscan_needles(
 pub(crate) fn derivation_to_build_request(
     derivation: &Derivation,
     inputs: BTreeMap<bytes::Bytes, Node>,
-) -> std::io::Result<BuildRequest> {
+) -> std::io::Result<proto::BuildRequest> {
     debug_assert!(derivation.validate(true).is_ok(), "drv must validate");
 
     // produce command_args, which is builder and arguments in a Vec.
@@ -113,7 +114,7 @@ pub(crate) fn derivation_to_build_request(
         provide_bin_sh: true,
     });
 
-    let build_request = BuildRequest {
+    let build_request = proto::BuildRequest {
         // Importantly, this must match the order of get_refscan_needles, since users may use that
         // function to map back from the found needles to a store path
         refscan_needles: get_refscan_needles(derivation)
@@ -141,10 +142,10 @@ pub(crate) fn derivation_to_build_request(
             .collect(),
     };
 
+    // FUTUREWORK: switch this function to construct the stricter BuildRequest directly.
     debug_assert!(
-        build_request.validate().is_ok(),
+        BuildRequest::try_from(build_request.clone()).is_ok(),
-        "invalid BuildRequest: {}",
+        "Tvix bug: BuildRequest would not be valid"
-        build_request.validate().unwrap_err()
     );
 
     Ok(build_request)