Skip to content

Commit

Permalink
Avoid comparing to system site packages in --dry-run mode (#11427)
Browse files Browse the repository at this point in the history 
## Summary

Right now, `uv sync --dry-run` returns the interpreter that _would've_
been used to create the environment; so we end up using _that_
interpreter's `site-packages`, and return changes relative to that
interpreter. Instead, we now create a temporary virtual environment and
compare against that.

Closes #11422.
  • Loading branch information
@charliermarsh
charliermarsh authored Feb 11, 2025
1 parent 2aaabb5 commit ecf40b9
Show file tree
Hide file tree
Showing 5 changed files with 125 additions and 66 deletions.
6 changes: 3 additions & 3 deletions crates/uv/src/commands/project/add.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ pub(crate) async fn add(
AddTarget::Project(project, Box::new(PythonTarget::Interpreter(interpreter)))
} else {
// Discover or create the virtual environment.
let venv = ProjectEnvironment::get_or_init(
let environment = ProjectEnvironment::get_or_init(
project.workspace(),
python.as_deref().map(PythonRequest::parse),
&install_mirrors,
Expand All @@ -239,9 +239,9 @@ pub(crate) async fn add(
printer,
)
.await?
.into_environment();
.into_environment()?;

AddTarget::Project(project, Box::new(PythonTarget::Environment(venv)))
AddTarget::Project(project, Box::new(PythonTarget::Environment(environment)))
}
};

Expand Down
130 changes: 86 additions & 44 deletions crates/uv/src/commands/project/mod.rs
View file
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::ops::Deref;
use std::path::{Path, PathBuf};
use std::sync::Arc;

Expand Down Expand Up @@ -183,6 +184,9 @@ pub(crate) enum ProjectError {
#[error("Failed to find `site-packages` directory for environment")]
NoSitePackages,

#[error("Attempted to drop a temporary virtual environment while still in-use")]
DroppedEnvironment,

#[error(transparent)]
DependencyGroup(#[from] DependencyGroupError),

Expand Down Expand Up @@ -937,15 +941,24 @@ enum ProjectEnvironment {
Existing(PythonEnvironment),
/// An existing [`PythonEnvironment`] was discovered, but did not satisfy the project's
/// requirements, and so was replaced.
///
/// In `--dry-run` mode, the environment will not be replaced, but this variant will still be
/// returned.
Replaced(PythonEnvironment, PathBuf),
Replaced(PythonEnvironment),
/// A new [`PythonEnvironment`] was created.
///
/// In `--dry-run` mode, the environment will not be created, but this variant will still be
/// returned.
New(PythonEnvironment, PathBuf),
Created(PythonEnvironment),
/// An existing [`PythonEnvironment`] was discovered, but did not satisfy the project's
/// requirements. A new environment would've been created, but `--dry-run` mode is enabled; as
/// such, a temporary environment was created instead.
WouldReplace(
PathBuf,
PythonEnvironment,
#[allow(unused)] tempfile::TempDir,
),
/// A new [`PythonEnvironment`] would've been created, but `--dry-run` mode is enabled; as such,
/// a temporary environment was created instead.
WouldCreate(
PathBuf,
PythonEnvironment,
#[allow(unused)] tempfile::TempDir,
),
}

impl ProjectEnvironment {
Expand Down Expand Up @@ -990,53 +1003,81 @@ impl ProjectEnvironment {

// Otherwise, create a virtual environment with the discovered interpreter.
ProjectInterpreter::Interpreter(interpreter) => {
let venv = workspace.venv(active);
let root = workspace.venv(active);

// Avoid removing things that are not virtual environments
let replace = match (venv.try_exists(), venv.join("pyvenv.cfg").try_exists()) {
let replace = match (root.try_exists(), root.join("pyvenv.cfg").try_exists()) {
// It's a virtual environment we can remove it
(_, Ok(true)) => true,
// It doesn't exist at all, we should use it without deleting it to avoid TOCTOU bugs
(Ok(false), Ok(false)) => false,
// If it's not a virtual environment, bail
(Ok(true), Ok(false)) => {
// Unless it's empty, in which case we just ignore it
if venv.read_dir().is_ok_and(|mut dir| dir.next().is_none()) {
if root.read_dir().is_ok_and(|mut dir| dir.next().is_none()) {
false
} else {
return Err(ProjectError::InvalidProjectEnvironmentDir(
venv,
root,
"it is not a compatible environment but cannot be recreated because it is not a virtual environment".to_string(),
));
}
}
// Similarly, if we can't _tell_ if it exists we should bail
(_, Err(err)) | (Err(err), _) => {
return Err(ProjectError::InvalidProjectEnvironmentDir(
venv,
root,
format!("it is not a compatible environment but cannot be recreated because uv cannot determine if it is a virtual environment: {err}"),
));
}
};

// Determine a prompt for the environment, in order of preference:
//
// 1) The name of the project
// 2) The name of the directory at the root of the workspace
// 3) No prompt
let prompt = workspace
.pyproject_toml()
.project
.as_ref()
.map(|p| p.name.to_string())
.or_else(|| {
workspace
.install_path()
.file_name()
.map(|f| f.to_string_lossy().to_string())
})
.map(uv_virtualenv::Prompt::Static)
.unwrap_or(uv_virtualenv::Prompt::None);

// Under `--dry-run`, avoid modifying the environment.
if dry_run.enabled() {
let environment = PythonEnvironment::from_interpreter(interpreter);
let temp_dir = cache.venv_dir()?;
let environment = uv_virtualenv::create_venv(
temp_dir.path(),
interpreter,
prompt,
false,
false,
false,
false,
)?;
return Ok(if replace {
Self::Replaced(environment, venv)
Self::WouldReplace(root, environment, temp_dir)
} else {
Self::New(environment, venv)
Self::WouldCreate(root, environment, temp_dir)
});
}

// Remove the existing virtual environment if it doesn't meet the requirements.
if replace {
match fs_err::remove_dir_all(&venv) {
match fs_err::remove_dir_all(&root) {
Ok(()) => {
writeln!(
printer.stderr(),
"Removed virtual environment at: {}",
venv.user_display().cyan()
root.user_display().cyan()
)?;
}
Err(e) if e.kind() == std::io::ErrorKind::NotFound => {}
Expand All @@ -1047,30 +1088,11 @@ impl ProjectEnvironment {
writeln!(
printer.stderr(),
"Creating virtual environment at: {}",
venv.user_display().cyan()
root.user_display().cyan()
)?;

// Determine a prompt for the environment, in order of preference:
//
// 1) The name of the project
// 2) The name of the directory at the root of the workspace
// 3) No prompt
let prompt = workspace
.pyproject_toml()
.project
.as_ref()
.map(|p| p.name.to_string())
.or_else(|| {
workspace
.install_path()
.file_name()
.map(|f| f.to_string_lossy().to_string())
})
.map(uv_virtualenv::Prompt::Static)
.unwrap_or(uv_virtualenv::Prompt::None);

let environment = uv_virtualenv::create_venv(
&venv,
&root,
interpreter,
prompt,
false,
Expand All @@ -1080,20 +1102,40 @@ impl ProjectEnvironment {
)?;

if replace {
Ok(Self::Replaced(environment, venv))
Ok(Self::Replaced(environment))
} else {
Ok(Self::New(environment, venv))
Ok(Self::Created(environment))
}
}
}
}

/// Convert the [`ProjectEnvironment`] into a [`PythonEnvironment`].
pub(crate) fn into_environment(self) -> PythonEnvironment {
///
/// Returns an error if the environment was created in `--dry-run` mode, as dropping the
/// associated temporary directory could lead to errors downstream.
#[allow(clippy::result_large_err)]
pub(crate) fn into_environment(self) -> Result<PythonEnvironment, ProjectError> {
match self {
Self::Existing(environment) => Ok(environment),
Self::Replaced(environment) => Ok(environment),
Self::Created(environment) => Ok(environment),
Self::WouldReplace(..) => Err(ProjectError::DroppedEnvironment),
Self::WouldCreate(..) => Err(ProjectError::DroppedEnvironment),
}
}
}

impl Deref for ProjectEnvironment {
type Target = PythonEnvironment;

fn deref(&self) -> &Self::Target {
match self {
Self::Existing(environment) => environment,
Self::Replaced(environment, ..) => environment,
Self::New(environment, ..) => environment,
Self::Replaced(environment) => environment,
Self::Created(environment) => environment,
Self::WouldReplace(_, environment, _) => environment,
Self::WouldCreate(_, environment, _) => environment,
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions crates/uv/src/commands/project/remove.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ pub(crate) async fn remove(
AddTarget::Project(project, Box::new(PythonTarget::Interpreter(interpreter)))
} else {
// Discover or create the virtual environment.
let venv = ProjectEnvironment::get_or_init(
let environment = ProjectEnvironment::get_or_init(
project.workspace(),
python.as_deref().map(PythonRequest::parse),
&install_mirrors,
Expand All @@ -237,9 +237,9 @@ pub(crate) async fn remove(
printer,
)
.await?
.into_environment();
.into_environment()?;

AddTarget::Project(project, Box::new(PythonTarget::Environment(venv)))
AddTarget::Project(project, Box::new(PythonTarget::Environment(environment)))
}
}
RemoveTarget::Script(script) => {
Expand Down
2 changes: 1 addition & 1 deletion crates/uv/src/commands/project/run.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -656,7 +656,7 @@ pub(crate) async fn run(
printer,
)
.await?
.into_environment()
.into_environment()?
};

if no_sync {
Expand Down
47 changes: 32 additions & 15 deletions crates/uv/src/commands/project/sync.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ pub(crate) async fn sync(
}

// Discover or create the virtual environment.
let environment = match ProjectEnvironment::get_or_init(
let environment = ProjectEnvironment::get_or_init(
project.workspace(),
python.as_deref().map(PythonRequest::parse),
&install_mirrors,
Expand All @@ -136,10 +136,12 @@ pub(crate) async fn sync(
dry_run,
printer,
)
.await?
{
ProjectEnvironment::Existing(environment) => {
if dry_run.enabled() {
.await?;

// In `--dry-run` mode, print the environment discovery or creation.
if dry_run.enabled() {
match &environment {
ProjectEnvironment::Existing(environment) => {
writeln!(
printer.stderr(),
"{}",
Expand All @@ -150,10 +152,29 @@ pub(crate) async fn sync(
.dimmed()
)?;
}
environment
}
ProjectEnvironment::Replaced(environment, root) => {
if dry_run.enabled() {
ProjectEnvironment::Replaced(environment) => {
writeln!(
printer.stderr(),
"{}",
format!(
"Replaced existing environment at: {}",
environment.root().user_display().bold()
)
.dimmed()
)?;
}
ProjectEnvironment::Created(environment) => {
writeln!(
printer.stderr(),
"{}",
format!(
"Created new environment at: {}",
environment.root().user_display().bold()
)
.dimmed()
)?;
}
ProjectEnvironment::WouldReplace(root, ..) => {
writeln!(
printer.stderr(),
"{}",
Expand All @@ -164,10 +185,7 @@ pub(crate) async fn sync(
.dimmed()
)?;
}
environment
}
ProjectEnvironment::New(environment, root) => {
if dry_run.enabled() {
ProjectEnvironment::WouldCreate(root, ..) => {
writeln!(
printer.stderr(),
"{}",
Expand All @@ -178,9 +196,8 @@ pub(crate) async fn sync(
.dimmed()
)?;
}
environment
}
};
}

// Initialize any shared state.
let state = UniversalState::default();
Expand Down

0 comments on commit ecf40b9

Please sign in to comment.