Merge #828

828: Avoid using display on paths. r=Emilgardis a=Alexhuszagh

Adds a helper trait `ToUtf8` which converts the path to UTF-8 if possible, and if not, creates a pretty debugging representation of the path. We can then change `display()` to `to_utf8()?` and be completely correct in all cases.

On POSIX systems, this doesn't matter since paths must be UTF-8 anyway, but on Windows some paths can be WTF-8 (see rust-lang/rust#12056). Either way, this can avoid creating a copy in cases, is always correct, and is more idiomatic about what we're doing. We might not be able to handle non-UTF-8 paths currently (like ISO-8859-1 paths, which are historically very common).

So, this doesn't change ergonomics: the resulting code is as compact and correct. It also requires less copies in most cases, which should be a good thing. But most importantly, if we're mounting data we can't silently fail or produce incorrect data. If something was lossily generated, we probably shouldn't try to mount with a replacement character, and also print more information than there was just an invalid Unicode character.

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

Author: bors[bot]

CHANGELOG.md

@@ -73,6 +73,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Internal
 
+- #828 - assume paths are Unicode and provide better error messages for path encoding errors.
 - #787 - add installer for git hooks.
 - #786, #791 - Migrate build script to rust: `cargo build-docker-image $TARGET`
 - #730 - make FreeBSD builds more resilient.

clippy.toml

@@ -0,0 +1,3 @@
+disallowed-methods = [
+    { path = "std::path::Path::display", reason = "incorrect handling of non-Unicode paths, use path.to_utf8() or debug (`{path:?}`) instead" },
+]

src/docker/custom.rs

@@ -3,7 +3,7 @@ use std::path::{Path, PathBuf};
 
 use crate::docker::Engine;
 use crate::{config::Config, docker, CargoMetadata, Target};
-use crate::{errors::*, file, CommandExt};
+use crate::{errors::*, file, CommandExt, ToUtf8};
 
 use super::{image_name, parse_docker_opts};
 
@@ -47,7 +47,7 @@ impl<'a> Dockerfile<'a> {
             &format!(
                 "{}.workspace_root={}",
                 crate::CROSS_LABEL_DOMAIN,
-                metadata.workspace_root.display()
+                metadata.workspace_root.to_utf8()?
             ),
         ]);
 

src/docker/local.rs

@@ -6,6 +6,7 @@ use super::shared::*;
 use crate::cargo::CargoMetadata;
 use crate::errors::Result;
 use crate::extensions::CommandExt;
+use crate::file::ToUtf8;
 use crate::{Config, Target};
 use atty::Stream;
 use eyre::Context;
@@ -49,33 +50,33 @@ pub(crate) fn run(
     docker_user_id(&mut docker, engine.kind);
 
     docker
-        .args(&["-v", &format!("{}:/xargo:Z", dirs.xargo.display())])
-        .args(&["-v", &format!("{}:/cargo:Z", dirs.cargo.display())])
+        .args(&["-v", &format!("{}:/xargo:Z", dirs.xargo.to_utf8()?)])
+        .args(&["-v", &format!("{}:/cargo:Z", dirs.cargo.to_utf8()?)])
         // Prevent `bin` from being mounted inside the Docker container.
         .args(&["-v", "/cargo/bin"]);
     if mount_volumes {
         docker.args(&[
             "-v",
             &format!(
                 "{}:{}:Z",
-                dirs.host_root.display(),
-                dirs.mount_root.display()
+                dirs.host_root.to_utf8()?,
+                dirs.mount_root.to_utf8()?
             ),
         ]);
     } else {
-        docker.args(&["-v", &format!("{}:/project:Z", dirs.host_root.display())]);
+        docker.args(&["-v", &format!("{}:/project:Z", dirs.host_root.to_utf8()?)]);
     }
     docker
-        .args(&["-v", &format!("{}:/rust:Z,ro", dirs.sysroot.display())])
-        .args(&["-v", &format!("{}:/target:Z", dirs.target.display())]);
+        .args(&["-v", &format!("{}:/rust:Z,ro", dirs.sysroot.to_utf8()?)])
+        .args(&["-v", &format!("{}:/target:Z", dirs.target.to_utf8()?)]);
     docker_cwd(&mut docker, metadata, &dirs, cwd, mount_volumes)?;
 
     // When running inside NixOS or using Nix packaging we need to add the Nix
     // Store to the running container so it can load the needed binaries.
     if let Some(ref nix_store) = dirs.nix_store {
         docker.args(&[
             "-v",
-            &format!("{}:{}:Z", nix_store.display(), nix_store.display()),
+            &format!("{}:{}:Z", nix_store.to_utf8()?, nix_store.to_utf8()?),
         ]);
     }
 

src/docker/shared.rs

@@ -9,7 +9,7 @@ use crate::cargo::CargoMetadata;
 use crate::config::Config;
 use crate::errors::*;
 use crate::extensions::{CommandExt, SafeCommand};
-use crate::file::{self, write_file};
+use crate::file::{self, write_file, PathExt, ToUtf8};
 use crate::id;
 use crate::Target;
 
@@ -180,7 +180,7 @@ pub(crate) fn mount(
     let mount_path = canonicalize_mount_path(&host_path, verbose)?;
     docker.args(&[
         "-v",
-        &format!("{}:{prefix}{}", host_path.display(), mount_path.display()),
+        &format!("{}:{prefix}{}", host_path.to_utf8()?, mount_path.to_utf8()?),
     ]);
     Ok(mount_path)
 }
@@ -236,20 +236,14 @@ pub(crate) fn docker_cwd(
     mount_volumes: bool,
 ) -> Result<()> {
     if mount_volumes {
-        docker.args(&["-w".as_ref(), dirs.mount_cwd.as_os_str()]);
+        docker.args(&["-w", dirs.mount_cwd.to_utf8()?]);
     } else if dirs.mount_cwd == metadata.workspace_root {
         docker.args(&["-w", "/project"]);
     } else {
         // We do this to avoid clashes with path separators. Windows uses `\` as a path separator on Path::join
         let cwd = &cwd;
         let working_dir = Path::new("project").join(cwd.strip_prefix(&metadata.workspace_root)?);
-        // No [T].join for OsStr
-        let mut mount_wd = std::ffi::OsString::new();
-        for part in working_dir.iter() {
-            mount_wd.push("/");
-            mount_wd.push(part);
-        }
-        docker.args(&["-w".as_ref(), mount_wd.as_os_str()]);
+        docker.args(&["-w", &working_dir.as_posix()?]);
     }
 
     Ok(())
@@ -282,15 +276,15 @@ pub(crate) fn docker_mount(
 
         if let Ok(val) = value {
             let mount_path = mount_cb(docker, val.as_ref(), verbose)?;
-            docker.args(&["-e", &format!("{}={}", var, mount_path.display())]);
+            docker.args(&["-e", &format!("{}={}", var, mount_path.to_utf8()?)]);
             store_cb((val, mount_path));
             mount_volumes = true;
         }
     }
 
     for path in metadata.path_dependencies() {
         let mount_path = mount_cb(docker, path, verbose)?;
-        store_cb((path.display().to_string(), mount_path));
+        store_cb((path.to_utf8()?.to_string(), mount_path));
         mount_volumes = true;
     }
 
@@ -310,12 +304,7 @@ fn wslpath(path: &Path, verbose: bool) -> Result<PathBuf> {
         .arg("-a")
         .arg(path)
         .run_and_get_stdout(verbose)
-        .wrap_err_with(|| {
-            format!(
-                "could not get linux compatible path for `{}`",
-                path.display()
-            )
-        })
+        .wrap_err_with(|| format!("could not get linux compatible path for `{path:?}`"))
         .map(|s| s.trim().into())
 }
 
@@ -375,7 +364,7 @@ pub(crate) fn docker_seccomp(
                 // podman weirdly expects a WSL path here, and fails otherwise
                 path = wslpath(&path, verbose)?;
             }
-            path.display().to_string()
+            path.to_utf8()?.to_string()
         };
 
         docker.args(&["--security-opt", &format!("seccomp={}", seccomp)]);

src/file.rs

@@ -2,10 +2,61 @@ use std::borrow::Cow;
 use std::ffi::OsStr;
 use std::fs::{self, File};
 use std::io::Read;
-use std::path::{Path, PathBuf};
+use std::path::{Component, Path, PathBuf};
 
 use crate::errors::*;
 
+pub trait ToUtf8 {
+    fn to_utf8(&self) -> Result<&str>;
+}
+
+impl ToUtf8 for OsStr {
+    fn to_utf8(&self) -> Result<&str> {
+        self.to_str()
+            .ok_or_else(|| eyre::eyre!("unable to convert `{self:?}` to UTF-8 string"))
+    }
+}
+
+impl ToUtf8 for Path {
+    fn to_utf8(&self) -> Result<&str> {
+        self.as_os_str().to_utf8()
+    }
+}
+
+pub trait PathExt {
+    fn as_posix(&self) -> Result<String>;
+}
+
+fn push_posix_path(path: &mut String, component: &str) {
+    if !path.is_empty() && path != "/" {
+        path.push('/');
+    }
+    path.push_str(component);
+}
+
+impl PathExt for Path {
+    fn as_posix(&self) -> Result<String> {
+        if cfg!(target_os = "windows") {
+            // iterate over components to join them
+            let mut output = String::new();
+            for component in self.components() {
+                match component {
+                    Component::Prefix(prefix) => {
+                        eyre::bail!("unix paths cannot handle windows prefix {prefix:?}.")
+                    }
+                    Component::RootDir => output = "/".to_string(),
+                    Component::CurDir => push_posix_path(&mut output, "."),
+                    Component::ParentDir => push_posix_path(&mut output, ".."),
+                    Component::Normal(path) => push_posix_path(&mut output, path.to_utf8()?),
+                }
+            }
+            Ok(output)
+        } else {
+            self.to_utf8().map(|x| x.to_string())
+        }
+    }
+}
+
 pub fn read<P>(path: P) -> Result<String>
 where
     P: AsRef<Path>,
@@ -16,9 +67,9 @@ where
 fn read_(path: &Path) -> Result<String> {
     let mut s = String::new();
     File::open(path)
-        .wrap_err_with(|| format!("couldn't open {}", path.display()))?
+        .wrap_err_with(|| format!("couldn't open {path:?}"))?
         .read_to_string(&mut s)
-        .wrap_err_with(|| format!("couldn't read {}", path.display()))?;
+        .wrap_err_with(|| format!("couldn't read {path:?}"))?;
     Ok(s)
 }
 
@@ -90,11 +141,11 @@ pub fn maybe_canonicalize(path: &Path) -> Cow<'_, OsStr> {
 pub fn write_file(path: impl AsRef<Path>, overwrite: bool) -> Result<File> {
     let path = path.as_ref();
     fs::create_dir_all(
-        &path.parent().ok_or_else(|| {
-            eyre::eyre!("could not find parent directory for `{}`", path.display())
-        })?,
+        &path
+            .parent()
+            .ok_or_else(|| eyre::eyre!("could not find parent directory for `{path:?}`"))?,
     )
-    .wrap_err_with(|| format!("couldn't create directory `{}`", path.display()))?;
+    .wrap_err_with(|| format!("couldn't create directory `{path:?}`"))?;
 
     let mut open = fs::OpenOptions::new();
     open.write(true);
@@ -106,12 +157,41 @@ pub fn write_file(path: impl AsRef<Path>, overwrite: bool) -> Result<File> {
     }
 
     open.open(&path)
-        .wrap_err(format!("couldn't write to file `{}`", path.display()))
+        .wrap_err(format!("couldn't write to file `{path:?}`"))
 }
 
 #[cfg(test)]
 mod tests {
     use super::*;
+    use std::fmt::Debug;
+
+    fn result_eq<T: PartialEq + Eq + Debug>(x: Result<T>, y: Result<T>) {
+        match (x, y) {
+            (Ok(x), Ok(y)) => assert_eq!(x, y),
+            (x, y) => panic!("assertion failed: `(left == right)`\nleft: {x:?}\nright: {y:?}"),
+        }
+    }
+
+    #[test]
+    fn as_posix() {
+        result_eq(Path::new(".").join("..").as_posix(), Ok("./..".to_string()));
+        result_eq(Path::new(".").join("/").as_posix(), Ok("/".to_string()));
+        result_eq(
+            Path::new("foo").join("bar").as_posix(),
+            Ok("foo/bar".to_string()),
+        );
+        result_eq(
+            Path::new("/foo").join("bar").as_posix(),
+            Ok("/foo/bar".to_string()),
+        );
+    }
+
+    #[test]
+    #[cfg(target_family = "windows")]
+    fn as_posix_prefix() {
+        assert_eq!(Path::new("C:").join(".."), Path::new("C:.."));
+        assert!(Path::new("C:").join("..").as_posix().is_err());
+    }
 
     #[test]
     #[cfg(target_family = "windows")]

src/lib.rs

@@ -46,6 +46,7 @@ use self::rustc::{TargetList, VersionMetaExt};
 
 pub use self::errors::{install_panic_hook, Result};
 pub use self::extensions::{CommandExt, OutputExt};
+pub use self::file::ToUtf8;
 
 pub const CROSS_LABEL_DOMAIN: &str = "org.cross-rs";
 
@@ -590,11 +591,11 @@ fn toml(metadata: &CargoMetadata) -> Result<Option<CrossToml>> {
     };
 
     if path.exists() {
-        let content = file::read(&path)
-            .wrap_err_with(|| format!("could not read file `{}`", path.display()))?;
+        let content =
+            file::read(&path).wrap_err_with(|| format!("could not read file `{path:?}`"))?;
 
         let (config, _) = CrossToml::parse(&content)
-            .wrap_err_with(|| format!("failed to parse file `{}` as TOML", path.display()))?;
+            .wrap_err_with(|| format!("failed to parse file `{path:?}` as TOML"))?;
 
         Ok(Some(config))
     } else {

src/rustup.rs

@@ -126,8 +126,8 @@ fn rustc_channel(version: &Version) -> Result<Channel> {
 pub fn rustc_version(toolchain_path: &Path) -> Result<Option<(Version, Channel, String)>> {
     let path = toolchain_path.join("lib/rustlib/multirust-channel-manifest.toml");
     if path.exists() {
-        let contents = std::fs::read(&path)
-            .wrap_err_with(|| format!("couldn't open file `{}`", path.display()))?;
+        let contents =
+            std::fs::read(&path).wrap_err_with(|| format!("couldn't open file `{path:?}`"))?;
         let manifest: toml::value::Table = toml::from_slice(&contents)?;
         if let Some(rust_version) = manifest
             .get("pkg")
@@ -137,9 +137,8 @@ pub fn rustc_version(toolchain_path: &Path) -> Result<Option<(Version, Channel,
         {
             // Field is `"{version} ({commit} {date})"`
             if let Some((version, meta)) = rust_version.split_once(' ') {
-                let version = Version::parse(version).wrap_err_with(|| {
-                    format!("invalid rust version found in {}", path.display())
-                })?;
+                let version = Version::parse(version)
+                    .wrap_err_with(|| format!("invalid rust version found in {path:?}"))?;
                 let channel = rustc_channel(&version)?;
                 return Ok(Some((version, channel, meta.to_owned())));
             }

src/tests/toml.rs

@@ -30,15 +30,15 @@ fn toml_check() -> Result<(), Box<dyn std::error::Error>> {
         if dir_entry.file_type().is_dir() {
             continue;
         }
-        eprintln!("File: {:?}", dir_entry.path().display());
+        eprintln!("File: {:?}", dir_entry.path());
         let mut file = std::fs::File::open(dir_entry.path()).unwrap();
         let mut contents = String::new();
         file.read_to_string(&mut contents).unwrap();
         for matches in TOML_REGEX.captures_iter(&contents) {
             let fence = matches.get(1).unwrap();
             eprintln!(
-                "testing snippet at: {}:{:?}",
-                dir_entry.path().display(),
+                "testing snippet at: {:?}:{:?}",
+                dir_entry.path(),
                 text_line_no(&contents, fence.range().start),
             );
             assert!(crate::cross_toml::CrossToml::parse(fence.as_str())?

xtask/src/build_docker_image.rs

@@ -2,7 +2,7 @@ use std::{path::Path, process::Command};
 
 use clap::Args;
 use color_eyre::Section;
-use cross::CommandExt;
+use cross::{CommandExt, ToUtf8};
 use std::fmt::Write;
 
 #[derive(Args, Debug)]
@@ -72,7 +72,7 @@ fn locate_dockerfile(
     } else {
         eyre::bail!("unable to find dockerfile for target \"{target}\"");
     };
-    let dockerfile = dockerfile_root.join(dockerfile_name).display().to_string();
+    let dockerfile = dockerfile_root.join(dockerfile_name).to_utf8()?.to_string();
     Ok((target, dockerfile))
 }