Merge #942

942: Use non-canonical paths for mount paths. r=Emilgardis a=Alexhuszagh

Symlinks and canonical paths can destroy assumptions about paths: say I have a file at `/tmp/path/to/file` I reference in my crate, and ask to mount `/tmp/path/to` on the container. Currently, on macOS, since `/tmp` is a symlink to `/private/tmp`, the code that expects `/tmp/path/to/file` will fail because in the container it will be mounted at `/private/tmp/path/to/file`.

In addition, this fixes `DeviceNS` parsing with drive letters (this previously didn't matter since it never occurred, due to `dunce` canonicalization). This PR also fixes a minor bug where the host root instead of the var names was passed as the environment variable (introduced in #904).

Closes #920.

Co-authored-by: Alex Huszagh <[email protected]>

Author: bors[bot]

.changes/942.json

@@ -0,0 +1,15 @@
+[
+    {
+        "description": "use non-canonical paths for mount locations.",
+        "type": "changed",
+        "issues": [920]
+    },
+    {
+        "description": "fixed DeviceNS drive parsing in creating WSL-style paths on windows.",
+        "type": "fixed"
+    },
+    {
+        "description": "fixed the environment variable name for mounted volumes.",
+        "type": "fixed"
+    }
+]

src/cli.rs

@@ -3,7 +3,7 @@ use std::path::{Path, PathBuf};
 
 use crate::cargo::Subcommand;
 use crate::errors::Result;
-use crate::file::PathExt;
+use crate::file::{absolute_path, PathExt};
 use crate::rustc::TargetList;
 use crate::shell::{self, MessageInfo};
 use crate::Target;
@@ -23,15 +23,6 @@ pub struct Args {
     pub color: Option<String>,
 }
 
-// Fix for issue #581. target_dir must be absolute.
-fn absolute_path(path: PathBuf) -> Result<PathBuf> {
-    Ok(if path.is_absolute() {
-        path
-    } else {
-        env::current_dir()?.join(path)
-    })
-}
-
 pub fn is_subcommand_list(stdout: &str) -> bool {
     stdout.starts_with("Installed Commands:")
 }
@@ -151,22 +142,8 @@ fn str_to_owned(arg: &str) -> Result<String> {
     Ok(arg.to_owned())
 }
 
-#[allow(clippy::needless_return)]
 fn store_manifest_path(path: String) -> Result<String> {
-    let p = Path::new(&path);
-    if p.is_absolute() {
-        #[cfg(target_os = "windows")]
-        {
-            return p.as_wslpath();
-        }
-        #[cfg(not(target_os = "windows"))]
-        {
-            use crate::file::ToUtf8;
-            return p.to_utf8().map(ToOwned::to_owned);
-        }
-    } else {
-        p.as_posix()
-    }
+    Path::new(&path).as_posix_relative()
 }
 
 fn store_target_dir(_: String) -> Result<String> {

src/docker/local.rs

@@ -1,5 +1,6 @@
 use std::io;
-use std::process::ExitStatus;
+use std::path::Path;
+use std::process::{Command, ExitStatus};
 
 use super::shared::*;
 use crate::errors::Result;
@@ -8,6 +9,16 @@ use crate::file::{PathExt, ToUtf8};
 use crate::shell::{MessageInfo, Stream};
 use eyre::Context;
 
+// NOTE: host path must be absolute
+fn mount(docker: &mut Command, host_path: &Path, absolute_path: &Path, prefix: &str) -> Result<()> {
+    let mount_path = absolute_path.as_posix_absolute()?;
+    docker.args(&[
+        "-v",
+        &format!("{}:{prefix}{}", host_path.to_utf8()?, mount_path),
+    ]);
+    Ok(())
+}
+
 pub(crate) fn run(
     options: DockerOptions,
     paths: DockerPaths,
@@ -39,7 +50,7 @@ pub(crate) fn run(
         &mut docker,
         &options,
         &paths,
-        |docker, val| mount(docker, val, ""),
+        |docker, host, absolute| mount(docker, host, absolute, ""),
         |_| {},
     )?;
 
@@ -81,7 +92,11 @@ pub(crate) fn run(
     if let Some(ref nix_store) = dirs.nix_store {
         docker.args(&[
             "-v",
-            &format!("{}:{}:Z", nix_store.to_utf8()?, nix_store.as_posix()?),
+            &format!(
+                "{}:{}:Z",
+                nix_store.to_utf8()?,
+                nix_store.as_posix_absolute()?
+            ),
         ]);
     }
 

src/docker/remote.rs

@@ -168,7 +168,11 @@ fn create_volume_dir(
     // make our parent directory if needed
     subcommand_or_exit(engine, "exec")?
         .arg(container)
-        .args(&["sh", "-c", &format!("mkdir -p '{}'", dir.as_posix()?)])
+        .args(&[
+            "sh",
+            "-c",
+            &format!("mkdir -p '{}'", dir.as_posix_absolute()?),
+        ])
         .run_and_get_status(msg_info, false)
 }
 
@@ -183,7 +187,7 @@ fn copy_volume_files(
     subcommand_or_exit(engine, "cp")?
         .arg("-a")
         .arg(src.to_utf8()?)
-        .arg(format!("{container}:{}", dst.as_posix()?))
+        .arg(format!("{container}:{}", dst.as_posix_absolute()?))
         .run_and_get_status(msg_info, false)
 }
 
@@ -214,7 +218,11 @@ fn container_path_exists(
 ) -> Result<bool> {
     Ok(subcommand_or_exit(engine, "exec")?
         .arg(container)
-        .args(&["bash", "-c", &format!("[[ -d '{}' ]]", path.as_posix()?)])
+        .args(&[
+            "bash",
+            "-c",
+            &format!("[[ -d '{}' ]]", path.as_posix_absolute()?),
+        ])
         .run_and_get_status(msg_info, true)?
         .success())
 }
@@ -567,7 +575,7 @@ fn read_dir_fingerprint(
             let modified = file.metadata()?.modified()?;
             let millis = modified.duration_since(epoch)?.as_millis() as u64;
             let rounded = epoch + time::Duration::from_millis(millis);
-            let relpath = file.path().strip_prefix(home)?.as_posix()?;
+            let relpath = file.path().strip_prefix(home)?.as_posix_relative()?;
             map.insert(relpath, rounded);
         }
     }
@@ -647,7 +655,7 @@ rm \"{PATH}\"
     // SAFETY: safe, single-threaded execution.
     let mut tempfile = unsafe { temp::TempFile::new()? };
     for file in files {
-        writeln!(tempfile.file(), "{}", dst.join(file).as_posix()?)?;
+        writeln!(tempfile.file(), "{}", dst.join(file).as_posix_relative()?)?;
     }
 
     // need to avoid having hundreds of files on the command, so
@@ -845,11 +853,6 @@ pub fn unique_container_identifier(
     Ok(format!("{toolchain_id}-{triple}-{name}-{project_hash}"))
 }
 
-fn mount_path(val: &Path) -> Result<String> {
-    let host_path = file::canonicalize(val)?;
-    canonicalize_mount_path(&host_path)
-}
-
 pub(crate) fn run(
     options: DockerOptions,
     paths: DockerPaths,
@@ -921,7 +924,7 @@ pub(crate) fn run(
         &mut docker,
         &options,
         &paths,
-        |_, val| mount_path(val),
+        |_, _, _| Ok(()),
         |(src, dst)| volumes.push((src, dst)),
     )
     .wrap_err("could not determine mount points")?;
@@ -1100,7 +1103,7 @@ pub(crate) fn run(
     let mut final_args = vec![];
     let mut iter = args.iter().cloned();
     let mut has_target_dir = false;
-    let target_dir_string = target_dir.as_posix()?;
+    let target_dir_string = target_dir.as_posix_absolute()?;
     while let Some(arg) = iter.next() {
         if arg == "--target-dir" {
             has_target_dir = true;
@@ -1156,7 +1159,11 @@ symlink_recurse \"${{prefix}}\"
 "
     ));
     for (src, dst) in to_symlink {
-        symlink.push(format!("ln -s \"{}\" \"{}\"", src.as_posix()?, dst));
+        symlink.push(format!(
+            "ln -s \"{}\" \"{}\"",
+            src.as_posix_absolute()?,
+            dst
+        ));
     }
     subcommand_or_exit(engine, "exec")?
         .arg(&container)
@@ -1185,7 +1192,7 @@ symlink_recurse \"${{prefix}}\"
     if !skip_artifacts && container_path_exists(engine, &container, &target_dir, msg_info)? {
         subcommand_or_exit(engine, "cp")?
             .arg("-a")
-            .arg(&format!("{container}:{}", target_dir.as_posix()?))
+            .arg(&format!("{container}:{}", target_dir.as_posix_absolute()?))
             .arg(
                 &dirs
                     .target