Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support --active for PEP 723 script environments #11433

Merged
merged 1 commit into from
Feb 13, 2025
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
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions crates/uv-cli/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3053,10 +3053,11 @@ pub struct SyncArgs {
#[arg(long, overrides_with("inexact"), hide = true)]
pub exact: bool,

/// Prefer the active virtual environment over the project's virtual environment.
/// Sync dependencies to the active virtual environment.
///
/// If the project virtual environment is active or no virtual environment is active, this has
/// no effect.
/// Instead of creating or updating the virtual environment for the project or script, the
/// active virtual environment will be preferred, if the `VIRTUAL_ENV` environment variable is
/// set.
#[arg(long, overrides_with = "no_active")]
pub active: bool,

Expand Down Expand Up @@ -3150,7 +3151,6 @@ pub struct SyncArgs {
/// adherence with PEP 723.
#[arg(
long,
conflicts_with = "active",
conflicts_with = "all_packages",
conflicts_with = "package",
conflicts_with = "no_install_project",
Expand Down
1 change: 1 addition & 0 deletions crates/uv-fs/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ fs-err = { workspace = true }
fs2 = { workspace = true }
path-slash = { workspace = true }
percent-encoding = { workspace = true }
same-file = { workspace = true }
schemars = { workspace = true, optional = true }
serde = { workspace = true, optional = true }
tempfile = { workspace = true }
Expand Down
32 changes: 32 additions & 0 deletions crates/uv-fs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,38 @@ pub mod cachedir;
mod path;
pub mod which;

/// Attempt to check if the two paths refer to the same file.
///
/// Returns `Some(true)` if the files are missing, but would be the same if they existed.
pub fn is_same_file_allow_missing(left: &Path, right: &Path) -> Option<bool> {
// First, check an exact path comparison.
if left == right {
return Some(true);
}

// Second, check the files directly.
if let Ok(value) = same_file::is_same_file(left, right) {
return Some(value);
};

// Often, one of the directories won't exist yet so perform the comparison up a level.
if let (Some(left_parent), Some(right_parent), Some(left_name), Some(right_name)) = (
left.parent(),
right.parent(),
left.file_name(),
right.file_name(),
) {
match same_file::is_same_file(left_parent, right_parent) {
Ok(true) => return Some(left_name == right_name),
Ok(false) => return Some(false),
_ => (),
}
};

// We couldn't determine if they're the same.
None
}

/// Reads data from the path and requires that it be valid UTF-8 or UTF-16.
///
/// This uses BOM sniffing to determine if the data should be transcoded
Expand Down
1 change: 0 additions & 1 deletion crates/uv-workspace/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ glob = { workspace = true }
itertools = { workspace = true }
owo-colors = { workspace = true }
rustc-hash = { workspace = true }
same-file = { workspace = true }
schemars = { workspace = true, optional = true }
serde = { workspace = true, features = ["derive"] }
thiserror = { workspace = true }
Expand Down
30 changes: 3 additions & 27 deletions crates/uv-workspace/src/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -560,7 +560,7 @@ impl Workspace {
Some(workspace.install_path.join(path))
}

// Resolve the `VIRTUAL_ENV` variable, if any.
/// Resolve the `VIRTUAL_ENV` variable, if any.
fn from_virtual_env_variable() -> Option<PathBuf> {
let value = std::env::var_os(EnvVars::VIRTUAL_ENV)?;

Expand All @@ -578,38 +578,14 @@ impl Workspace {
Some(CWD.join(path))
}

// Attempt to check if the two paths refer to the same directory.
fn is_same_dir(left: &Path, right: &Path) -> Option<bool> {
// First, attempt to check directly
if let Ok(value) = same_file::is_same_file(left, right) {
return Some(value);
};

// Often, one of the directories won't exist yet so perform the comparison up a level
if let (Some(left_parent), Some(right_parent), Some(left_name), Some(right_name)) = (
left.parent(),
right.parent(),
left.file_name(),
right.file_name(),
) {
match same_file::is_same_file(left_parent, right_parent) {
Ok(true) => return Some(left_name == right_name),
Ok(false) => return Some(false),
_ => (),
}
};

// We couldn't determine if they're the same
None
}

// Determine the default value
let project_env = from_project_environment_variable(self)
.unwrap_or_else(|| self.install_path.join(".venv"));

// Warn if it conflicts with `VIRTUAL_ENV`
if let Some(from_virtual_env) = from_virtual_env_variable() {
if !is_same_dir(&from_virtual_env, &project_env).unwrap_or(false) {
if !uv_fs::is_same_file_allow_missing(&from_virtual_env, &project_env).unwrap_or(false)
{
match active {
Some(true) => {
debug!(
Expand Down
1 change: 1 addition & 0 deletions crates/uv/src/commands/project/add.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ pub(crate) async fn add(
allow_insecure_host,
&install_mirrors,
no_config,
active,
cache,
printer,
)
Expand Down
1 change: 1 addition & 0 deletions crates/uv/src/commands/project/export.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ pub(crate) async fn export(
allow_insecure_host,
&install_mirrors,
no_config,
Some(false),
cache,
printer,
)
Expand Down
1 change: 1 addition & 0 deletions crates/uv/src/commands/project/lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ pub(crate) async fn lock(
allow_insecure_host,
&install_mirrors,
no_config,
Some(false),
cache,
printer,
)
Expand Down
111 changes: 87 additions & 24 deletions crates/uv/src/commands/project/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ use uv_resolver::{
};
use uv_scripts::Pep723ItemRef;
use uv_settings::PythonInstallMirrors;
use uv_static::EnvVars;
use uv_types::{BuildIsolation, EmptyInstalledPackages, HashStrategy};
use uv_warnings::{warn_user, warn_user_once};
use uv_workspace::dependency_groups::DependencyGroupError;
Expand Down Expand Up @@ -532,30 +533,88 @@ pub(crate) enum ScriptInterpreter {

impl ScriptInterpreter {
/// Return the expected virtual environment path for the [`Pep723Script`].
pub(crate) fn root(script: Pep723ItemRef<'_>, cache: &Cache) -> PathBuf {
let entry = match script {
// For local scripts, use a hash of the path to the script.
Pep723ItemRef::Script(script) => {
let digest = cache_digest(&script.path);
if let Some(file_name) = script
.path
.file_stem()
.and_then(|name| name.to_str())
.and_then(cache_name)
{
format!("{file_name}-{digest}")
} else {
digest
///
/// If `--active` is set, the active virtual environment will be preferred.
///
/// See: [`Workspace::venv`].
pub(crate) fn root(script: Pep723ItemRef<'_>, active: Option<bool>, cache: &Cache) -> PathBuf {
/// Resolve the `VIRTUAL_ENV` variable, if any.
fn from_virtual_env_variable() -> Option<PathBuf> {
let value = std::env::var_os(EnvVars::VIRTUAL_ENV)?;

if value.is_empty() {
return None;
};

let path = PathBuf::from(value);
if path.is_absolute() {
return Some(path);
};

// Resolve the path relative to current directory.
Some(CWD.join(path))
}

// Determine the stable path to the script environment in the cache.
let cache_env = {
let entry = match script {
// For local scripts, use a hash of the path to the script.
Pep723ItemRef::Script(script) => {
let digest = cache_digest(&script.path);
if let Some(file_name) = script
.path
.file_stem()
.and_then(|name| name.to_str())
.and_then(cache_name)
{
format!("{file_name}-{digest}")
} else {
digest
}
}
}
// For remote scripts, use a hash of the URL.
Pep723ItemRef::Remote(.., url) => cache_digest(url),
// Otherwise, use a hash of the metadata.
Pep723ItemRef::Stdin(metadata) => cache_digest(&metadata.raw),
// For remote scripts, use a hash of the URL.
Pep723ItemRef::Remote(.., url) => cache_digest(url),
// Otherwise, use a hash of the metadata.
Pep723ItemRef::Stdin(metadata) => cache_digest(&metadata.raw),
};

cache
.shard(CacheBucket::Environments, entry)
.into_path_buf()
};
cache
.shard(CacheBucket::Environments, entry)
.into_path_buf()

// If `--active` is set, prefer the active virtual environment.
if let Some(from_virtual_env) = from_virtual_env_variable() {
if !uv_fs::is_same_file_allow_missing(&from_virtual_env, &cache_env).unwrap_or(false) {
match active {
Some(true) => {
debug!(
"Using active virtual environment `{}` instead of script environment `{}`",
from_virtual_env.user_display(),
cache_env.user_display()
);
return from_virtual_env;
}
Some(false) => {}
None => {
warn_user_once!(
"`VIRTUAL_ENV={}` does not match the script environment path `{}` and will be ignored; use `--active` to target the active environment instead",
from_virtual_env.user_display(),
cache_env.user_display()
);
Comment on lines +600 to +604
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to show on uv run too? I think that'd be surprising, for a script. Though it does seem useful for sync.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I can scope it to sync. I'm a little uncomfortable with the inconsistency... but I get it.

}
}
}
} else {
if active.unwrap_or_default() {
debug!(
"Use of the active virtual environment was requested, but `VIRTUAL_ENV` is not set"
);
}
}

// Otherwise, use the cache root.
cache_env
}

/// Discover the interpreter to use for the current [`Pep723Item`].
Expand All @@ -569,6 +628,7 @@ impl ScriptInterpreter {
allow_insecure_host: &[TrustedHost],
install_mirrors: &PythonInstallMirrors,
no_config: bool,
active: Option<bool>,
cache: &Cache,
printer: Printer,
) -> Result<Self, ProjectError> {
Expand All @@ -581,7 +641,8 @@ impl ScriptInterpreter {
requires_python,
} = ScriptPython::from_request(python_request, workspace, script, no_config).await?;

let root = Self::root(script, cache);
let root = Self::root(script, active, cache);

match PythonEnvironment::from_root(&root, cache) {
Ok(venv) => {
if python_request.as_ref().map_or(true, |request| {
Expand Down Expand Up @@ -1279,6 +1340,7 @@ impl ScriptEnvironment {
allow_insecure_host: &[TrustedHost],
install_mirrors: &PythonInstallMirrors,
no_config: bool,
active: Option<bool>,
cache: &Cache,
dry_run: DryRun,
printer: Printer,
Expand All @@ -1296,6 +1358,7 @@ impl ScriptEnvironment {
allow_insecure_host,
install_mirrors,
no_config,
active,
cache,
printer,
)
Expand All @@ -1306,7 +1369,7 @@ impl ScriptEnvironment {

// Otherwise, create a virtual environment with the discovered interpreter.
ScriptInterpreter::Interpreter(interpreter) => {
let root = ScriptInterpreter::root(script, cache);
let root = ScriptInterpreter::root(script, active, cache);

// Determine a prompt for the environment, in order of preference:
//
Expand Down
1 change: 1 addition & 0 deletions crates/uv/src/commands/project/remove.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ pub(crate) async fn remove(
allow_insecure_host,
&install_mirrors,
no_config,
active,
cache,
printer,
)
Expand Down
3 changes: 3 additions & 0 deletions crates/uv/src/commands/project/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ hint: If you are running a script with `{}` in the shebang, you may need to incl
allow_insecure_host,
&install_mirrors,
no_config,
active.map_or(Some(false), Some),
cache,
DryRun::Disabled,
printer,
Expand Down Expand Up @@ -346,6 +347,7 @@ hint: If you are running a script with `{}` in the shebang, you may need to incl
allow_insecure_host,
&install_mirrors,
no_config,
active.map_or(Some(false), Some),
cache,
DryRun::Disabled,
printer,
Expand Down Expand Up @@ -402,6 +404,7 @@ hint: If you are running a script with `{}` in the shebang, you may need to incl
allow_insecure_host,
&install_mirrors,
no_config,
active.map_or(Some(false), Some),
cache,
printer,
)
Expand Down
1 change: 1 addition & 0 deletions crates/uv/src/commands/project/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ pub(crate) async fn sync(
allow_insecure_host,
&install_mirrors,
no_config,
active,
cache,
dry_run,
printer,
Expand Down
1 change: 1 addition & 0 deletions crates/uv/src/commands/project/tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ pub(crate) async fn tree(
allow_insecure_host,
&install_mirrors,
no_config,
Some(false),
cache,
printer,
)
Expand Down
Loading
Loading