refactor(tvix/build): use stricter BuildRequest type

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

tvix/build/src/oci/bundle.rs

@@ -5,7 +5,7 @@ use std::{
 };
 
 use super::scratch_name;
-use crate::proto::BuildRequest;
+use crate::buildservice::BuildRequest;
 use anyhow::{bail, Context};
 use tracing::{debug, instrument};
 
@@ -60,12 +60,10 @@ pub(crate) fn get_host_output_paths(
 
     for output_path in request.outputs.iter() {
         // calculate the location of the path.
-        if let Some((mp, relpath)) =
-            find_path_in_scratchs(output_path, request.scratch_paths.as_slice())
-        {
+        if let Some((mp, relpath)) = find_path_in_scratchs(output_path, &request.scratch_paths) {
             host_output_paths.push(scratch_root.join(scratch_name(mp)).join(relpath));
         } else {
-            bail!("unable to find path {}", output_path);
+            bail!("unable to find path {output_path:?}");
         }
     }
 
@@ -77,16 +75,18 @@ pub(crate) fn get_host_output_paths(
 /// relative path from there to the search_path.
 /// mountpoints must be sorted, so we can iterate over the list from the back
 /// and match on the prefix.
-fn find_path_in_scratchs<'a, 'b>(
-    search_path: &'a str,
-    mountpoints: &'b [String],
-) -> Option<(&'b str, &'a str)> {
-    mountpoints.iter().rev().find_map(|mp| {
-        Some((
-            mp.as_str(),
-            search_path.strip_prefix(mp)?.strip_prefix('/')?,
-        ))
-    })
+fn find_path_in_scratchs<'a, 'b, I>(
+    search_path: &'a Path,
+    mountpoints: I,
+) -> Option<(&'b Path, &'a Path)>
+where
+    I: IntoIterator<Item = &'b PathBuf>,
+    I::IntoIter: DoubleEndedIterator,
+{
+    mountpoints
+        .into_iter()
+        .rev()
+        .find_map(|mp| Some((mp.as_path(), search_path.strip_prefix(mp).ok()?)))
 }
 
 #[cfg(test)]
@@ -95,7 +95,7 @@ mod tests {
 
     use rstest::rstest;
 
-    use crate::{oci::scratch_name, proto::BuildRequest};
+    use crate::{buildservice::BuildRequest, oci::scratch_name};
 
     use super::{find_path_in_scratchs, get_host_output_paths};
 
@@ -108,7 +108,18 @@ mod tests {
         #[case] mountpoints: &[String],
         #[case] expected: Option<(&str, &str)>,
     ) {
-        assert_eq!(find_path_in_scratchs(search_path, mountpoints), expected);
+        let expected = expected.map(|e| (Path::new(e.0), Path::new(e.1)));
+        assert_eq!(
+            find_path_in_scratchs(
+                Path::new(search_path),
+                mountpoints
+                    .iter()
+                    .map(PathBuf::from)
+                    .collect::<Vec<_>>()
+                    .as_slice()
+            ),
+            expected
+        );
     }
 
     #[test]
@@ -125,7 +136,7 @@ mod tests {
         let mut expected_path = PathBuf::new();
         expected_path.push("bundle-root");
         expected_path.push("scratch");
-        expected_path.push(scratch_name("nix/store"));
+        expected_path.push(scratch_name(Path::new("nix/store")));
         expected_path.push("fhaj6gmwns62s6ypkcldbaj2ybvkhx3p-foo");
 
         assert_eq!(vec![expected_path], paths)

tvix/build/src/oci/mod.rs

@@ -5,9 +5,12 @@ pub(crate) use bundle::get_host_output_paths;
 pub(crate) use bundle::make_bundle;
 pub(crate) use spec::make_spec;
 
+use std::path::Path;
+
 /// For a given scratch path, return the scratch_name that's allocated.
 // We currently use use lower hex encoding of the b3 digest of the scratch
 // path, so we don't need to globally allocate and pass down some uuids.
-pub(crate) fn scratch_name(scratch_path: &str) -> String {
-    data_encoding::BASE32.encode(blake3::hash(scratch_path.as_bytes()).as_bytes())
+pub(crate) fn scratch_name(scratch_path: &Path) -> String {
+    data_encoding::BASE32
+        .encode(blake3::hash(scratch_path.as_os_str().as_encoded_bytes()).as_bytes())
 }

tvix/build/src/oci/spec.rs

@@ -1,11 +1,10 @@
 //! Module to create a OCI runtime spec for a given [BuildRequest].
-use crate::proto::BuildRequest;
+use crate::buildservice::{BuildConstraints, BuildRequest};
 use oci_spec::{
     runtime::{Capability, LinuxNamespace, LinuxNamespaceBuilder, LinuxNamespaceType},
     OciSpecError,
 };
 use std::{collections::HashSet, path::Path};
-use tvix_castore::proto as castorepb;
 
 use super::scratch_name;
 
@@ -35,33 +34,26 @@ pub(crate) fn make_spec(
     rootless: bool,
     sandbox_shell: &str,
 ) -> Result<oci_spec::runtime::Spec, oci_spec::OciSpecError> {
-    // TODO: add BuildRequest validations. BuildRequest must contain strings as inputs
-
     let allow_network = request
         .constraints
-        .as_ref()
-        .is_some_and(|c| c.network_access);
+        .contains(&BuildConstraints::NetworkAccess);
 
     // Assemble ro_host_mounts. Start with constraints.available_ro_paths.
-    let mut ro_host_mounts = request
+    let mut ro_host_mounts: Vec<_> = request
         .constraints
-        .as_ref()
-        .map(|constraints| {
-            constraints
-                .available_ro_paths
-                .iter()
-                .map(|e| (e.as_str(), e.as_str()))
-                .collect::<Vec<_>>()
+        .iter()
+        .filter_map(|constraint| match constraint {
+            BuildConstraints::AvailableReadOnlyPath(path) => Some((path.as_path(), path.as_path())),
+            _ => None,
         })
-        .unwrap_or_default();
+        .collect();
 
     // If provide_bin_sh is set, mount sandbox_shell to /bin/sh
     if request
         .constraints
-        .as_ref()
-        .is_some_and(|c| c.provide_bin_sh)
+        .contains(&BuildConstraints::ProvideBinSh)
     {
-        ro_host_mounts.push((sandbox_shell, "/bin/sh"))
+        ro_host_mounts.push((Path::new(sandbox_shell), Path::new("/bin/sh")))
     }
 
     oci_spec::runtime::SpecBuilder::default()
@@ -92,9 +84,9 @@ pub(crate) fn make_spec(
         .mounts(configure_mounts(
             rootless,
             allow_network,
-            request.scratch_paths.iter().map(|e| e.as_str()),
+            request.scratch_paths.iter().map(|e| e.as_path()),
             request.inputs.iter(),
-            &request.inputs_dir, // TODO: validate
+            &request.inputs_dir,
             ro_host_mounts,
         )?)
         .build()
@@ -106,7 +98,7 @@ pub(crate) fn make_spec(
 /// Capabilities are a bit more complicated in case rootless building is requested.
 fn configure_process<'a>(
     command_args: &[String],
-    cwd: &String,
+    cwd: &Path,
     env: impl IntoIterator<Item = (&'a str, String)>,
     rootless: bool,
 ) -> Result<oci_spec::runtime::Process, oci_spec::OciSpecError> {
@@ -232,10 +224,11 @@ fn configure_linux(
 fn configure_mounts<'a>(
     rootless: bool,
     allow_network: bool,
-    scratch_paths: impl IntoIterator<Item = &'a str>,
-    inputs: impl Iterator<Item = &'a castorepb::Node>,
-    inputs_dir: &str,
-    ro_host_mounts: impl IntoIterator<Item = (&'a str, &'a str)>,
+    scratch_paths: impl IntoIterator<Item = &'a Path>,
+    inputs: impl Iterator<Item = (&'a tvix_castore::PathComponent, &'a tvix_castore::Node)>,
+
+    inputs_dir: &Path,
+    ro_host_mounts: impl IntoIterator<Item = (&'a Path, &'a Path)>,
 ) -> Result<Vec<oci_spec::runtime::Mount>, oci_spec::OciSpecError> {
     let mut mounts: Vec<_> = if rootless {
         oci_spec::runtime::get_rootless_mounts()
@@ -244,8 +237,8 @@ fn configure_mounts<'a>(
     };
 
     mounts.push(configure_mount(
-        "tmpfs",
-        "/tmp",
+        Path::new("tmpfs"),
+        Path::new("/tmp"),
         "tmpfs",
         &["nosuid", "noatime", "mode=700"],
     )?);
@@ -255,28 +248,19 @@ fn configure_mounts<'a>(
     for scratch_path in scratch_paths.into_iter() {
         let src = scratch_root.join(scratch_name(scratch_path));
         mounts.push(configure_mount(
-            src.to_str().unwrap(),
-            Path::new("/").join(scratch_path).to_str().unwrap(),
+            &src,
+            &Path::new("/").join(scratch_path),
             "none",
             &["rbind", "rw"],
         )?);
     }
 
     // For each input, create a bind mount from inputs/$name into $inputs_dir/$name.
-    for input in inputs {
-        let (input_name, _input) = input
-            .clone()
-            .try_into_name_and_node()
-            .expect("invalid input name");
-
+    for (input_name, _input) in inputs {
         let input_name = std::str::from_utf8(input_name.as_ref()).expect("invalid input name");
         mounts.push(configure_mount(
-            Path::new("inputs").join(input_name).to_str().unwrap(),
-            Path::new("/")
-                .join(inputs_dir)
-                .join(input_name)
-                .to_str()
-                .unwrap(),
+            &Path::new("inputs").join(input_name),
+            &Path::new("/").join(inputs_dir).join(input_name),
             "none",
             &[
                 "rbind", "ro",
@@ -295,7 +279,11 @@ fn configure_mounts<'a>(
 
     // In case network is enabled, also mount in /etc/{resolv.conf,services,hosts}
     if allow_network {
-        for p in ["/etc/resolv.conf", "/etc/services", "/etc/hosts"] {
+        for p in [
+            Path::new("/etc/resolv.conf"),
+            Path::new("/etc/services"),
+            Path::new("/etc/hosts"),
+        ] {
             mounts.push(configure_mount(p, p, "none", &["rbind", "ro"])?);
         }
     }
@@ -305,15 +293,15 @@ fn configure_mounts<'a>(
 
 /// Helper function to produce a mount.
 fn configure_mount(
-    source: &str,
-    destination: &str,
+    source: &Path,
+    destination: &Path,
     typ: &str,
     options: &[&str],
 ) -> Result<oci_spec::runtime::Mount, oci_spec::OciSpecError> {
     oci_spec::runtime::MountBuilder::default()
-        .destination(destination.to_string())
+        .destination(destination)
         .typ(typ.to_string())
-        .source(source.to_string())
+        .source(source)
         .options(options.iter().map(|e| e.to_string()).collect::<Vec<_>>())
         .build()
 }