Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions crates/uv-fs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -858,10 +858,9 @@ pub fn copy_dir_all(src: impl AsRef<Path>, dst: impl AsRef<Path>) -> std::io::Re

/// Perform a safe removal of a virtual environment.
///
/// Links at `location` are removed without following them.
/// The link or file at `location` is removed without following it.
pub fn remove_virtualenv(location: &Path) -> io::Result<()> {
let file_type = fs_err::symlink_metadata(location)?.file_type();
if file_type.is_symlink() {
if !fs_err::symlink_metadata(location)?.is_dir() {
return remove_symlink(location);
}

Expand Down
162 changes: 114 additions & 48 deletions crates/uv/src/commands/project/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::collections::{BTreeMap, BTreeSet};
use std::fmt::Write;
use std::io;
use std::path::{Path, PathBuf};
use std::sync::Arc;

Expand Down Expand Up @@ -1120,30 +1121,58 @@ pub(crate) fn centralized_environments_enabled(
true
}

/// Return whether `path` is a link into the current cache's environment bucket.
pub(crate) fn is_centralized_environment_link(path: &Path, cache: &Cache) -> bool {
let Ok(target) = fs_err::read_link(path) else {
return false;
};
/// Return whether `path` is lexically within `base`.
fn is_path_lexically_within(path: &Path, base: &Path) -> bool {
// Normally only longer paths must be in the verbatim namespace, normalise both so the
// comparison works correctly regardless.
verbatim_path(path).starts_with(verbatim_path(base).as_ref())
}
Comment thread
EliteTK marked this conversation as resolved.

/// Return whether `path` looks like a path we wrote and references our environment cache.
///
/// This isn't fully robust, and cannot be, as the path may not exist.
fn is_centralized_environment_path(path: &Path, cache: &Cache) -> bool {
let Ok(environments) = std::path::absolute(cache.bucket(CacheBucket::Environments)) else {
// If we can't resolve the cache directory, the environment can't be in the cache.
return false;
};
// Compare Windows paths in the verbatim namespace so long targets returned with `\\?\` match
// the cache root.
let starts_with =
|path: &Path, base: &Path| verbatim_path(path).starts_with(verbatim_path(base).as_ref());
if starts_with(&target, &environments) {
if is_path_lexically_within(path, &environments) {
return true;
}

// Resolve existing relative or indirect links; only lexical targets can be dangling.
fs_err::canonicalize(path).is_ok_and(|target| {
// Resolve existing relative or indirect paths; only the lexical check can handle dangling
// paths.
fs_err::canonicalize(path).is_ok_and(|path| {
fs_err::canonicalize(&environments)
.is_ok_and(|environments| starts_with(&target, &environments))
.is_ok_and(|environments| is_path_lexically_within(&path, &environments))
})
}

/// Return whether `path` appears to link into the current cache's environment bucket.
fn is_centralized_environment_link(path: &Path, cache: &Cache) -> bool {
let Ok(target) = fs_err::read_link(path) else {
return false;
};
is_centralized_environment_path(&target, cache) || is_centralized_environment_path(path, cache)
}

/// Read an environment path from a file.
fn read_environment_path_file(path: &Path) -> io::Result<PathBuf> {
let target = PathBuf::from(fs_err::read_to_string(path)?);
Ok(if target.is_absolute() {
target
} else {
path.parent().unwrap_or(Path::new("")).join(target)
})
}

/// Return whether `path` refers to an environment in the current cache's environment bucket.
pub(crate) fn is_centralized_environment_reference(path: &Path, cache: &Cache) -> bool {
is_centralized_environment_link(path, cache)
|| read_environment_path_file(path)
.ok()
.is_some_and(|target| is_centralized_environment_path(&target, cache))
}

/// Return the centralized environment path for a given workspace and interpreter.
pub(crate) fn centralized_environment_root(
workspace: &Workspace,
Expand Down Expand Up @@ -1217,38 +1246,60 @@ pub(crate) fn update_project_environment_link(
link_error_reporting: LinkErrorReporting,
) -> bool {
let link = workspace.install_path().join(".venv");
let report_error = |message: &str, err: &std::io::Error| match link_error_reporting {
LinkErrorReporting::User => {
warn_user_once!("{message} at `{}`: {err}", link.user_display());
}
LinkErrorReporting::Log => warn!("{message} at `{}`: {err}", link.user_display()),
let report_error = |message: std::fmt::Arguments<'_>| match link_error_reporting {
LinkErrorReporting::User => warn_user_once!("{message}"),
LinkErrorReporting::Log => warn!("{message}"),
};

if fs_err::symlink_metadata(&link).is_ok_and(|metadata| metadata.is_dir()) {
if uv_fs::is_virtualenv_base(&link) {
if let Err(err) = uv_fs::remove_virtualenv(&link) {
report_error("Failed to remove existing local virtual environment", &err);
report_error(format_args!(
"Failed to remove existing local virtual environment: {err}"
));
return false;
}
} else {
// On Windows, copying a junction can produce an empty directory.
#[cfg(windows)]
if let Err(err) = fs_err::remove_dir(&link) {
report_error("Failed to create link to project environment", &err);
report_error(format_args!(
"Failed to create link to project environment: {err}"
));
return false;
}
}
}

// TODO(tk): When directory links are unavailable, write `.venv` as a file containing the
// environment path.
match uv_fs::replace_symlink(environment.root(), &link) {
Ok(()) => true,
Err(err) => {
report_error("Failed to create link to project environment", &err);
false
}
// On Windows replace_symlink won't replace a file, but we want to try to upgrade to a junction
// if possible.
if cfg!(windows) {
let _ = fs_err::remove_file(&link);
}

let Err(link_error) = uv_fs::replace_symlink(environment.root(), &link) else {
Comment thread
EliteTK marked this conversation as resolved.
return true;
};
warn!("Failed to create link to project environment: {link_error}");

let Some(target) = environment.root().to_str() else {
report_error(format_args!(
"Failed to write the environment path to `{}`: the path is not valid UTF-8",
link.simplified_display()
));
return false;
};

if let Err(err) = uv_fs::write_atomic_sync(&link, target.as_bytes()) {
report_error(format_args!("Failed to write the environment path: {err}"));
return false;
}

report_error(format_args!(
"Failed to create link to project environment; wrote the environment path to `{}` instead",
link.simplified_display()
));
false
}

/// An interpreter suitable for the project.
Expand Down Expand Up @@ -1292,7 +1343,13 @@ impl ProjectInterpreter {
// the cache root instead of trusting the link target.
if centralized {
let project_environment_path = workspace.install_path().join(".venv");
if let Ok(candidate) = PythonEnvironment::from_root(&project_environment_path, cache) {
if let Ok(candidate) = PythonEnvironment::from_root(
read_environment_path_file(&project_environment_path)
.ok()
.as_deref()
.unwrap_or(&project_environment_path),
cache,
) {
let root = centralized_environment_root(
workspace,
candidate.interpreter(),
Expand All @@ -1311,18 +1368,28 @@ impl ProjectInterpreter {
return Ok(Self::Environment(environment));
}
}
} else if let Some(environment) = discover_project_environment(
&environment_selection
} else {
let project_environment_path = environment_selection
.explicit_path()
.map_or_else(|| workspace.install_path().join(".venv"), Path::to_path_buf),
python_request.as_ref(),
python_preference,
requires_python.as_ref(),
keep_incompatible,
centralized,
cache,
)? {
return Ok(Self::Environment(environment));
.map_or_else(|| workspace.install_path().join(".venv"), Path::to_path_buf);
// TODO(tk): Revisit after PEP 832.
// A centralized path file is not a local environment; let initialization replace it.
if !(environment_selection.is_default()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also allow non-default values that should resolve to the same behavior (e.g. UV_PROJECT_ENVIRONMENT=.venv)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

If someone is specifying UV_PROJECT_ENVIRONMENT= ... it would be weird if we didn't follow their instructions to the letter. Feels like a footgun otherwise?

&& read_environment_path_file(&project_environment_path)
.ok()
.is_some_and(|target| is_centralized_environment_path(&target, cache)))
&& let Some(environment) = discover_project_environment(
&project_environment_path,
python_request.as_ref(),
python_preference,
requires_python.as_ref(),
keep_incompatible,
centralized,
cache,
)?
{
return Ok(Self::Environment(environment));
}
}

let reporter = PythonDownloadReporter::single(printer);
Expand Down Expand Up @@ -1741,12 +1808,12 @@ impl ProjectEnvironment {
.explicit_path()
.map_or_else(|| workspace.install_path().join(".venv"), Path::to_path_buf)
};
let centralized_environment_link =
!centralized && is_centralized_environment_link(&root, cache);
let centralized_environment_reference =
!centralized && is_centralized_environment_reference(&root, cache);

// Avoid removing things that are not virtual environments and are outside the
// environment cache.
let replace_environment = if centralized_environment_link {
let replace_environment = if centralized_environment_reference {
true
} else {
match (root.try_exists(), root.join("pyvenv.cfg").try_exists()) {
Expand Down Expand Up @@ -1823,9 +1890,8 @@ impl ProjectEnvironment {
}

if replace_environment {
// `clear_virtualenv` follows directory links, so unlink centralized links
// directly to preserve their cached targets.
let removed = if centralized_environment_link {
// Remove centralized references directly to preserve their cached targets.
let removed = if centralized_environment_reference {
match uv_fs::remove_virtualenv(&root) {
Ok(()) => true,
Err(err) if err.kind() == std::io::ErrorKind::NotFound => false,
Expand All @@ -1835,7 +1901,7 @@ impl ProjectEnvironment {
uv_fs::clear_virtualenv(&root).map_err(uv_virtualenv::Error::from)?
};
if removed {
let removed_entry = if centralized_environment_link {
let removed_entry = if centralized_environment_reference {
"link to project environment"
} else {
"virtual environment"
Expand Down
17 changes: 13 additions & 4 deletions crates/uv/src/commands/venv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ use crate::commands::pip::loggers::{DefaultInstallLogger, InstallLogger};
use crate::commands::pip::operations::{Changelog, report_interpreter};
use crate::commands::project::{
LinkErrorReporting, WorkspacePython, centralized_environment_root,
centralized_environments_enabled, is_centralized_environment_link, lock_project_environment,
update_project_environment_link, validate_project_requires_python,
centralized_environments_enabled, is_centralized_environment_reference,
lock_project_environment, update_project_environment_link, validate_project_requires_python,
};
use crate::commands::reporters::PythonDownloadReporter;
use crate::printer::Printer;
Expand Down Expand Up @@ -252,10 +252,19 @@ pub(crate) async fn venv(
OnExisting::Remove(RemovalReason::ManagedEnvironment)
}
OnExisting::Prompt | OnExisting::Remove(_)
if is_centralized_environment_link(&path, cache) =>
if is_centralized_environment_reference(&path, cache) =>
{
// Remove `.venv` without following it into the cache.
uv_fs::remove_symlink(&path).map_err(|err| VenvError::Creation(err.into()))?;
uv_fs::remove_virtualenv(&path).map_err(|err| VenvError::Creation(err.into()))?;
on_existing
}
OnExisting::Allow
if fs_err::symlink_metadata(&path).is_ok_and(|metadata| metadata.is_file())
&& is_centralized_environment_reference(&path, cache) =>
{
// TODO(tk): Revisit after PEP 832.
// Ignore uv-owned path files when creating a local environment.
uv_fs::remove_virtualenv(&path).map_err(|err| VenvError::Creation(err.into()))?;
on_existing
}
_ => on_existing,
Expand Down
58 changes: 58 additions & 0 deletions crates/uv/tests/project/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7266,3 +7266,61 @@ fn run_centralized_environment_no_sync_uses_incompatible_python() -> Result<()>
"#);
Ok(())
}

#[test]
fn run_centralized_environment_path_file() -> Result<()> {
let context = uv_test::test_context_with_versions!(&["3.11", "3.12"])
.with_filtered_centralized_environment_hashes();
context
.temp_dir
.child("pyproject.toml")
.write_str(indoc! {r#"
[project]
name = "project"
version = "0.1.0"
requires-python = ">=3.11"
dependencies = []
"#})?;
context
.sync()
.arg("--preview-features")
.arg("centralized-project-envs")
.arg("--python")
.arg("3.12")
.assert()
.success();

// Point the path file at an environment outside the centralized store.
let environment = context.temp_dir.child(".venv");
uv_fs::remove_virtualenv(environment.path())?;
let external = context.temp_dir.child("external");
context
.venv()
.arg(external.path())
.arg("--python")
.arg("3.12")
.assert()
.success();
// Resolve a relative path file target from `.venv`'s parent.
environment.write_str("external")?;

// Like a directory link, use the path file's interpreter to select the cached environment.
uv_snapshot!(context.filters(), context.run()
.arg("--preview-features")
.arg("centralized-project-envs")
.arg("--no-sync")
.arg("--python")
.arg("3.11")
.arg("python")
.arg("-c")
.arg("import sys; print(f'{sys.version_info.major}.{sys.version_info.minor}')"), @r#"
success: true
exit_code: 0
----- stdout -----
3.12

----- stderr -----
warning: Using incompatible environment (`project-cp3.12.[X]-[HASH]`) due to `--no-sync` (The project environment's Python version does not satisfy the request: `Python 3.11`)
"#);
Ok(())
}
Loading
Loading