From 22f199947034ac770985b7920a1e6869278adbb6 Mon Sep 17 00:00:00 2001 From: Valentin Haudiquet Date: Sat, 3 Oct 2026 00:53:58 +0200 Subject: [PATCH] feat(cli): start the command in the mapped current directory The default --bind overlay mounted the current directory at /root/, but the command still started in /root: the library's working-directory fallback only considers read-write binds. Pass the mapped target as the explicit working_dir so relative paths keep meaning what they meant on the host. Bind sources are canonicalized when the targets are built instead of at mount time. This fixes --bind . and other basename-less relative paths, which cli_mount_target rejected, and lets the --bind/--bind-rw dedup match relative and absolute spellings of the same directory. --- Cargo.lock | 1 + crates/ecr-cli/Cargo.toml | 4 + crates/ecr-cli/src/main.rs | 159 ++++++++++++++++++++++++++++++++++--- 3 files changed, 154 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 30f78ed..9336359 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -331,6 +331,7 @@ dependencies = [ "ecr", "indicatif", "log", + "tempfile", ] [[package]] diff --git a/crates/ecr-cli/Cargo.toml b/crates/ecr-cli/Cargo.toml index 9f0d75b..268f836 100644 --- a/crates/ecr-cli/Cargo.toml +++ b/crates/ecr-cli/Cargo.toml @@ -27,3 +27,7 @@ log = { version = "0.4", features = ["std"] } # Progress bar rendering of library progress events indicatif = "0.18" + +[dev-dependencies] +# Temp directories for bind-target tests +tempfile = "3" diff --git a/crates/ecr-cli/src/main.rs b/crates/ecr-cli/src/main.rs index 7a60dfd..af9cbee 100644 --- a/crates/ecr-cli/src/main.rs +++ b/crates/ecr-cli/src/main.rs @@ -112,13 +112,14 @@ fn main() -> Result<()> { /// Run in namespace/chroot mode, returning the command's exit code fn namespace_mode(args: &Args, rootfs: &Path, config: &Config, arch: &str) -> Result { + let (binds, working_dir) = cli_bind_targets(args)?; let opts = ExecOptions { arch: arch.to_string(), - binds: cli_bind_targets(args)?, + binds, env: chroot::default_env(rootfs), dns: config.dns.clone(), command: args.command.clone(), - working_dir: None, + working_dir, }; let exit_code = ecr::exec(rootfs, &opts); @@ -135,8 +136,15 @@ fn namespace_mode(args: &Args, rootfs: &Path, config: &Config, arch: &str) -> Re /// --bind overlays read-only at /root/, --bind-rw bind-mounts /// read-write at /mnt/ (taking precedence over --bind for the /// same source path). With no --bind given, the current directory is -/// overlaid by default. -fn cli_bind_targets(args: &Args) -> Result> { +/// overlaid by default. Also returns the in-rootfs target where the +/// current directory is mapped, when it is: the command starts there so +/// relative paths keep meaning what they meant on the host. +fn cli_bind_targets(args: &Args) -> Result<(Vec, Option)> { + let cwd = std::env::current_dir().ok(); + bind_targets(args, cwd.as_deref()) +} + +fn bind_targets(args: &Args, cwd: Option<&Path>) -> Result<(Vec, Option)> { // --no-bind means "skip mounting any directory". Combining it with an // explicit --bind-rw is contradictory; error rather than silently ignoring // the flag the user asked for. @@ -147,19 +155,32 @@ fn cli_bind_targets(args: &Args) -> Result> { --no-bind skips all mounts, including read-write ones" )); } - return Ok(Vec::new()); + return Ok((Vec::new(), None)); } let mut binds = Vec::new(); + // Sources are canonicalized here rather than at mount time so that + // relative paths ("."), the --bind/--bind-rw dedup and the + // current-directory match below all see the same path. A source that + // cannot be canonicalized (e.g. missing directory) is kept as typed: + // cli_mount_target then fails on basename-less paths, and real paths + // fail at mount time with the OS error. + let canonical = |path: &Path| path.canonicalize().unwrap_or_else(|_| path.to_path_buf()); + let ro_sources: Vec = if args.bind.is_empty() { - vec![std::env::current_dir().context("Could not get current directory")?] + // current_dir() already returns the canonical path + vec![cwd + .context("Could not get current directory")? + .to_path_buf()] } else { - args.bind.clone() + args.bind.iter().map(|p| canonical(p)).collect() }; + let rw_sources: Vec = args.bind_rw.iter().map(|p| canonical(p)).collect(); + for source in ro_sources { - if args.bind_rw.contains(&source) { + if rw_sources.contains(&source) { continue; } binds.push(BindTarget { @@ -169,7 +190,7 @@ fn cli_bind_targets(args: &Args) -> Result> { }); } - for source in &args.bind_rw { + for source in &rw_sources { binds.push(BindTarget { target: cli_mount_target(source, "mnt")?, source: source.clone(), @@ -177,7 +198,14 @@ fn cli_bind_targets(args: &Args) -> Result> { }); } - Ok(binds) + let working_dir = cwd.map(Path::to_path_buf).and_then(|dir| { + binds + .iter() + .find(|b| b.source == dir) + .map(|b| b.target.clone()) + }); + + Ok((binds, working_dir)) } fn cli_mount_target(source: &Path, parent: &str) -> Result { @@ -186,3 +214,114 @@ fn cli_mount_target(source: &Path, parent: &str) -> Result { .ok_or_else(|| anyhow::anyhow!("Invalid bind path: {}", source.display()))?; Ok(Path::new("/").join(parent).join(basename)) } + +#[cfg(test)] +mod tests { + use super::*; + + fn args(bind: Vec, bind_rw: Vec, no_bind: bool) -> Args { + Args { + distro: "ubuntu".into(), + arch: None, + bind, + bind_rw, + no_cache: false, + no_bind, + verbose: false, + kernel: None, + memory: "2G".into(), + command: Vec::new(), + } + } + + #[test] + fn default_bind_starts_in_mapped_current_directory() { + let dir = tempfile::tempdir().unwrap(); + let cwd = dir.path().canonicalize().unwrap(); + let (_, working_dir) = + bind_targets(&args(Vec::new(), Vec::new(), false), Some(&cwd)).unwrap(); + assert_eq!( + working_dir, + Some(Path::new("/root").join(cwd.file_name().unwrap())) + ); + } + + #[test] + fn relative_bind_of_current_directory_starts_there() { + // "." must resolve to the current directory the way the mounts + // canonicalize the source; relative resolution follows the process + // cwd, so this test goes through cli_bind_targets with the process + // cwd moved into the temp dir. + let dir = tempfile::tempdir().unwrap(); + let original = std::env::current_dir().unwrap(); + std::env::set_current_dir(dir.path()).unwrap(); + let cwd = std::env::current_dir().unwrap(); + let result = cli_bind_targets(&args(vec![PathBuf::from(".")], Vec::new(), false)); + std::env::set_current_dir(original).unwrap(); + + let (_, working_dir) = result.unwrap(); + assert_eq!( + working_dir, + Some(Path::new("/root").join(cwd.file_name().unwrap())) + ); + } + + #[test] + fn bind_rw_of_current_directory_starts_in_mnt() { + let dir = tempfile::tempdir().unwrap(); + let cwd = dir.path().canonicalize().unwrap(); + let (_, working_dir) = + bind_targets(&args(Vec::new(), vec![cwd.clone()], false), Some(&cwd)).unwrap(); + assert_eq!( + working_dir, + Some(Path::new("/mnt").join(cwd.file_name().unwrap())) + ); + } + + #[test] + fn bind_rw_takes_precedence_for_the_current_directory() { + let dir = tempfile::tempdir().unwrap(); + let cwd = dir.path().canonicalize().unwrap(); + let (binds, working_dir) = bind_targets( + &args(vec![cwd.clone()], vec![cwd.clone()], false), + Some(&cwd), + ) + .unwrap(); + assert_eq!(binds.len(), 1); + assert!(!binds[0].read_only); + assert_eq!( + working_dir, + Some(Path::new("/mnt").join(cwd.file_name().unwrap())) + ); + } + + #[test] + fn unmapped_current_directory_keeps_library_default() { + // The current directory is not among the binds: pass None so the + // library falls back to its own working-directory resolution. + let dir = tempfile::tempdir().unwrap(); + let cwd = dir.path().canonicalize().unwrap(); + let (_, working_dir) = bind_targets( + &args(vec![PathBuf::from("/opt/toolchain")], Vec::new(), false), + Some(&cwd), + ) + .unwrap(); + assert_eq!(working_dir, None); + } + + #[test] + fn no_bind_keeps_library_default() { + let (_, working_dir) = bind_targets(&args(Vec::new(), Vec::new(), true), None).unwrap(); + assert_eq!(working_dir, None); + } + + #[test] + fn no_bind_with_bind_rw_is_rejected() { + assert!(bind_targets(&args(Vec::new(), vec![PathBuf::from("/data")], true), None).is_err()); + } + + #[test] + fn default_bind_without_current_directory_errors() { + assert!(bind_targets(&args(Vec::new(), Vec::new(), false), None).is_err()); + } +}