From a46ff3a99b06cafac69990bf6c3878c648ea2ad7 Mon Sep 17 00:00:00 2001 From: SFG545 Date: Mon, 9 Mar 2026 02:44:43 -0500 Subject: [PATCH] feat: update depot version to 0.12.0 and refactor environment variable handling in tests --- Cargo.lock | 2 +- Cargo.toml | 2 +- meson.build | 2 +- src/builder/cmake.rs | 4 +- src/builder/mod.rs | 38 ++++---- src/builder/perl.rs | 10 +- src/commands.rs | 227 +++++++++++++++++++++++++++++-------------- src/config.rs | 20 +--- src/main.rs | 2 + src/test_support.rs | 50 ++++++++++ 10 files changed, 235 insertions(+), 122 deletions(-) create mode 100644 src/test_support.rs diff --git a/Cargo.lock b/Cargo.lock index 6a37665..7f55616 100755 --- a/Cargo.lock +++ b/Cargo.lock @@ -382,7 +382,7 @@ checksum = "807800ff3288b621186fe0a8f3392c4652068257302709c24efd918c3dffcdc2" [[package]] name = "depot" -version = "0.11.1" +version = "0.12.0" dependencies = [ "anyhow", "ar", diff --git a/Cargo.toml b/Cargo.toml index fc64f1c..ad37f2c 100755 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "depot" -version = "0.11.1" +version = "0.12.0" edition = "2024" [lints.rust] diff --git a/meson.build b/meson.build index fd2618a..1e26b6c 100644 --- a/meson.build +++ b/meson.build @@ -1,6 +1,6 @@ project( 'depot', - version: '0.11.0', + version: '0.12.0', meson_version: '>=0.60.0', ) diff --git a/src/builder/cmake.rs b/src/builder/cmake.rs index d211833..754cddf 100755 --- a/src/builder/cmake.rs +++ b/src/builder/cmake.rs @@ -388,12 +388,14 @@ fn resolve_actual_src( mod tests { use super::*; use crate::package::{Build, BuildFlags, BuildType, PackageInfo, PackageSpec}; + use crate::test_support::TestEnv; use tempfile::tempdir; #[test] fn test_expand_env_vars_replaces_vars() { // Set a test env var - unsafe { std::env::set_var("DEPOT_TEST_FOO", "bar") }; + let mut env = TestEnv::new(); + env.set_var("DEPOT_TEST_FOO", "bar"); let input = "$DEPOT_TEST_FOO and ${DEPOT_TEST_FOO}"; let out = expand_env_vars(input); assert!(out.contains("bar")); diff --git a/src/builder/mod.rs b/src/builder/mod.rs index 630a330..eb06b97 100755 --- a/src/builder/mod.rs +++ b/src/builder/mod.rs @@ -326,6 +326,7 @@ pub fn build( mod tests { use super::*; use crate::package::{Build, BuildFlags, BuildType, Dependencies, PackageInfo, PackageSpec}; + use crate::test_support::TestEnv; use std::collections::HashMap; use std::ffi::OsStr; use std::path::PathBuf; @@ -373,14 +374,13 @@ mod tests { // Set an env var that should be cleared cmd.env("FORBIDDEN", "value"); // Set PATH manually in the current process to ensure it's picked up if it exists - unsafe { - std::env::set_var("PATH", "/usr/bin"); - std::env::set_var("HOME", "/home/test"); - std::env::set_var("SHELL", "/bin/zsh"); - std::env::set_var("DEPOT_ROOTFS", "/my/rootfs"); - std::env::set_var("TERM", "xterm-256color"); - std::env::set_var("CLICOLOR_FORCE", "1"); - } + let mut env = TestEnv::new(); + env.set_var("PATH", "/usr/bin"); + env.set_var("HOME", "/home/test"); + env.set_var("SHELL", "/bin/zsh"); + env.set_var("DEPOT_ROOTFS", "/my/rootfs"); + env.set_var("TERM", "xterm-256color"); + env.set_var("CLICOLOR_FORCE", "1"); prepare_command(&mut cmd, &vec![("MYVAR".to_string(), "myval".to_string())]); @@ -416,9 +416,8 @@ mod tests { #[test] fn test_prepare_command_preserves_destdir() { let mut cmd = std::process::Command::new("ls"); - unsafe { - std::env::set_var("DESTDIR", "/tmp/dest"); - } + let mut env = TestEnv::new(); + env.set_var("DESTDIR", "/tmp/dest"); prepare_command(&mut cmd, &Vec::new()); let envs: HashMap<_, _> = cmd.get_envs().collect(); assert_eq!( @@ -430,10 +429,9 @@ mod tests { #[test] fn test_prepare_command_preserves_rust_toolchain_homes() { let mut cmd = std::process::Command::new("ls"); - unsafe { - std::env::set_var("CARGO_HOME", "/var/cache/cargo-home"); - std::env::set_var("RUSTUP_HOME", "/var/cache/rustup-home"); - } + let mut env = TestEnv::new(); + env.set_var("CARGO_HOME", "/var/cache/cargo-home"); + env.set_var("RUSTUP_HOME", "/var/cache/rustup-home"); prepare_command(&mut cmd, &Vec::new()); let envs: HashMap<_, _> = cmd.get_envs().collect(); assert_eq!( @@ -581,9 +579,8 @@ mod tests { let mut spec = mk_spec(Vec::new(), Vec::new()); spec.build.flags.passthrough_env = vec!["RUSTFLAGS".into()]; - unsafe { - std::env::set_var("RUSTFLAGS", "-C target-cpu=native"); - } + let mut env = TestEnv::new(); + env.set_var("RUSTFLAGS", "-C target-cpu=native"); let env = standard_build_env(&spec, None, false, true); assert!( @@ -599,9 +596,8 @@ mod tests { spec.build.flags.cc = "spec-cc".to_string(); spec.build.flags.passthrough_env = vec!["CC".into()]; - unsafe { - std::env::set_var("CC", "host-cc"); - } + let mut env = TestEnv::new(); + env.set_var("CC", "host-cc"); let env = standard_build_env(&spec, None, true, true); assert!( diff --git a/src/builder/perl.rs b/src/builder/perl.rs index 72158d0..4e1015f 100644 --- a/src/builder/perl.rs +++ b/src/builder/perl.rs @@ -276,6 +276,7 @@ fn has_assignment_prefix(args: &[String], name: &str) -> bool { mod tests { use super::*; use crate::package::{Build, BuildFlags, BuildType, Dependencies, PackageInfo}; + use crate::test_support::TestEnv; use std::os::unix::fs::PermissionsExt; use tempfile::tempdir; @@ -378,10 +379,9 @@ exec "$@" "#, )?; + let mut env = TestEnv::new(); let old_path = std::env::var("PATH").unwrap_or_default(); - unsafe { - std::env::set_var("PATH", format!("{}:{}", tools_path.display(), old_path)); - } + env.set_var("PATH", format!("{}:{}", tools_path.display(), old_path)); let mut spec = mk_spec("perl-test", "1.0"); spec.build.flags.make_exec = tools_path.join("make").to_string_lossy().into_owned(); @@ -389,10 +389,6 @@ exec "$@" let build_result = build(&spec, src_path, dest_path, None, true); - unsafe { - std::env::set_var("PATH", old_path); - } - build_result?; let perl_args = fs::read_to_string(src_path.join("perl-args.log"))?; diff --git a/src/commands.rs b/src/commands.rs index e356edf..7858cfd 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -53,16 +53,42 @@ fn maybe_reexec_with_sudo(cli: &Cli) -> Result { } } -fn run_child_install_command( - install_request: &Path, +#[derive(Clone, Copy)] +struct ChildInstallCommandOptions<'a> { + no_deps: bool, + assume_yes: bool, + no_flags: bool, + cross_prefix: Option<&'a str>, + clean: bool, + dep_chain: Option<&'a str>, +} + +fn install_request_display(install_requests: &[PathBuf]) -> String { + install_requests + .iter() + .map(|path| path.display().to_string()) + .collect::>() + .join(", ") +} + +fn run_install_command_with_program( + program: &Path, + install_requests: &[PathBuf], rootfs: &Path, - options: InstallPlanExecutionOptions<'_>, + options: ChildInstallCommandOptions<'_>, ) -> Result<()> { - let exe = std::env::current_exe().context("Failed to locate depot executable")?; - let mut cmd = std::process::Command::new(&exe); + if install_requests.is_empty() { + return Ok(()); + } + + let mut cmd = std::process::Command::new(program); cmd.arg("-r").arg(rootfs); - cmd.arg("--no-deps"); - cmd.arg("--yes"); + if options.no_deps { + cmd.arg("--no-deps"); + } + if options.assume_yes { + cmd.arg("--yes"); + } if options.no_flags { cmd.arg("--no-flags"); } @@ -72,12 +98,16 @@ fn run_child_install_command( if options.clean { cmd.arg("--clean"); } - cmd.arg("install").arg(install_request); + cmd.arg("install"); + cmd.args(install_requests); + if let Some(dep_chain) = options.dep_chain { + cmd.env("DEPOT_DEPCHAIN", dep_chain); + } let status = cmd.status().with_context(|| { format!( "Failed to spawn child install for {}", - install_request.display() + install_request_display(install_requests) ) })?; if status.success() { @@ -85,12 +115,33 @@ fn run_child_install_command( } else { anyhow::bail!( "Child install failed for {} with status {}", - install_request.display(), + install_request_display(install_requests), status ); } } +fn run_child_install_command( + install_requests: &[PathBuf], + rootfs: &Path, + options: InstallPlanExecutionOptions<'_>, +) -> Result<()> { + let exe = std::env::current_exe().context("Failed to locate depot executable")?; + run_install_command_with_program( + &exe, + install_requests, + rootfs, + ChildInstallCommandOptions { + no_deps: true, + assume_yes: true, + no_flags: options.no_flags, + cross_prefix: options.cross_prefix, + clean: options.clean, + dep_chain: None, + }, + ) +} + fn parse_licenses_from_toml(metadata: &toml::Value) -> Vec { if let Some(s) = metadata.get("license").and_then(|v| v.as_str()) { return vec![s.to_string()]; @@ -1250,10 +1301,11 @@ fn execute_install_plan_with_child_commands( } if should_delegate_live_rootfs_installs(rootfs) { + let mut install_requests = Vec::new(); for step in actionable_steps { match &step.origin { planner::PlanOrigin::Source { path, .. } => { - run_child_install_command(path, rootfs, options)?; + install_requests.push(path.clone()); } planner::PlanOrigin::Binary { repo_name, record } => { let repo_cfg = config.binary_repos.get(repo_name).with_context(|| { @@ -1266,11 +1318,12 @@ fn execute_install_plan_with_child_commands( record, &config.package_cache_dir, )?; - run_child_install_command(&archive_path, rootfs, options)?; + install_requests.push(archive_path); } planner::PlanOrigin::Installed => {} } } + run_child_install_command(&install_requests, rootfs, options)?; return Ok(()); } @@ -1712,42 +1765,35 @@ fn run_direct_install_request( format!("{},{}", dep_chain, pkg_spec.package.name) }; - // Attempt to install missing deps + let mut dep_spec_paths = Vec::new(); for dep in missing { // Use package index for O(1) lookup let candidate = pkg_index.find(&dep); if let Some(dep_spec_path) = candidate { - ui::info(format!("Installing dependency: {}...", dep)); - - let mut cmd = std::process::Command::new(std::env::current_exe()?); - cmd.arg("-r").arg(options.rootfs); - - if options.no_deps { - cmd.arg("--no-deps"); - } - if options.no_flags { - cmd.arg("--no-flags"); - } - if let Some(p) = options.cross_prefix { - cmd.arg("--cross-prefix").arg(p); - } - if options.clean { - cmd.arg("--clean"); - } - - cmd.arg("install").arg(&dep_spec_path); - cmd.env("DEPOT_DEPCHAIN", &new_chain); - - let status = cmd.status()?; - - if !status.success() { - anyhow::bail!("Failed to install dependency: {}", dep); - } + dep_spec_paths.push(dep_spec_path); } else { anyhow::bail!("Could not find package spec for dependency: {}", dep); } } + ui::info(format!( + "Installing dependencies: {}", + install_request_display(&dep_spec_paths) + )); + let exe = std::env::current_exe().context("Failed to locate depot executable")?; + run_install_command_with_program( + &exe, + &dep_spec_paths, + options.rootfs, + ChildInstallCommandOptions { + no_deps: options.no_deps, + assume_yes: false, + no_flags: options.no_flags, + cross_prefix: options.cross_prefix, + clean: options.clean, + dep_chain: Some(&new_chain), + }, + )?; } } @@ -2206,19 +2252,17 @@ pub fn run(cli: Cli) -> Result<()> { } if ui::prompt_package_action("installation", &install_targets, false)? { if should_delegate_live_rootfs_installs(&cli.rootfs) { - for archive in &created_files { - run_child_install_command( - archive, - &cli.rootfs, - InstallPlanExecutionOptions { - no_flags: cli.no_flags, - cross_prefix: cli.cross_prefix.as_deref(), - clean: cli.clean, - dry_run: cli.dry_run, - confirm_installation: false, - }, - )?; - } + run_child_install_command( + &created_files, + &cli.rootfs, + InstallPlanExecutionOptions { + no_flags: cli.no_flags, + cross_prefix: cli.cross_prefix.as_deref(), + clean: cli.clean, + dry_run: cli.dry_run, + confirm_installation: false, + }, + )?; if cli.clean { clean_build_workspace(&config)?; } @@ -2915,28 +2959,7 @@ mod tests { let mut cfg = config::Config::for_rootfs(rootfs.path()); cfg.build_dir = rootfs.path().join("var/cache/depot/build"); - let blocked_tmp = rootfs.path().join("blocked-tmp"); - fs::create_dir_all(&blocked_tmp) - .with_context(|| format!("Failed to create {}", blocked_tmp.display()))?; - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - fs::set_permissions(&blocked_tmp, fs::Permissions::from_mode(0o555)) - .with_context(|| format!("Failed to chmod {}", blocked_tmp.display()))?; - } - - let previous_tmpdir = std::env::var_os("TMPDIR"); - unsafe { - std::env::set_var("TMPDIR", &blocked_tmp); - } let staged = extract_package_archive_to_staging(&cfg, &archive_path)?; - unsafe { - if let Some(value) = previous_tmpdir { - std::env::set_var("TMPDIR", value); - } else { - std::env::remove_var("TMPDIR"); - } - } assert!(staged.path().starts_with(staging_temp_root(&cfg))); assert!(staged.path().join("usr/bin/hello").exists()); @@ -3143,6 +3166,64 @@ optional = [] assert_eq!(merged, vec!["make", "pkgconf", "glibc", "openssl", "zlib"]); } + #[test] + #[cfg(unix)] + fn child_install_command_batches_multiple_requests_in_one_invocation() -> Result<()> { + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().context("Failed to create temp dir")?; + let script_path = temp.path().join("capture-child-install.sh"); + let args_path = temp.path().join("args.txt"); + let env_path = temp.path().join("env.txt"); + let script = format!( + "#!/bin/sh\nprintf '%s\\n' \"$@\" > \"{}\"\nprintf '%s' \"${{DEPOT_DEPCHAIN:-}}\" > \"{}\"\n", + args_path.display(), + env_path.display() + ); + fs::write(&script_path, script) + .with_context(|| format!("Failed to write {}", script_path.display()))?; + fs::set_permissions(&script_path, fs::Permissions::from_mode(0o755)) + .with_context(|| format!("Failed to chmod {}", script_path.display()))?; + + let requests = vec![ + PathBuf::from("/tmp/pkg-a.toml"), + PathBuf::from("/tmp/pkg-b.toml"), + ]; + let rootfs = Path::new("/"); + run_install_command_with_program( + &script_path, + &requests, + rootfs, + ChildInstallCommandOptions { + no_deps: false, + assume_yes: false, + no_flags: true, + cross_prefix: Some("x86_64-linux-musl"), + clean: true, + dep_chain: Some("parent"), + }, + )?; + + let captured_args = fs::read_to_string(&args_path) + .with_context(|| format!("Failed to read {}", args_path.display()))?; + assert_eq!( + captured_args.lines().collect::>(), + vec![ + "-r", + "/", + "--no-flags", + "--cross-prefix", + "x86_64-linux-musl", + "--clean", + "install", + "/tmp/pkg-a.toml", + "/tmp/pkg-b.toml", + ] + ); + assert_eq!(fs::read_to_string(&env_path)?, "parent"); + Ok(()) + } + #[test] fn rootfs_is_system_root_detects_live_rootfs() { assert!(rootfs_is_system_root(Path::new("/"))); diff --git a/src/config.rs b/src/config.rs index efcb82f..940e666 100755 --- a/src/config.rs +++ b/src/config.rs @@ -511,6 +511,7 @@ impl Default for Config { #[cfg(test)] mod tests { use super::*; + use crate::test_support::TestEnv; #[test] fn test_config_for_rootfs() { @@ -616,13 +617,9 @@ cflags += ["-g"] #[test] fn test_config_non_root_fallback() { - // Make the test deterministic by overriding HOME for the duration of the check. - // Save and restore the original value so other tests aren't affected. - let orig_home = std::env::var_os("HOME"); let fake_home = tempfile::tempdir().unwrap(); - unsafe { - std::env::set_var("HOME", fake_home.path()); - } + let mut env = TestEnv::new(); + env.set_var("HOME", fake_home.path()); let config = Config::for_rootfs(Path::new("/")); @@ -639,17 +636,6 @@ cflags += ["-g"] // If running as root (e.g. in some CI), it should use /var assert_eq!(config.cache_dir, PathBuf::from("/var/cache/depot/sources")); } - - // restore original HOME - if let Some(v) = orig_home { - unsafe { - std::env::set_var("HOME", v); - } - } else { - unsafe { - std::env::remove_var("HOME"); - } - } } #[test] diff --git a/src/main.rs b/src/main.rs index 38dbf35..6d00d47 100755 --- a/src/main.rs +++ b/src/main.rs @@ -20,6 +20,8 @@ mod shell_helpers; mod signing; mod source; mod staging; +#[cfg(test)] +mod test_support; mod ui; use anyhow::Result; diff --git a/src/test_support.rs b/src/test_support.rs new file mode 100644 index 0000000..96cef0b --- /dev/null +++ b/src/test_support.rs @@ -0,0 +1,50 @@ +use std::collections::BTreeMap; +use std::env; +use std::ffi::{OsStr, OsString}; +use std::sync::{Mutex, MutexGuard, OnceLock}; + +fn env_lock() -> &'static Mutex<()> { + static ENV_LOCK: OnceLock> = OnceLock::new(); + ENV_LOCK.get_or_init(|| Mutex::new(())) +} + +pub(crate) struct TestEnv { + _guard: MutexGuard<'static, ()>, + saved: BTreeMap>, +} + +impl TestEnv { + pub(crate) fn new() -> Self { + Self { + _guard: env_lock().lock().expect("test env lock poisoned"), + saved: BTreeMap::new(), + } + } + + pub(crate) fn set_var(&mut self, key: &str, value: impl AsRef) { + self.snapshot(key); + unsafe { + env::set_var(key, value); + } + } + + fn snapshot(&mut self, key: &str) { + self.saved + .entry(key.to_string()) + .or_insert_with(|| env::var_os(key)); + } +} + +impl Drop for TestEnv { + fn drop(&mut self) { + for (key, value) in &self.saved { + unsafe { + if let Some(value) = value { + env::set_var(key, value); + } else { + env::remove_var(key); + } + } + } + } +}