From a43ef0d3ed9abc59a597a0fdb81353bfca19c505 Mon Sep 17 00:00:00 2001 From: Alex Huszagh Date: Sat, 16 Jul 2022 11:14:48 -0500 Subject: [PATCH] Resolve symlinks for cargo and xargo home. Resolve symlinks for the xargo and cargo home (as well as the Nix store) prior to mounting, since they are mounted at a fixed location anyway. This is because podman mounts symlinks as root by default. We always canonicalize the paths on the host, after creating them, to ensure that any symlinks are resolved. Say we have `/path/to/home`, and `/path/to` is a symlink but `/path/to/home` doesn't exist yet. Trying to canonicalize `/path/to/home/.xargo` will fail, and will still be a symlink if we maybe canonicalize before. First creating the directories and canonicalizing them on the host should always work. Change the mount points from `/cargo`, `/xargo`, and `/rust` to the same paths as on the host, which avoids unnecessary recompilation when `cargo` and `cross` are intermittently used. --- .changes/947.json | 12 +++ src/bin/commands/containers.rs | 7 +- src/docker/image.rs | 6 ++ src/docker/local.rs | 28 +++++-- src/docker/remote.rs | 131 ++++++++++++++------------------- src/docker/shared.rs | 97 +++++++++++++++++++----- src/file.rs | 10 ++- src/lib.rs | 12 +++ 8 files changed, 196 insertions(+), 107 deletions(-) create mode 100644 .changes/947.json diff --git a/.changes/947.json b/.changes/947.json new file mode 100644 index 000000000..ec5243eb1 --- /dev/null +++ b/.changes/947.json @@ -0,0 +1,12 @@ +[ + { + "type": "internal", + "description": "resolve symlinks to cargo and xargo home before mounting", + "issues": [373] + }, + { + "type": "fixed", + "description": "mount cargo, xargo, and the sysroot at the same path as on the host to avoid unnecessary recompilation.", + "issues": [551] + } +] diff --git a/src/bin/commands/containers.rs b/src/bin/commands/containers.rs index f602b59c8..973609d9e 100644 --- a/src/bin/commands/containers.rs +++ b/src/bin/commands/containers.rs @@ -451,15 +451,14 @@ pub fn create_persistent_volume( docker::remote::copy_volume_container_xargo( engine, &container, - &dirs.xargo, - &toolchain_host, + &dirs, mount_prefix.as_ref(), msg_info, )?; docker::remote::copy_volume_container_cargo( engine, &container, - &dirs.cargo, + &dirs, mount_prefix.as_ref(), copy_registry, msg_info, @@ -467,7 +466,7 @@ pub fn create_persistent_volume( docker::remote::copy_volume_container_rust( engine, &container, - &toolchain, + &dirs, None, mount_prefix.as_ref(), msg_info, diff --git a/src/docker/image.rs b/src/docker/image.rs index 7bfd5b9f5..266c5813c 100644 --- a/src/docker/image.rs +++ b/src/docker/image.rs @@ -135,6 +135,12 @@ impl ImagePlatform { } } +impl Default for ImagePlatform { + fn default() -> ImagePlatform { + ImagePlatform::DEFAULT + } +} + impl TryFrom<&str> for ImagePlatform { type Error = ::Err; diff --git a/src/docker/local.rs b/src/docker/local.rs index e11b2bb8c..665bb385e 100644 --- a/src/docker/local.rs +++ b/src/docker/local.rs @@ -27,7 +27,13 @@ pub(crate) fn run( .image .platform .specify_platform(&options.engine, &mut docker); - docker_envvars(&mut docker, &options.config, &options.target, msg_info)?; + docker_envvars( + &mut docker, + &options.config, + dirs, + &options.target, + msg_info, + )?; docker_mount( &mut docker, @@ -44,10 +50,16 @@ pub(crate) fn run( docker_user_id(&mut docker, engine.kind); docker - .args(&["-v", &format!("{}:/xargo:Z", dirs.xargo.to_utf8()?)]) - .args(&["-v", &format!("{}:/cargo:Z", dirs.cargo.to_utf8()?)]) + .args(&[ + "-v", + &format!("{}:{}:Z", dirs.xargo.to_utf8()?, dirs.xargo_mount_path()), + ]) + .args(&[ + "-v", + &format!("{}:{}:Z", dirs.cargo.to_utf8()?, dirs.cargo_mount_path()), + ]) // Prevent `bin` from being mounted inside the Docker container. - .args(&["-v", "/cargo/bin"]); + .args(&["-v", &format!("{}/bin", dirs.cargo_mount_path())]); docker.args(&[ "-v", &format!("{}:{}:Z", dirs.host_root.to_utf8()?, dirs.mount_root), @@ -55,7 +67,11 @@ pub(crate) fn run( docker .args(&[ "-v", - &format!("{}:/rust:Z,ro", paths.get_sysroot().to_utf8()?), + &format!( + "{}:{}:Z,ro", + dirs.get_sysroot().to_utf8()?, + dirs.sysroot_mount_path() + ), ]) .args(&["-v", &format!("{}:/target:Z", dirs.target.to_utf8()?)]); docker_cwd(&mut docker, &paths)?; @@ -84,7 +100,7 @@ pub(crate) fn run( docker .arg(&image_name) - .args(&["sh", "-c", &format!("PATH=$PATH:/rust/bin {:?}", cmd)]) + .args(&["sh", "-c", &build_command(dirs, &cmd)]) .run_and_get_status(msg_info, false) .map_err(Into::into) } diff --git a/src/docker/remote.rs b/src/docker/remote.rs index 5a97a807c..bdb2926e8 100644 --- a/src/docker/remote.rs +++ b/src/docker/remote.rs @@ -241,24 +241,19 @@ fn copy_volume_files_nocache( pub fn copy_volume_container_xargo( engine: &Engine, container: &str, - xargo_dir: &Path, - target: &Target, + dirs: &Directories, mount_prefix: &Path, msg_info: &mut MessageInfo, ) -> Result<()> { - // only need to copy the rustlib files for our current target. - let triple = target.triple(); - let relpath = Path::new("lib").join("rustlib").join(&triple); - let src = xargo_dir.join(&relpath); - let dst = mount_prefix.join("xargo").join(&relpath); - if Path::new(&src).exists() { + let dst = mount_prefix.join(&dirs.xargo_mount_path_relative()?); + if dirs.xargo.exists() { create_volume_dir( engine, container, dst.parent().expect("destination should have a parent"), msg_info, )?; - copy_volume_files(engine, container, &src, &dst, msg_info)?; + copy_volume_files(engine, container, &dirs.xargo, &dst, msg_info)?; } Ok(()) @@ -267,23 +262,23 @@ pub fn copy_volume_container_xargo( pub fn copy_volume_container_cargo( engine: &Engine, container: &str, - cargo_dir: &Path, + dirs: &Directories, mount_prefix: &Path, copy_registry: bool, msg_info: &mut MessageInfo, ) -> Result<()> { - let dst = mount_prefix.join("cargo"); + let dst = mount_prefix.join(&dirs.cargo_mount_path_relative()?); let copy_registry = env::var("CROSS_REMOTE_COPY_REGISTRY") .map(|s| bool_from_envvar(&s)) .unwrap_or(copy_registry); if copy_registry { - copy_volume_files(engine, container, cargo_dir, &dst, msg_info)?; + copy_volume_files(engine, container, &dirs.cargo, &dst, msg_info)?; } else { // can copy a limit subset of files: the rest is present. create_volume_dir(engine, container, &dst, msg_info)?; - for entry in fs::read_dir(cargo_dir) - .wrap_err_with(|| format!("when reading directory {cargo_dir:?}"))? + for entry in fs::read_dir(&dirs.cargo) + .wrap_err_with(|| format!("when reading directory {:?}", dirs.cargo))? { let file = entry?; let basename = file @@ -371,17 +366,17 @@ fn warn_symlinks(had_symlinks: bool, msg_info: &mut MessageInfo) -> Result<()> { fn copy_volume_container_rust_base( engine: &Engine, container: &str, - sysroot: &Path, + dirs: &Directories, mount_prefix: &Path, msg_info: &mut MessageInfo, ) -> Result<()> { // the rust toolchain is quite large, but most of it isn't needed // we need the bin, libexec, and etc directories, and part of the lib directory. - let dst = mount_prefix.join("rust"); + let dst = mount_prefix.join(&dirs.sysroot_mount_path_relative()?); let rustlib = Path::new("lib").join("rustlib"); create_volume_dir(engine, container, &dst.join(&rustlib), msg_info)?; for basename in ["bin", "libexec", "etc"] { - let file = sysroot.join(basename); + let file = dirs.get_sysroot().join(basename); copy_volume_files(engine, container, &file, &dst, msg_info)?; } @@ -393,9 +388,9 @@ fn copy_volume_container_rust_base( // SAFETY: safe, single-threaded execution. let tempdir = unsafe { temp::TempDir::new()? }; let temppath = tempdir.path(); - fs::create_dir_all(&temppath.join(&rustlib))?; + file::create_dir_all(&temppath.join(&rustlib))?; let mut had_symlinks = copy_dir( - &sysroot.join("lib"), + &dirs.get_sysroot().join("lib"), &temppath.join("lib"), true, 0, @@ -404,7 +399,7 @@ fn copy_volume_container_rust_base( // next, copy the src/etc directories inside rustlib had_symlinks |= copy_dir( - &sysroot.join(&rustlib), + &dirs.get_sysroot().join(&rustlib), &temppath.join(&rustlib), true, 0, @@ -418,21 +413,21 @@ fn copy_volume_container_rust_base( fn copy_volume_container_rust_manifest( engine: &Engine, container: &str, - sysroot: &Path, + dirs: &Directories, mount_prefix: &Path, msg_info: &mut MessageInfo, ) -> Result<()> { // copy over all the manifest files in rustlib // these are small text files containing names/paths to toolchains - let dst = mount_prefix.join("rust"); + let dst = mount_prefix.join(&dirs.sysroot_mount_path_relative()?); let rustlib = Path::new("lib").join("rustlib"); // SAFETY: safe, single-threaded execution. let tempdir = unsafe { temp::TempDir::new()? }; let temppath = tempdir.path(); - fs::create_dir_all(&temppath.join(&rustlib))?; + file::create_dir_all(&temppath.join(&rustlib))?; let had_symlinks = copy_dir( - &sysroot.join(&rustlib), + &dirs.get_sysroot().join(&rustlib), &temppath.join(&rustlib), true, 0, @@ -447,18 +442,20 @@ fn copy_volume_container_rust_manifest( pub fn copy_volume_container_rust_triple( engine: &Engine, container: &str, - toolchain: &QualifiedToolchain, + dirs: &Directories, target_triple: &TargetTriple, mount_prefix: &Path, skip_exists: bool, msg_info: &mut MessageInfo, ) -> Result<()> { - let sysroot = toolchain.get_sysroot(); // copy over the files for a specific triple - let dst = mount_prefix.join("rust"); + let dst = mount_prefix.join(&dirs.sysroot_mount_path_relative()?); let rustlib = Path::new("lib").join("rustlib"); let dst_rustlib = dst.join(&rustlib); - let src_toolchain = sysroot.join(&rustlib).join(target_triple.triple()); + let src_toolchain = dirs + .get_sysroot() + .join(&rustlib) + .join(target_triple.triple()); let dst_toolchain = dst_rustlib.join(target_triple.triple()); // skip if the toolchain target component already exists. for the host toolchain @@ -473,7 +470,7 @@ pub fn copy_volume_container_rust_triple( if !skip && skip_exists { // this means we have a persistent data volume and we have a // new target, meaning we might have new manifests as well. - copy_volume_container_rust_manifest(engine, container, sysroot, mount_prefix, msg_info)?; + copy_volume_container_rust_manifest(engine, container, dirs, mount_prefix, msg_info)?; } Ok(()) @@ -482,41 +479,28 @@ pub fn copy_volume_container_rust_triple( pub fn copy_volume_container_rust( engine: &Engine, container: &str, - toolchain: &QualifiedToolchain, + dirs: &Directories, target_triple: Option<&TargetTriple>, mount_prefix: &Path, msg_info: &mut MessageInfo, ) -> Result<()> { - copy_volume_container_rust_base( - engine, - container, - toolchain.get_sysroot(), - mount_prefix, - msg_info, - )?; - copy_volume_container_rust_manifest( - engine, - container, - toolchain.get_sysroot(), - mount_prefix, - msg_info, - )?; + copy_volume_container_rust_base(engine, container, dirs, mount_prefix, msg_info)?; + copy_volume_container_rust_manifest(engine, container, dirs, mount_prefix, msg_info)?; copy_volume_container_rust_triple( engine, container, - toolchain, - &toolchain.host().target, + dirs, + &dirs.toolchain.host().target, mount_prefix, false, msg_info, )?; - // TODO: impl Eq if let Some(target_triple) = target_triple { - if target_triple.triple() != toolchain.host().target.triple() { + if target_triple.triple() != dirs.toolchain.host().target.triple() { copy_volume_container_rust_triple( engine, container, - toolchain, + dirs, target_triple, mount_prefix, false, @@ -631,7 +615,7 @@ fn copy_volume_file_list( for file in files { let src_path = src.join(file); let dst_path = temppath.join(file); - fs::create_dir_all(dst_path.parent().expect("must have parent"))?; + file::create_dir_all(dst_path.parent().expect("must have parent"))?; fs::copy(&src_path, &dst_path)?; } copy_volume_files(engine, container, temppath, dst, msg_info) @@ -698,7 +682,7 @@ fn copy_volume_container_project( match volume { VolumeId::Keep(_) => { let parent = temp::dir()?; - fs::create_dir_all(&parent)?; + file::create_dir_all(&parent)?; let fingerprint = parent.join(container); let current = get_project_fingerprint(src, copy_cache)?; // need to check if the container path exists, otherwise we might @@ -997,28 +981,14 @@ pub(crate) fn run( }; let mount_prefix_path = mount_prefix.as_ref(); if let VolumeId::Discard = volume { - copy_volume_container_xargo( - engine, - &container, - &dirs.xargo, - target, - mount_prefix_path, - msg_info, - ) - .wrap_err("when copying xargo")?; - copy_volume_container_cargo( - engine, - &container, - &dirs.cargo, - mount_prefix_path, - false, - msg_info, - ) - .wrap_err("when copying cargo")?; + copy_volume_container_xargo(engine, &container, dirs, mount_prefix_path, msg_info) + .wrap_err("when copying xargo")?; + copy_volume_container_cargo(engine, &container, dirs, mount_prefix_path, false, msg_info) + .wrap_err("when copying cargo")?; copy_volume_container_rust( engine, &container, - &dirs.toolchain, + dirs, Some(target.target()), mount_prefix_path, msg_info, @@ -1029,7 +999,7 @@ pub(crate) fn run( copy_volume_container_rust_triple( engine, &container, - &dirs.toolchain, + dirs, target.target(), mount_prefix_path, true, @@ -1066,9 +1036,18 @@ pub(crate) fn run( .wrap_err("when copying project")?; let sysroot = dirs.get_sysroot().to_owned(); let mut copied = vec![ - (&dirs.xargo, mount_prefix_path.join("xargo")), - (&dirs.cargo, mount_prefix_path.join("cargo")), - (&sysroot, mount_prefix_path.join("rust")), + ( + &dirs.xargo, + mount_prefix_path.join(&dirs.xargo_mount_path_relative()?), + ), + ( + &dirs.cargo, + mount_prefix_path.join(&dirs.cargo_mount_path_relative()?), + ), + ( + &sysroot, + mount_prefix_path.join(&dirs.sysroot_mount_path_relative()?), + ), (&dirs.host_root, mount_root.clone()), ]; let mut to_symlink = vec![]; @@ -1188,10 +1167,10 @@ symlink_recurse \"${{prefix}}\" // 6. execute our cargo command inside the container let mut docker = subcommand(engine, "exec"); docker_user_id(&mut docker, engine.kind); - docker_envvars(&mut docker, &options.config, target, msg_info)?; + docker_envvars(&mut docker, &options.config, dirs, target, msg_info)?; docker_cwd(&mut docker, &paths)?; docker.arg(&container); - docker.args(&["sh", "-c", &format!("PATH=$PATH:/rust/bin {:?}", cmd)]); + docker.args(&["sh", "-c", &build_command(dirs, &cmd)]); bail_container_exited!(); let status = docker .run_and_get_status(msg_info, false) diff --git a/src/docker/shared.rs b/src/docker/shared.rs index 7e1459b93..1e572b095 100644 --- a/src/docker/shared.rs +++ b/src/docker/shared.rs @@ -256,6 +256,9 @@ pub struct Directories { pub mount_root: String, pub mount_cwd: String, pub toolchain: QualifiedToolchain, + pub cargo_mount_path: String, + pub xargo_mount_path: String, + pub sysroot_mount_path: String, } impl Directories { @@ -277,10 +280,22 @@ impl Directories { // otherwise `docker` will create them but they will be owned by `root` // cargo builds all intermediate directories, but fails // if it has other issues (such as permission errors). - fs::create_dir_all(&cargo)?; - fs::create_dir_all(&xargo)?; + file::create_dir_all(&cargo)?; + file::create_dir_all(&xargo)?; + if let Some(ref nix_store) = nix_store { + file::create_dir_all(nix_store)?; + } create_target_dir(target)?; + // now that we know the paths exist, canonicalize them. this avoids creating + // directories after failed canonicalization into a shared directory. + let cargo = file::canonicalize(&cargo)?; + let xargo = file::canonicalize(&xargo)?; + let nix_store = match nix_store { + Some(store) => Some(file::canonicalize(&store)?), + None => None, + }; + let cargo = mount_finder.find_mount_path(cargo); let xargo = mount_finder.find_mount_path(xargo); let target = mount_finder.find_mount_path(target); @@ -293,21 +308,17 @@ impl Directories { }); // root is either workspace_root, or, if we're outside the workspace root, the current directory - let mount_root: String; - #[cfg(target_os = "windows")] - { - // On Windows, we can not mount the directory name directly. Instead, we use wslpath to convert the path to a linux compatible path. - mount_root = host_root.as_wslpath()?; - } - #[cfg(not(target_os = "windows"))] - { - // NOTE: host root has already found the mount path - mount_root = host_root.to_utf8()?.to_owned(); - } + // NOTE: host root has already found the mount path + let mount_root = canonicalize_mount_path(&host_root)?; let mount_cwd = mount_finder.find_path(cwd, false)?; toolchain.set_sysroot(|p| mount_finder.find_mount_path(p)); + // canonicalize these once to avoid syscalls + let cargo_mount_path = canonicalize_mount_path(&cargo)?; + let xargo_mount_path = canonicalize_mount_path(&xargo)?; + let sysroot_mount_path = canonicalize_mount_path(toolchain.get_sysroot())?; + Ok(Directories { cargo, xargo, @@ -317,12 +328,48 @@ impl Directories { mount_root, mount_cwd, toolchain, + cargo_mount_path, + xargo_mount_path, + sysroot_mount_path, }) } pub fn get_sysroot(&self) -> &Path { self.toolchain.get_sysroot() } + + pub fn cargo_mount_path(&self) -> &str { + &self.cargo_mount_path + } + + pub fn xargo_mount_path(&self) -> &str { + &self.xargo_mount_path + } + + pub fn sysroot_mount_path(&self) -> &str { + &self.sysroot_mount_path + } + + pub fn cargo_mount_path_relative(&self) -> Result { + self.cargo_mount_path() + .strip_prefix('/') + .map(ToOwned::to_owned) + .ok_or_else(|| eyre::eyre!("cargo directory must be relative to root")) + } + + pub fn xargo_mount_path_relative(&self) -> Result { + self.xargo_mount_path() + .strip_prefix('/') + .map(ToOwned::to_owned) + .ok_or_else(|| eyre::eyre!("xargo directory must be relative to root")) + } + + pub fn sysroot_mount_path_relative(&self) -> Result { + self.sysroot_mount_path() + .strip_prefix('/') + .map(ToOwned::to_owned) + .ok_or_else(|| eyre::eyre!("sysroot directory must be relative to root")) + } } const CACHEDIR_TAG: &str = "Signature: 8a477f597d28d172789f06886806bc55 @@ -333,7 +380,7 @@ fn create_target_dir(path: &Path) -> Result<()> { // cargo creates all paths to the target directory, and writes // a cache dir tag only if the path doesn't previously exist. if !path.exists() { - fs::create_dir_all(&path)?; + file::create_dir_all(&path)?; fs::OpenOptions::new() .write(true) .create_new(true) @@ -467,6 +514,7 @@ pub(crate) fn mount(docker: &mut Command, host_path: &Path, prefix: &str) -> Res pub(crate) fn docker_envvars( docker: &mut Command, config: &Config, + dirs: &Directories, target: &Target, msg_info: &mut MessageInfo, ) -> Result<()> { @@ -482,8 +530,8 @@ pub(crate) fn docker_envvars( let cross_runner = format!("CROSS_RUNNER={}", runner.unwrap_or_default()); docker .args(&["-e", "PKG_CONFIG_ALLOW_CROSS=1"]) - .args(&["-e", "XARGO_HOME=/xargo"]) - .args(&["-e", "CARGO_HOME=/cargo"]) + .args(&["-e", &format!("XARGO_HOME={}", dirs.xargo_mount_path())]) + .args(&["-e", &format!("CARGO_HOME={}", dirs.cargo_mount_path())]) .args(&["-e", "CARGO_TARGET_DIR=/target"]) .args(&["-e", &cross_runner]); add_cargo_configuration_envvars(docker); @@ -513,6 +561,14 @@ pub(crate) fn docker_envvars( Ok(()) } +pub(crate) fn build_command(dirs: &Directories, cmd: &SafeCommand) -> String { + format!( + "PATH=\"$PATH\":\"{}/bin\" {:?}", + dirs.sysroot_mount_path(), + cmd + ) +} + pub(crate) fn docker_cwd(docker: &mut Command, paths: &DockerPaths) -> Result<()> { docker.args(&["-w", paths.mount_cwd()]); @@ -961,6 +1017,7 @@ mod tests { mod directories { use super::*; use crate::cargo::cargo_metadata_with_args; + use crate::rustc::{self, VersionMetaExt}; use crate::temp; fn unset_env() -> Vec<(&'static str, Option)> { @@ -1020,14 +1077,18 @@ mod tests { } fn get_toolchain() -> Result { + let host_version_meta = rustc::version_meta()?; + let host = host_version_meta.host(); + let image_platform = + crate::docker::ImagePlatform::from_const_target(host.triple().into()); let sysroot = home()? .join(".rustup") .join("toolchains") - .join("stable-x86_64-unknown-linux-gnu"); + .join(host.triple()); Ok(QualifiedToolchain::new( "stable", &None, - &crate::docker::ImagePlatform::X86_64_UNKNOWN_LINUX_GNU, + &image_platform, &sysroot, )) } diff --git a/src/file.rs b/src/file.rs index c2c976cfd..bebf2050b 100644 --- a/src/file.rs +++ b/src/file.rs @@ -143,6 +143,11 @@ where read_(path.as_ref()) } +pub fn create_dir_all(path: impl AsRef) -> Result<()> { + fs::create_dir_all(path.as_ref()) + .wrap_err_with(|| format!("couldn't create directory {:?}", path.as_ref())) +} + fn read_(path: &Path) -> Result { let mut s = String::new(); File::open(path) @@ -222,12 +227,11 @@ pub fn maybe_canonicalize(path: &Path) -> Cow<'_, OsStr> { pub fn write_file(path: impl AsRef, overwrite: bool) -> Result { let path = path.as_ref(); - fs::create_dir_all( + create_dir_all( &path .parent() .ok_or_else(|| eyre::eyre!("could not find parent directory for `{path:?}`"))?, - ) - .wrap_err_with(|| format!("couldn't create directory `{path:?}`"))?; + )?; let mut open = fs::OpenOptions::new(); open.write(true); diff --git a/src/lib.rs b/src/lib.rs index 58dd6dd12..b147006b8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -235,6 +235,12 @@ impl<'a> From<&'a str> for TargetTriple { } } +impl Default for TargetTriple { + fn default() -> TargetTriple { + TargetTriple::DEFAULT + } +} + impl std::str::FromStr for TargetTriple { type Err = std::convert::Infallible; @@ -367,6 +373,12 @@ impl Target { } } +impl Default for Target { + fn default() -> Target { + Target::DEFAULT + } +} + impl std::fmt::Display for Target { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.write_str(self.triple())