From 7348d704ea54d604f5a940ac992f4643af188a29 Mon Sep 17 00:00:00 2001 From: marshmallow Date: Thu, 21 May 2026 13:03:07 +1000 Subject: [PATCH] use snix StorePath for store paths (#496) --- Cargo.toml | 1 + clippy.toml | 3 + crates/core/src/commands/common.rs | 8 ++- crates/core/src/commands/mod.rs | 28 ++------ crates/core/src/errors.rs | 17 ++++- crates/core/src/hive/executor.rs | 10 ++- crates/core/src/hive/node.rs | 24 ++----- crates/core/src/hive/steps/activate.rs | 16 +++-- crates/core/src/hive/steps/build.rs | 8 ++- crates/core/src/hive/steps/keys.rs | 15 +++-- crates/core/src/lib.rs | 88 ++++++++++++++++++++++++++ 11 files changed, 156 insertions(+), 62 deletions(-) create mode 100644 clippy.toml diff --git a/Cargo.toml b/Cargo.toml index fe3c3e6..eba5698 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -10,6 +10,7 @@ name = "wire" [workspace.lints.clippy] pedantic = { level = "deny", priority = -1 } missing_const_for_fn = "deny" +disallowed_types = "warn" # annoying to deal with missing_errors_doc = "allow" diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 0000000..66ce851 --- /dev/null +++ b/clippy.toml @@ -0,0 +1,3 @@ +disallowed-types = [ + { path = "nix_compat::store_path::StorePath", reason = "use `SafeStorePath` instead to ensure path prefixes are handled correctly", replacement = "crate::SafeStorePath" }, +] diff --git a/crates/core/src/commands/common.rs b/crates/core/src/commands/common.rs index ade67bf..fee16e6 100644 --- a/crates/core/src/commands/common.rs +++ b/crates/core/src/commands/common.rs @@ -48,8 +48,8 @@ pub async fn push( host = target.get_preferred_host()?, ), match push { - Push::Derivation(drv) => format!("{drv} --derivation"), - Push::Path(path) => path.clone(), + Push::Derivation(drv) => format!("{}^* --derivation", drv.to_absolute_path()), + Push::Path(path) => path.to_absolute_path(), }, ]); @@ -73,7 +73,9 @@ pub async fn push( status.map_err(|error| HiveLibError::NixCopyError { name: context.name.clone(), - path: push.to_string(), + path: match push { + Push::Derivation(path) | Push::Path(path) => path.clone(), + }, error: Box::new(error), help, })?; diff --git a/crates/core/src/commands/mod.rs b/crates/core/src/commands/mod.rs index b23e664..bffc395 100644 --- a/crates/core/src/commands/mod.rs +++ b/crates/core/src/commands/mod.rs @@ -2,6 +2,7 @@ // Copyright 2024-2025 wire Contributors use crate::{ + SafeStorePath, commands::pty::{InteractiveChildChip, interactive_command_with_env}, hive::node::{BuildNameMap, SharedTarget}, }; @@ -9,7 +10,6 @@ use core::str; use std::{ borrow::Cow, collections::HashMap, - path::Path, sync::{Arc, LazyLock, nonpoison::Mutex}, }; @@ -296,34 +296,14 @@ fn log_print( } fn drv_path_to_build_name(drv_path: &[u8]) -> Arc { - let string = match String::from_utf8(drv_path.to_vec()) { + let store_path = match SafeStorePath::<&str>::from_absolute_path(drv_path) { Err(err) => { error!(err = %err, "failed to parse build job name"); return Arc::new(String::from_utf8_lossy(drv_path).to_string()); } - Ok(str) => str, + Ok(path) => path, }; - let Some(file_stem) = Path::new(&string).file_stem() else { - error!("drv path build job's file_stem was None"); - - return Arc::new(String::from_utf8_lossy(drv_path).to_string()); - }; - - let Some(file_stem) = file_stem.to_str() else { - error!("drv path build job's file_stem was not valid unicode"); - - return Arc::new(String::from_utf8_lossy(drv_path).to_string()); - }; - - let build_name = file_stem.split_once('-').map_or_else( - || { - error!("unexpected drv build job file stem format"); - file_stem - }, - |(_, name)| name, - ); - - Arc::new(build_name.to_string()) + Arc::new(store_path.name().to_string()) } diff --git a/crates/core/src/errors.rs b/crates/core/src/errors.rs index 9e84d8c..172803f 100644 --- a/crates/core/src/errors.rs +++ b/crates/core/src/errors.rs @@ -10,7 +10,10 @@ use nix_compat::flakeref::{FlakeRef, FlakeRefError}; use thiserror::Error; use tokio::task::JoinError; -use crate::hive::node::{Name, SwitchToConfigurationGoal}; +use crate::{ + SafeStorePath, + hive::node::{Name, SwitchToConfigurationGoal}, +}; #[derive(Debug, Diagnostic, Error)] pub enum KeyError { @@ -247,10 +250,10 @@ pub enum HiveLibError { }, #[diagnostic(code(wire::CopyPath))] - #[error("failed to copy path {path} to node {name}")] + #[error("failed to copy path {} to node {name}", path.to_absolute_path())] NixCopyError { name: Name, - path: String, + path: SafeStorePath, #[source] error: Box, #[help] @@ -276,4 +279,12 @@ pub enum HiveLibError { #[diagnostic(code(wire::SIGINT))] #[error("SIGINT received, shut down")] Sigint, + + #[diagnostic(code(wire::SnixStorePath))] + #[error("Failed to parse store path {path:?}")] + StorePath { + path: String, + #[source] + error: nix_compat::store_path::Error, + }, } diff --git a/crates/core/src/hive/executor.rs b/crates/core/src/hive/executor.rs index 6e3ba30..90c010f 100644 --- a/crates/core/src/hive/executor.rs +++ b/crates/core/src/hive/executor.rs @@ -1,4 +1,5 @@ use crate::{ + SafeStorePath, hive::node::Step, status::{NodeStatus, UI_SENDER, UiMessage}, }; @@ -12,7 +13,7 @@ use crate::{ errors::HiveLibError, hive::{ HiveLocation, - node::{Context, Derivation, ExecuteStep, Name}, + node::{Context, ExecuteStep, Name}, plan::NodePlan, }, }; @@ -32,7 +33,7 @@ fn app_shutdown_guard(context: &Context) -> Result<(), HiveLibError> { /// Task that evaluates the node. #[instrument(skip_all, name = "eval")] async fn evaluate_task( - tx: tokio::sync::oneshot::Sender>, + tx: tokio::sync::oneshot::Sender, HiveLibError>>, hive_location: Arc, name: Name, modifiers: SubCommandModifiers, @@ -45,6 +46,11 @@ async fn evaluate_task( crate::errors::HiveInitialisationError::ParseEvaluateError(e), ) }) + }) + .and_then(|output: String| { + debug!(pre_parsed_output = %output, "evaluated {name}"); + + SafeStorePath::::from_absolute_path(output.as_bytes()) }); debug!(output = ?output, done = true); diff --git a/crates/core/src/hive/node.rs b/crates/core/src/hive/node.rs index 8ac1c9a..4df52b5 100644 --- a/crates/core/src/hive/node.rs +++ b/crates/core/src/hive/node.rs @@ -22,7 +22,7 @@ use crate::hive::steps::evaluate::Evaluate; use crate::hive::steps::keys::{Key, Keys, PushKeyAgent}; use crate::hive::steps::ping::Ping; use crate::hive::steps::push::{PushBuildOutput, PushEvaluatedOutput}; -use crate::{StrictHostKeyChecking, SubCommandModifiers}; +use crate::{SafeStorePath, StrictHostKeyChecking, SubCommandModifiers}; use super::HiveLibError; use super::steps::activate::SwitchToConfiguration; @@ -235,19 +235,9 @@ pub fn should_apply_locally(allow_local_deployment: bool, name: &str) -> bool { *name == *gethostname() && allow_local_deployment } -#[derive(derive_more::Display)] pub enum Push<'a> { - Derivation(&'a Derivation), - Path(&'a String), -} - -#[derive(Deserialize, Clone, Debug)] -pub struct Derivation(String); - -impl Display for Derivation { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.0.fmt(f).and_then(|()| write!(f, "^*")) - } + Derivation(&'a SafeStorePath), + Path(&'a SafeStorePath), } #[derive(derive_more::Display, Debug, Clone, Copy, PartialEq, Eq)] @@ -282,10 +272,10 @@ pub enum HandleUnreachable { #[derive(Default)] pub struct StepState { - pub evaluation: Option, - pub evaluation_rx: Option>>, - pub build: Option, - pub key_agent_directory: Option, + pub evaluation: Option>, + pub evaluation_rx: Option, HiveLibError>>>, + pub build: Option>, + pub key_agent_directory: Option>, } pub type BuildNameMap = Arc>>>; diff --git a/crates/core/src/hive/steps/activate.rs b/crates/core/src/hive/steps/activate.rs index 99fbfb9..52bb499 100644 --- a/crates/core/src/hive/steps/activate.rs +++ b/crates/core/src/hive/steps/activate.rs @@ -6,7 +6,7 @@ use std::{fmt::Display, sync::Arc}; use tracing::{error, info, instrument, warn}; use crate::{ - HiveLibError, + HiveLibError, SafeStorePath, commands::{CommandArguments, WireCommandChip, builder::CommandStringBuilder, run_command}, errors::{ActivationError, NetworkError}, hive::node::{Context, ExecuteStep, SharedTarget, SwitchToConfigurationGoal}, @@ -47,7 +47,11 @@ async fn wait_for_ping(target: &SharedTarget, ctx: &Context) -> Result<(), HiveL } impl SwitchToConfiguration { - async fn set_profile(&self, built_path: &String, ctx: &Context) -> Result<(), HiveLibError> { + async fn set_profile( + &self, + built_path: &SafeStorePath, + ctx: &Context, + ) -> Result<(), HiveLibError> { info!( "Setting profiles in anticipation for switch-to-configuration {}", self.goal @@ -55,7 +59,7 @@ impl SwitchToConfiguration { let mut command_string = CommandStringBuilder::new("nix-env"); command_string.args(&["-p", "/nix/var/nix/profiles/system", "--set"]); - command_string.arg(built_path); + command_string.arg(built_path.to_absolute_path()); let child = run_command( &CommandArguments::new(command_string, ctx.modifiers) @@ -93,8 +97,10 @@ impl ExecuteStep for SwitchToConfiguration { info!("Running switch-to-configuration {}", self.goal); - let mut command_string = - CommandStringBuilder::new(format!("{built_path}/bin/switch-to-configuration")); + let mut command_string = CommandStringBuilder::new(format!( + "{}/bin/switch-to-configuration", + built_path.to_absolute_path() + )); command_string.arg(match self.goal { SwitchToConfigurationGoal::Switch => "switch", SwitchToConfigurationGoal::Boot => "boot", diff --git a/crates/core/src/hive/steps/build.rs b/crates/core/src/hive/steps/build.rs index f8871dc..9e6b50b 100644 --- a/crates/core/src/hive/steps/build.rs +++ b/crates/core/src/hive/steps/build.rs @@ -6,7 +6,7 @@ use std::fmt::Display; use tracing::{info, instrument}; use crate::{ - HiveLibError, + HiveLibError, SafeStorePath, commands::{ CommandArguments, Either, WireCommandChip, builder::CommandStringBuilder, run_command_with_env, @@ -40,7 +40,7 @@ impl ExecuteStep for Build { "--print-out-paths", ]); command_string.opt_arg(ctx.modifiers.print_build_logs, "--print-build-logs"); - command_string.arg(top_level.to_string()); + command_string.arg(format!("{}^*", top_level.to_absolute_path())); let status = run_command_with_env( &CommandArguments::new(command_string, ctx.modifiers) @@ -67,7 +67,9 @@ impl ExecuteStep for Build { // print built path to stdout println!("{stdout}"); - ctx.state.build = Some(stdout); + ctx.state.build = Some(SafeStorePath::::from_absolute_path( + stdout.as_bytes(), + )?); Ok(()) } diff --git a/crates/core/src/hive/steps/keys.rs b/crates/core/src/hive/steps/keys.rs index 0a40568..b45cdaa 100644 --- a/crates/core/src/hive/steps/keys.rs +++ b/crates/core/src/hive/steps/keys.rs @@ -26,12 +26,12 @@ use tokio::{fs::File, io::AsyncRead}; use tokio_util::codec::LengthDelimitedCodec; use tracing::{debug, instrument}; -use crate::HiveLibError; use crate::commands::builder::CommandStringBuilder; use crate::commands::common::push; use crate::commands::{CommandArguments, WireCommandChip, run_command}; use crate::errors::KeyError; use crate::hive::node::{Context, ExecuteStep, Push, SharedTarget}; +use crate::{HiveLibError, SafeStorePath}; #[derive(Serialize, Deserialize, Clone, Debug, Eq, PartialEq, Hash)] #[serde(tag = "t", content = "c")] @@ -245,8 +245,10 @@ impl ExecuteStep for Keys { return Ok(()); } - let command_string = - CommandStringBuilder::new(format!("{agent_directory}/bin/wire-key-agent")); + let command_string = CommandStringBuilder::new(format!( + "{}/bin/wire-key-agent", + agent_directory.to_absolute_path() + )); let mut child = run_command( &CommandArguments::new(command_string, ctx.modifiers) @@ -321,17 +323,20 @@ impl ExecuteStep for PushKeyAgent { ), }; + let agent_store_path = + SafeStorePath::::from_absolute_path(agent_directory.as_bytes())?; + if let Some(ref target) = self.target { push( ctx, target, - Push::Path(&agent_directory), + Push::Path(&agent_store_path), self.substitute_on_destination, ) .await?; } - ctx.state.key_agent_directory = Some(agent_directory); + ctx.state.key_agent_directory = Some(agent_store_path); Ok(()) } diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index 78454a3..a36cf77 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -8,6 +8,7 @@ use std::{io::IsTerminal, sync::LazyLock}; +use serde::Deserialize; use tokio::sync::{AcquireError, Semaphore, SemaphorePermit, mpsc::UnboundedSender, oneshot}; use crate::{ @@ -98,3 +99,90 @@ pub async fn acquire_stdin_lock<'a>() -> Result, AcquireError> Ok(result) } + +/// This type exists to restrict `StorePath` usage to only methods that deal with +/// absolute paths. By default, the `StorePath` type implements Display that +/// does not include `/nix/store/` can introduce many hard to catch bugs. +/// +/// +/// If +/// is ever closed, this can be dropped from the codebase. +#[derive(Debug, Clone)] +#[allow(clippy::disallowed_types)] +pub struct SafeStorePath(nix_compat::store_path::StorePath); + +#[allow(clippy::disallowed_types)] +impl SafeStorePath +where + S: AsRef, +{ + pub fn from_absolute_path<'a>(s: &'a [u8]) -> Result, HiveLibError> + where + S: From<&'a str>, + { + Ok(Self( + nix_compat::store_path::StorePath::from_absolute_path(s).map_err(|error| { + HiveLibError::StorePath { + path: String::from_utf8_lossy(s).to_string(), + error, + } + })?, + )) + } + + pub fn from_name_and_digest<'a>(name: &'a str, digest: &[u8]) -> Result + where + S: From<&'a str>, + { + Ok(Self( + nix_compat::store_path::StorePath::from_name_and_digest(name, digest).map_err( + |error| HiveLibError::StorePath { + path: format!("raw name & digest: {digest:?}-{name:?}"), + error, + }, + )?, + )) + } + + pub fn into_inner(self) -> nix_compat::store_path::StorePath { + self.0 + } + + pub fn to_absolute_path(&self) -> String { + self.0.to_absolute_path() + } + + pub fn digest(&self) -> &[u8; nix_compat::store_path::DIGEST_SIZE] { + self.0.digest() + } + + pub fn name(&self) -> &S { + self.0.name() + } +} + +#[allow(clippy::disallowed_types)] +impl<'de, S> Deserialize<'de> for SafeStorePath +where + nix_compat::store_path::StorePath: Deserialize<'de>, +{ + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + Ok(SafeStorePath( + nix_compat::store_path::StorePath::deserialize(deserializer)?, + )) + } +} + +impl PartialEq for SafeStorePath +where + S: AsRef, +{ + fn eq(&self, other: &Self) -> bool { + self.0.eq(&other.0) + } +} + +impl Eq for SafeStorePath where S: AsRef {} -- 2.51.2