Remove UNC prefixes on Windows (#1086)

## Summary

This PR adds a `NormalizedDisplay` trait that we can use for user-facing
paths, to strip the UNC prefix on Windows.

On other platforms, the implementation is a no-op (vs. `Display`).

I audited all usages of `.display()`, and changed any that were
user-facing, either via `println!` or `eprintln!`, or by way of being
included in error messages. I did _not_ change uses that were only in
tests or only went to tracing.

Closes https://github.com/astral-sh/puffin/issues/1084.
This commit is contained in:
Charlie Marsh 2024-01-25 08:44:22 -08:00 committed by GitHub
parent 035cd81ac8
commit f4939e50a6
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
23 changed files with 98 additions and 43 deletions

6
Cargo.lock generated
View File

@ -821,6 +821,7 @@ dependencies = [
"pep440_rs 0.3.12",
"pep508_rs",
"platform-tags",
"puffin-fs",
"puffin-git",
"puffin-normalize",
"pypi-types",
@ -1476,6 +1477,7 @@ dependencies = [
"platform-host",
"platform-info",
"plist",
"puffin-fs",
"puffin-normalize",
"pyo3",
"pypi-types",
@ -2283,6 +2285,7 @@ dependencies = [
"clap",
"distribution-filename",
"distribution-types",
"dunce",
"filetime",
"flate2",
"fs-err",
@ -2308,6 +2311,7 @@ dependencies = [
"puffin-client",
"puffin-dispatch",
"puffin-distribution",
"puffin-fs",
"puffin-installer",
"puffin-interpreter",
"puffin-normalize",
@ -2576,6 +2580,7 @@ dependencies = [
"glob",
"hex",
"once_cell",
"puffin-fs",
"rand",
"reqwest",
"serde",
@ -3021,6 +3026,7 @@ dependencies = [
"once_cell",
"pep440_rs 0.3.12",
"pep508_rs",
"puffin-fs",
"puffin-normalize",
"regex",
"serde",

View File

@ -18,6 +18,7 @@ distribution-filename = { path = "../distribution-filename", features = ["serde"
pep440_rs = { path = "../pep440-rs" }
pep508_rs = { path = "../pep508-rs" }
platform-tags = { path = "../platform-tags" }
puffin-fs = { path = "../puffin-fs" }
puffin-git = { path = "../puffin-git", features = ["vendored-openssl"] }
puffin-normalize = { path = "../puffin-normalize" }
pypi-types = { path = "../pypi-types" }

View File

@ -6,6 +6,7 @@ use fs_err as fs;
use url::Url;
use pep440_rs::Version;
use puffin_fs::NormalizedDisplay;
use puffin_normalize::PackageName;
use crate::{InstalledMetadata, InstalledVersion, Name};
@ -102,8 +103,12 @@ impl InstalledDist {
pub fn metadata(&self) -> Result<pypi_types::Metadata21> {
let path = self.path().join("METADATA");
let contents = fs::read(&path)?;
pypi_types::Metadata21::parse(&contents)
.with_context(|| format!("Failed to parse METADATA file at: {}", path.display()))
pypi_types::Metadata21::parse(&contents).with_context(|| {
format!(
"Failed to parse METADATA file at: {}",
path.normalized_display()
)
})
}
/// Return the [`Url`] of the distribution, if it is editable.

View File

@ -24,6 +24,7 @@ distribution-filename = { path = "../distribution-filename" }
pep440_rs = { path = "../pep440-rs" }
platform-host = { path = "../platform-host" }
puffin-normalize = { path = "../puffin-normalize" }
puffin-fs = { path = "../puffin-fs" }
pypi-types = { path = "../pypi-types" }
clap = { workspace = true, optional = true, features = ["derive", "env"] }

View File

@ -5,6 +5,8 @@ use fs2::FileExt;
use fs_err::File;
use tracing::{error, warn};
use puffin_fs::NormalizedDisplay;
const INSTALL_LOCKFILE: &str = "install-wheel-rs.lock";
/// I'm not sure that's the right way to normalize here, but it's a single place to change
@ -55,7 +57,7 @@ impl Drop for LockedDir {
if let Err(err) = self.lockfile.file().unlock() {
error!(
"Failed to unlock {}: {}",
self.lockfile.path().display(),
self.lockfile.path().normalized_display(),
err
);
}

View File

@ -22,6 +22,7 @@ use zip::{ZipArchive, ZipWriter};
use distribution_filename::WheelFilename;
use pep440_rs::Version;
use puffin_fs::NormalizedDisplay;
use puffin_normalize::PackageName;
use pypi_types::DirectUrl;
@ -236,7 +237,7 @@ fn unpack_wheel_files<R: Read + Seek>(
.ok_or_else(|| {
Error::RecordFile(format!(
"Missing hash for {} (expected {})",
relative.display(),
relative.normalized_display(),
encoded_hash
))
})?;
@ -244,7 +245,7 @@ fn unpack_wheel_files<R: Read + Seek>(
if relative.as_os_str().to_string_lossy().starts_with("torch-") {
error!(
"Hash mismatch for {}. Recorded: {}, Actual: {}",
relative.display(),
relative.normalized_display(),
recorded_hash,
encoded_hash,
);
@ -256,7 +257,7 @@ fn unpack_wheel_files<R: Read + Seek>(
}
return Err(Error::RecordFile(format!(
"Hash mismatch for {}. Recorded: {}, Actual: {}",
relative.display(),
relative.normalized_display(),
recorded_hash,
encoded_hash,
)));
@ -499,7 +500,7 @@ fn bytecode_compile(
io::ErrorKind::NotFound,
format!(
"Didn't find pyc generated by compileall: {}",
site_packages.join(&pyc_path).display()
site_packages.join(&pyc_path).normalized_display()
),
)));
}
@ -593,9 +594,9 @@ pub fn relative_to(path: &Path, base: &Path) -> Result<PathBuf, Error> {
Error::Io(io::Error::new(
io::ErrorKind::Other,
format!(
"trivial strip case should have worked: {} vs {}",
path.display(),
base.display()
"Trivial strip failed: {} vs. {}",
path.normalized_display(),
base.normalized_display()
),
))
})?;
@ -717,8 +718,8 @@ fn install_script(
// This should be possible to occur at this point, but filesystems and such
Error::RecordFile(format!(
"Could not find entry for {} ({})",
relative_to_site_packages.display(),
path.display()
relative_to_site_packages.normalized_display(),
path.normalized_display()
))
})?;
entry.path = target_path.display().to_string();

View File

@ -8,6 +8,10 @@ use tracing::{error, warn};
use puffin_warnings::warn_user;
pub use crate::path::*;
mod path;
/// Create a symlink from `src` to `dst`, replacing any existing symlink.
///
/// On Windows, this uses the `junction` crate to create a junction point.
@ -59,7 +63,7 @@ pub async fn write_atomic(path: impl AsRef<Path>, data: impl AsRef<[u8]>) -> std
std::io::ErrorKind::Other,
format!(
"Failed to persist temporary file to {}: {}",
path.as_ref().display(),
path.normalized_display(),
err.error
),
)
@ -80,7 +84,7 @@ pub fn write_atomic_sync(path: impl AsRef<Path>, data: impl AsRef<[u8]>) -> std:
std::io::ErrorKind::Other,
format!(
"Failed to persist temporary file to {}: {}",
path.as_ref().display(),
path.normalized_display(),
err.error
),
)
@ -192,7 +196,7 @@ impl LockedFile {
warn_user!(
"Waiting to acquire lock for {} (lockfile: {})",
resource,
path.as_ref().display()
path.normalized_display(),
);
file.file().lock_exclusive()?;
Ok(Self(file))

View File

@ -0,0 +1,15 @@
use std::path::Path;
pub trait NormalizedDisplay {
/// Render a [`Path`] for user-facing display.
///
/// On Windows, this will strip the `\\?\` prefix from paths. On other platforms, it's
/// equivalent to [`std::path::Display`].
fn normalized_display(&self) -> std::path::Display;
}
impl<T: AsRef<Path>> NormalizedDisplay for T {
fn normalized_display(&self) -> std::path::Display {
dunce::simplified(self.as_ref()).display()
}
}

View File

@ -14,6 +14,7 @@ workspace = true
[dependencies]
cache-key = { path = "../cache-key" }
puffin-fs = { path = "../puffin-fs" }
anyhow = { workspace = true }
cargo-util = { workspace = true }

View File

@ -9,6 +9,7 @@ use std::{env, str};
use anyhow::{anyhow, Context as _, Result};
use cargo_util::{paths, ProcessBuilder};
use git2::{self, ErrorClass, ObjectType};
use puffin_fs::NormalizedDisplay;
use reqwest::Client;
use reqwest::StatusCode;
use tracing::{debug, warn};
@ -149,7 +150,7 @@ impl GitRemote {
let reference = locked_ref.as_ref().unwrap_or(reference);
if let Some(mut db) = db {
fetch(&mut db.repo, self.url.as_str(), reference, strategy, client)
.with_context(|| format!("failed to fetch into: {}", into.display()))?;
.with_context(|| format!("failed to fetch into: {}", into.normalized_display()))?;
let resolved_commit_hash = match locked_rev {
Some(rev) => db.contains(rev).then_some(rev),
@ -169,7 +170,7 @@ impl GitRemote {
paths::create_dir_all(into)?;
let mut repo = init(into, true)?;
fetch(&mut repo, self.url.as_str(), reference, strategy, client)
.with_context(|| format!("failed to clone into: {}", into.display()))?;
.with_context(|| format!("failed to clone into: {}", into.normalized_display()))?;
let rev = match locked_rev {
Some(rev) => rev,
None => reference.resolve(&repo)?,

View File

@ -5,7 +5,7 @@ use tracing::debug;
use platform_host::Platform;
use puffin_cache::Cache;
use puffin_fs::LockedFile;
use puffin_fs::{LockedFile, NormalizedDisplay};
use crate::cfg::Configuration;
use crate::python_platform::PythonPlatform;
@ -88,7 +88,7 @@ impl Virtualenv {
/// Lock the virtual environment to prevent concurrent writes.
pub fn lock(&self) -> Result<LockedFile, std::io::Error> {
LockedFile::acquire(self.root.join(".lock"), self.root.display())
LockedFile::acquire(self.root.join(".lock"), self.root.normalized_display())
}
}

View File

@ -27,6 +27,7 @@ puffin-cache = { path = "../puffin-cache", features = ["clap"] }
puffin-client = { path = "../puffin-client" }
puffin-dispatch = { path = "../puffin-dispatch" }
puffin-distribution = { path = "../puffin-distribution" }
puffin-fs = { path = "../puffin-fs" }
puffin-installer = { path = "../puffin-installer" }
puffin-interpreter = { path = "../puffin-interpreter" }
puffin-normalize = { path = "../puffin-normalize" }
@ -67,6 +68,7 @@ tracing-tree = { workspace = true }
url = { workspace = true }
waitmap = { workspace = true }
which = { workspace = true }
dunce = "1.0.4"
[target.'cfg(target_os = "windows")'.dependencies]
mimalloc = "0.1.39"

View File

@ -5,6 +5,7 @@ use fs_err as fs;
use owo_colors::OwoColorize;
use puffin_cache::Cache;
use puffin_fs::NormalizedDisplay;
use puffin_normalize::PackageName;
use crate::commands::ExitStatus;
@ -20,7 +21,7 @@ pub(crate) fn clean(
writeln!(
printer,
"No cache found at: {}",
cache.root().display().cyan()
cache.root().normalized_display().cyan()
)?;
return Ok(ExitStatus::Success);
}
@ -29,10 +30,14 @@ pub(crate) fn clean(
writeln!(
printer,
"Clearing cache at: {}",
cache.root().display().cyan()
cache.root().normalized_display().cyan()
)?;
fs::remove_dir_all(cache.root())
.with_context(|| format!("Failed to clear cache at: {}", cache.root().display()))?;
fs::remove_dir_all(cache.root()).with_context(|| {
format!(
"Failed to clear cache at: {}",
cache.root().normalized_display()
)
})?;
} else {
for package in packages {
let count = cache.purge(package)?;

View File

@ -8,6 +8,7 @@ use tracing::debug;
use distribution_types::Name;
use platform_host::Platform;
use puffin_cache::Cache;
use puffin_fs::NormalizedDisplay;
use puffin_installer::SitePackages;
use puffin_interpreter::Virtualenv;
@ -23,7 +24,7 @@ pub(crate) fn freeze(cache: &Cache, strict: bool, mut printer: Printer) -> Resul
debug!(
"Using Python {} environment at {}",
venv.interpreter().python_version(),
venv.python_executable().display().cyan()
venv.python_executable().normalized_display().cyan()
);
// Build the installed index.

View File

@ -11,7 +11,6 @@ use anyhow::{anyhow, Context, Result};
use chrono::{DateTime, Utc};
use itertools::Itertools;
use owo_colors::OwoColorize;
use puffin_warnings::warn_user;
use rustc_hash::FxHashSet;
use tempfile::tempdir_in;
use tracing::debug;
@ -23,6 +22,7 @@ use platform_tags::Tags;
use puffin_cache::Cache;
use puffin_client::{FlatIndex, FlatIndexClient, RegistryClientBuilder};
use puffin_dispatch::BuildDispatch;
use puffin_fs::NormalizedDisplay;
use puffin_installer::{Downloader, NoBinary};
use puffin_interpreter::{Interpreter, PythonVersion};
use puffin_normalize::{ExtraName, PackageName};
@ -31,6 +31,7 @@ use puffin_resolver::{
ResolutionOptions, Resolver,
};
use puffin_traits::{InFlight, SetupPyStrategy};
use puffin_warnings::warn_user;
use requirements_txt::EditableRequirement;
use crate::commands::reporters::{DownloadReporter, ResolverReporter};
@ -129,7 +130,7 @@ pub(crate) async fn pip_compile(
debug!(
"Using Python {} interpreter at {} for builds",
interpreter.python_version(),
interpreter.sys_executable().display().cyan()
interpreter.sys_executable().normalized_display().cyan()
);
if let Some(python_version) = python_version.as_ref() {
// If the requested version does not match the version we're using warn the user

View File

@ -19,6 +19,7 @@ use platform_tags::Tags;
use puffin_cache::Cache;
use puffin_client::{FlatIndex, FlatIndexClient, RegistryClient, RegistryClientBuilder};
use puffin_dispatch::BuildDispatch;
use puffin_fs::NormalizedDisplay;
use puffin_installer::{
BuiltEditable, Downloader, NoBinary, Plan, Planner, Reinstall, ResolvedEditable, SitePackages,
};
@ -91,7 +92,7 @@ pub(crate) async fn pip_install(
debug!(
"Using Python {} environment at {}",
venv.interpreter().python_version(),
venv.python_executable().display().cyan()
venv.python_executable().normalized_display().cyan()
);
let _lock = venv.lock()?;

View File

@ -12,6 +12,7 @@ use platform_tags::Tags;
use puffin_cache::Cache;
use puffin_client::{FlatIndex, FlatIndexClient, RegistryClient, RegistryClientBuilder};
use puffin_dispatch::BuildDispatch;
use puffin_fs::NormalizedDisplay;
use puffin_installer::{
Downloader, NoBinary, Plan, Planner, Reinstall, ResolvedEditable, SitePackages,
};
@ -56,7 +57,7 @@ pub(crate) async fn pip_sync(
debug!(
"Using Python {} environment at {}",
venv.interpreter().python_version(),
venv.python_executable().display().cyan()
venv.python_executable().normalized_display().cyan()
);
let _lock = venv.lock()?;

View File

@ -7,6 +7,7 @@ use tracing::debug;
use distribution_types::{InstalledMetadata, Name};
use platform_host::Platform;
use puffin_cache::Cache;
use puffin_fs::NormalizedDisplay;
use puffin_interpreter::Virtualenv;
use crate::commands::{elapsed, ExitStatus};
@ -30,7 +31,7 @@ pub(crate) async fn pip_uninstall(
debug!(
"Using Python {} environment at {}",
venv.interpreter().python_version(),
venv.python_executable().display().cyan()
venv.python_executable().normalized_display().cyan(),
);
let _lock = venv.lock()?;

View File

@ -13,6 +13,7 @@ use platform_host::Platform;
use puffin_cache::Cache;
use puffin_client::{FlatIndex, FlatIndexClient, RegistryClientBuilder};
use puffin_dispatch::BuildDispatch;
use puffin_fs::NormalizedDisplay;
use puffin_installer::NoBinary;
use puffin_interpreter::{find_default_python, find_requested_python, Interpreter};
use puffin_resolver::InMemoryIndex;
@ -90,14 +91,14 @@ async fn venv_impl(
printer,
"Using Python {} interpreter at {}",
interpreter.python_version(),
interpreter.sys_executable().display().cyan()
interpreter.sys_executable().normalized_display().cyan()
)
.into_diagnostic()?;
writeln!(
printer,
"Creating virtual environment at: {}",
path.display().cyan()
path.normalized_display().cyan()
)
.into_diagnostic()?;

View File

@ -8,6 +8,7 @@ use fs_err as fs;
use rustc_hash::FxHashSet;
use pep508_rs::Requirement;
use puffin_fs::NormalizedDisplay;
use puffin_normalize::{ExtraName, PackageName};
use requirements_txt::{EditableRequirement, RequirementsTxt};
@ -117,7 +118,7 @@ impl RequirementsSpecification {
RequirementsSource::PyprojectToml(path) => {
let contents = fs::read_to_string(path)?;
let pyproject_toml = toml::from_str::<pyproject_toml::PyProjectToml>(&contents)
.with_context(|| format!("Failed to parse `{}`", path.display()))?;
.with_context(|| format!("Failed to parse `{}`", path.normalized_display()))?;
let mut used_extras = FxHashSet::default();
let mut requirements = Vec::new();
let mut project_name = None;
@ -139,7 +140,7 @@ impl RequirementsSpecification {
}
// Parse the project name
project_name = Some(PackageName::new(project.name).with_context(|| {
format!("Invalid `project.name` in {}", path.display())
format!("Invalid `project.name` in {}", path.normalized_display())
})?);
}

View File

@ -8,6 +8,7 @@ use insta_cmd::{assert_cmd_snapshot, get_cargo_bin};
use url::Url;
use common::{BIN_NAME, INSTA_FILTERS};
use puffin_fs::NormalizedDisplay;
use crate::common::{create_venv_py312, venv_to_interpreter};
@ -345,7 +346,7 @@ fn missing_record() -> Result<()> {
let dist_info_str = regex::escape(&format!(
"RECORD file not found at: {}",
dist_info.display()
dist_info.normalized_display()
));
let filters: Vec<_> = iter::once((
dist_info_str.as_str(),

View File

@ -15,6 +15,7 @@ workspace = true
[dependencies]
pep440_rs = { path = "../pep440-rs", features = ["serde"] }
pep508_rs = { path = "../pep508-rs", features = ["serde"] }
puffin-fs = { path = "../puffin-fs" }
puffin-normalize = { path = "../puffin-normalize" }
fs-err = { workspace = true }

View File

@ -46,6 +46,7 @@ use unscanny::{Pattern, Scanner};
use url::Url;
use pep508_rs::{split_scheme, Pep508Error, Pep508ErrorSource, Requirement, VerbatimUrl};
use puffin_fs::NormalizedDisplay;
/// We emit one of those for each requirements.txt entry
enum RequirementsTxtStatement {
@ -562,28 +563,28 @@ impl Display for RequirementsTxtFileError {
write!(
f,
"Invalid editable path in `{}`: {given}",
self.file.display()
self.file.normalized_display()
)
}
RequirementsTxtParserError::UnsupportedUrl(url) => {
write!(
f,
"Unsupported URL (expected a `file://` scheme) in `{}`: {url}",
self.file.display(),
self.file.normalized_display(),
)
}
RequirementsTxtParserError::Parser { message, location } => {
write!(
f,
"{message} in `{}` at position {location}",
self.file.display(),
self.file.normalized_display(),
)
}
RequirementsTxtParserError::UnsupportedRequirement { start, end, .. } => {
write!(
f,
"Unsupported requirement in {} position {} to {}",
self.file.display(),
self.file.normalized_display(),
start,
end,
)
@ -592,14 +593,14 @@ impl Display for RequirementsTxtFileError {
write!(
f,
"Couldn't parse requirement in `{}` at position {start}",
self.file.display(),
self.file.normalized_display(),
)
}
RequirementsTxtParserError::Subfile { start, .. } => {
write!(
f,
"Error parsing included file in `{}` at position {start}",
self.file.display(),
self.file.normalized_display(),
)
}
}
@ -624,6 +625,7 @@ mod test {
use fs_err as fs;
use indoc::indoc;
use puffin_fs::NormalizedDisplay;
use tempfile::tempdir;
use test_case::test_case;
@ -699,12 +701,12 @@ mod test {
errors[0],
format!(
"Error parsing included file in `{}` at position 0",
basic.display()
basic.normalized_display()
)
);
assert_eq!(
errors[1],
format!("failed to open file `{}`", missing.display()),
format!("failed to open file `{}`", missing.normalized_display()),
);
// The last error message is os specific
}
@ -721,7 +723,7 @@ mod test {
let expected = &[
format!(
"Couldn't parse requirement in `{}` at position 0",
basic.display()
basic.normalized_display()
),
indoc! {"
Expected an alphanumeric character starting the extra name, found 'ö'