Skip to content

Commit

Permalink
Fix relative and absolute path handling in lockfiles
Browse files Browse the repository at this point in the history
Previously, `b` would have been incorrectly locked to `a`.
  • Loading branch information
konstin authored and BurntSushi committed Jun 13, 2024
1 parent 5d1305a commit b5d85f9
Show file tree
Hide file tree
Showing 4 changed files with 214 additions and 88 deletions.
66 changes: 3 additions & 63 deletions crates/install-wheel-rs/src/wheel.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::collections::HashMap;
use std::io::{BufRead, BufReader, Cursor, Read, Write};
use std::path::{Path, PathBuf};
use std::{env, io, iter};
use std::{env, io};

use data_encoding::BASE64URL_NOPAD;
use fs_err as fs;
Expand All @@ -15,7 +15,7 @@ use zip::write::FileOptions;
use zip::ZipWriter;

use pypi_types::DirectUrl;
use uv_fs::Simplified;
use uv_fs::{relative_to, Simplified};

use crate::record::RecordEntry;
use crate::script::Script;
Expand Down Expand Up @@ -368,38 +368,6 @@ pub(crate) fn parse_wheel_file(wheel_text: &str) -> Result<LibKind, Error> {
Ok(lib_kind)
}

/// Give the path relative to the base directory
///
/// lib/python/site-packages/foo/__init__.py and lib/python/site-packages -> foo/__init__.py
/// lib/marker.txt and lib/python/site-packages -> ../../marker.txt
/// `bin/foo_launcher` and lib/python/site-packages -> ../../../`bin/foo_launcher`
pub(crate) fn relative_to(path: &Path, base: &Path) -> Result<PathBuf, Error> {
// Find the longest common prefix, and also return the path stripped from that prefix
let (stripped, common_prefix) = base
.ancestors()
.find_map(|ancestor| {
path.strip_prefix(ancestor)
.ok()
.map(|stripped| (stripped, ancestor))
})
.ok_or_else(|| {
Error::Io(io::Error::new(
io::ErrorKind::Other,
format!(
"Trivial strip failed: {} vs. {}",
path.simplified_display(),
base.simplified_display()
),
))
})?;

// go as many levels up as required
let levels_up = base.components().count() - common_prefix.components().count();
let up = iter::repeat("..").take(levels_up).collect::<PathBuf>();

Ok(up.join(stripped))
}

/// Moves the files and folders in src to dest, updating the RECORD in the process
pub(crate) fn move_folder_recorded(
src_dir: &Path,
Expand Down Expand Up @@ -759,7 +727,7 @@ mod test {
use crate::wheel::format_shebang;
use crate::Error;

use super::{parse_key_value_file, parse_wheel_file, read_record_file, relative_to, Script};
use super::{parse_key_value_file, parse_wheel_file, read_record_file, Script};

#[test]
fn test_parse_key_value_file() {
Expand Down Expand Up @@ -816,34 +784,6 @@ mod test {
assert_eq!(expected, actual);
}

#[test]
fn test_relative_to() {
assert_eq!(
relative_to(
Path::new("/home/ferris/carcinization/lib/python/site-packages/foo/__init__.py"),
Path::new("/home/ferris/carcinization/lib/python/site-packages"),
)
.unwrap(),
Path::new("foo/__init__.py")
);
assert_eq!(
relative_to(
Path::new("/home/ferris/carcinization/lib/marker.txt"),
Path::new("/home/ferris/carcinization/lib/python/site-packages"),
)
.unwrap(),
Path::new("../../marker.txt")
);
assert_eq!(
relative_to(
Path::new("/home/ferris/carcinization/bin/foo_launcher"),
Path::new("/home/ferris/carcinization/lib/python/site-packages"),
)
.unwrap(),
Path::new("../../../bin/foo_launcher")
);
}

#[test]
fn test_script_from_value() {
assert_eq!(
Expand Down
62 changes: 37 additions & 25 deletions crates/uv-distribution/src/metadata/lowering.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use pep440_rs::VersionSpecifiers;
use pep508_rs::{VerbatimUrl, VersionOrUrl};
use pypi_types::{Requirement, RequirementSource, VerbatimParsedUrl};
use uv_configuration::PreviewMode;
use uv_fs::Simplified;
use uv_fs::{relative_to, Simplified};
use uv_git::GitReference;
use uv_normalize::PackageName;
use uv_warnings::warn_user_once;
Expand Down Expand Up @@ -44,6 +44,8 @@ pub enum LoweringError {
MissingPreview,
#[error("Editable must refer to a local directory, not a file: `{0}`")]
EditableFile(String),
#[error(transparent)] // Function attaches the context
RelativeTo(io::Error),
}

/// Combine `project.dependencies` or `project.optional-dependencies` with `tool.uv.sources`.
Expand Down Expand Up @@ -234,25 +236,30 @@ fn path_source(
workspace_root: &Path,
editable: bool,
) -> Result<RequirementSource, LoweringError> {
let url = VerbatimUrl::parse_path(path.as_ref(), project_dir)?
.with_given(path.as_ref().to_string_lossy());
let path_buf = path.as_ref().to_path_buf();
let path_buf = path_buf
let path = path.as_ref();
let url = VerbatimUrl::parse_path(path, project_dir)?.with_given(path.to_string_lossy());
let absolute_path = path
.to_path_buf()
.absolutize_from(project_dir)
.map_err(|err| LoweringError::Absolutize(path.as_ref().to_path_buf(), err))?
.map_err(|err| LoweringError::Absolutize(path.to_path_buf(), err))?
.to_path_buf();
let ascend_to_workspace = project_dir
.strip_prefix(workspace_root)
.expect("Project must be below workspace root");
let is_dir = if let Ok(metadata) = path_buf.metadata() {
let relative_to_workspace = if path.is_relative() {
// Relative paths in a project are relative to the project root, but the lockfile is
// relative to the workspace root.
relative_to(&absolute_path, workspace_root).map_err(LoweringError::RelativeTo)?
} else {
// If the user gave us an absolute path, we respect that.
path.to_path_buf()
};
let is_dir = if let Ok(metadata) = absolute_path.metadata() {
metadata.is_dir()
} else {
path_buf.extension().is_none()
absolute_path.extension().is_none()
};
if is_dir {
Ok(RequirementSource::Directory {
install_path: path_buf,
lock_path: ascend_to_workspace.join(project_dir),
install_path: absolute_path,
lock_path: relative_to_workspace,
url,
editable,
})
Expand All @@ -261,8 +268,8 @@ fn path_source(
return Err(LoweringError::EditableFile(url.to_string()));
}
Ok(RequirementSource::Path {
install_path: path_buf,
lock_path: ascend_to_workspace.join(project_dir),
install_path: absolute_path,
lock_path: relative_to_workspace,
url,
})
}
Expand All @@ -275,19 +282,24 @@ fn directory_source(
workspace_root: &Path,
editable: bool,
) -> Result<RequirementSource, LoweringError> {
let url = VerbatimUrl::parse_path(path.as_ref(), project_dir)?
.with_given(path.as_ref().to_string_lossy());
let path_buf = path.as_ref().to_path_buf();
let path_buf = path_buf
let path = path.as_ref();
let url = VerbatimUrl::parse_path(path, project_dir)?.with_given(path.to_string_lossy());
let absolute_path = path
.to_path_buf()
.absolutize_from(project_dir)
.map_err(|err| LoweringError::Absolutize(path.as_ref().to_path_buf(), err))?
.map_err(|err| LoweringError::Absolutize(path.to_path_buf(), err))?
.to_path_buf();
let ascend_to_workspace = project_dir
.strip_prefix(workspace_root)
.expect("Project must be below workspace root");
let relative_to_workspace = if path.is_relative() {
// Relative paths in a project are relative to the project root, but the lockfile is
// relative to the workspace root.
relative_to(&absolute_path, workspace_root).map_err(LoweringError::RelativeTo)?
} else {
// If the user gave us an absolute path, we respect that.
path.to_path_buf()
};
Ok(RequirementSource::Directory {
install_path: path_buf,
lock_path: ascend_to_workspace.join(project_dir),
install_path: absolute_path,
lock_path: relative_to_workspace,
url,
editable,
})
Expand Down
63 changes: 63 additions & 0 deletions crates/uv-fs/src/path.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use either::Either;
use std::borrow::Cow;
use std::path::{Component, Path, PathBuf};
use std::{io, iter};

use once_cell::sync::Lazy;
use path_slash::PathExt;
Expand Down Expand Up @@ -320,6 +321,40 @@ fn is_windows_store_python(path: &Path) -> bool {
is_windows_store_python_shim(path) || is_windows_store_python_executable(path)
}

/// Compute a path describing `path` relative to `base`.
///
/// `lib/python/site-packages/foo/__init__.py` and `lib/python/site-packages` -> `foo/__init__.py`
/// `lib/marker.txt` and `lib/python/site-packages` -> `../../marker.txt`
/// `bin/foo_launcher` and `lib/python/site-packages` -> `../../../bin/foo_launcher`
pub fn relative_to(path: impl AsRef<Path>, base: impl AsRef<Path>) -> Result<PathBuf, io::Error> {
// Find the longest common prefix, and also return the path stripped from that prefix
let (stripped, common_prefix) = base
.as_ref()
.ancestors()
.find_map(|ancestor| {
path.as_ref()
.strip_prefix(ancestor)
.ok()
.map(|stripped| (stripped, ancestor))
})
.ok_or_else(|| {
io::Error::new(
io::ErrorKind::Other,
format!(
"Trivial strip failed: {} vs. {}",
path.simplified_display(),
base.simplified_display()
),
)
})?;

// go as many levels up as required
let levels_up = base.as_ref().components().count() - common_prefix.components().count();
let up = iter::repeat("..").take(levels_up).collect::<PathBuf>();

Ok(up.join(stripped))
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down Expand Up @@ -378,4 +413,32 @@ mod tests {
let err = normalize_path(path).unwrap_err();
assert_eq!(err.kind(), std::io::ErrorKind::InvalidInput);
}

#[test]
fn test_relative_to() {
assert_eq!(
relative_to(
Path::new("/home/ferris/carcinization/lib/python/site-packages/foo/__init__.py"),
Path::new("/home/ferris/carcinization/lib/python/site-packages"),
)
.unwrap(),
Path::new("foo/__init__.py")
);
assert_eq!(
relative_to(
Path::new("/home/ferris/carcinization/lib/marker.txt"),
Path::new("/home/ferris/carcinization/lib/python/site-packages"),
)
.unwrap(),
Path::new("../../marker.txt")
);
assert_eq!(
relative_to(
Path::new("/home/ferris/carcinization/bin/foo_launcher"),
Path::new("/home/ferris/carcinization/lib/python/site-packages"),
)
.unwrap(),
Path::new("../../../bin/foo_launcher")
);
}
}
Loading

0 comments on commit b5d85f9

Please sign in to comment.