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.

Sign up for GitHub

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

Add warning when VIRTUAL_ENV is set but will not be respected in project commands #6864

Merged
merged 1 commit into from
Sep 3, 2024
Merged

Add warning when VIRTUAL_ENV is set but will not be respected in project commands #6864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

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

1 change: 1 addition & 0 deletions crates/uv-workspace/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ either = { workspace = true }
fs-err = { workspace = true }
glob = { 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
67 changes: 62 additions & 5 deletions crates/uv-workspace/src/workspace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ use tracing::{debug, trace, warn};

use pep508_rs::{MarkerTree, RequirementOrigin, VerbatimUrl};
use pypi_types::{Requirement, RequirementSource, SupportedEnvironments, VerbatimParsedUrl};
use uv_fs::Simplified;
use uv_fs::{Simplified, CWD};
use uv_normalize::{GroupName, PackageName, DEV_DEPENDENCIES};
use uv_warnings::warn_user;
use uv_warnings::{warn_user, warn_user_once};

use crate::pyproject::{Project, PyProjectToml, Source, ToolUvWorkspace};

Expand Down Expand Up @@ -442,7 +442,7 @@ impl Workspace {
/// it is resolved relative to the install path.
pub fn venv(&self) -> PathBuf {
/// Resolve the `UV_PROJECT_ENVIRONMENT` value, if any.
fn from_environment_variable(workspace: &Workspace) -> Option<PathBuf> {
fn from_project_environment_variable(workspace: &Workspace) -> Option<PathBuf> {
let value = std::env::var_os("UV_PROJECT_ENVIRONMENT")?;

if value.is_empty() {
Expand All @@ -458,8 +458,65 @@ impl Workspace {
Some(workspace.install_path.join(path))
}

// TODO(zanieb): Warn if `VIRTUAL_ENV` is set and does not match
from_environment_variable(self).unwrap_or_else(|| self.install_path.join(".venv"))
// Resolve the `VIRTUAL_ENV` variable, if any.
fn from_virtual_env_variable() -> Option<PathBuf> {
let value = std::env::var_os("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.
// Note this differs from `UV_PROJECT_ENVIRONMENT`
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)) = (
Comment on lines +486 to +487
Copy link
Member Author

@zanieb zanieb Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

A little awkward, but important for cases where .venv is being created in a project — it'd be weird not to warn on the first invocation but warn on later ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any value to the previous check when this one's already here? This just feels like a more general version of the previous one. The only thing I can think of is that this one will trigger on case-insensitive filesystems if the casing differs, while I don't think that is_same_file will.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure honestly. Seems safe to keep it.

cc @BurntSushi for a post-merge review if you have time.

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) {
warn_user_once!(
"`VIRTUAL_ENV={}` does not match the project environment path `{}` and will be ignored",
from_virtual_env.user_display(),
project_env.user_display()
);
}
}

project_env
}

/// The members of the workspace.
Expand Down
3 changes: 3 additions & 0 deletions crates/uv/tests/init.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ fn init_application() -> Result<()> {
Hello from foo!

----- stderr -----
warning: `VIRTUAL_ENV=[VENV]/` does not match the project environment path `.venv` and will be ignored
Using Python 3.12.[X] interpreter at: [PYTHON-3.12]
Creating virtualenv at: .venv
Resolved 1 package in [TIME]
Expand Down Expand Up @@ -296,6 +297,7 @@ fn init_application_package() -> Result<()> {
Hello from foo!

----- stderr -----
warning: `VIRTUAL_ENV=[VENV]/` does not match the project environment path `.venv` and will be ignored
Using Python 3.12.[X] interpreter at: [PYTHON-3.12]
Creating virtualenv at: .venv
Resolved 1 package in [TIME]
Expand Down Expand Up @@ -367,6 +369,7 @@ fn init_library() -> Result<()> {
Hello from foo!

----- stderr -----
warning: `VIRTUAL_ENV=[VENV]/` does not match the project environment path `.venv` and will be ignored
Using Python 3.12.[X] interpreter at: [PYTHON-3.12]
Creating virtualenv at: .venv
Resolved 1 package in [TIME]
Expand Down
1 change: 1 addition & 0 deletions crates/uv/tests/run.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1307,6 +1307,7 @@ fn run_from_directory() -> Result<()> {
3.12.[X]

----- stderr -----
warning: `VIRTUAL_ENV=[VENV]/` does not match the project environment path `.venv` and will be ignored
Using Python 3.12.[X] interpreter at: [PYTHON-3.12]
Creating virtualenv at: .venv
Resolved 1 package in [TIME]
Expand Down
144 changes: 144 additions & 0 deletions crates/uv/tests/sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1747,3 +1747,147 @@ fn sync_workspace_custom_environment_path() -> Result<()> {

Ok(())
}

// Test for warnings when `VIRTUAL_ENV` is set but will not be respected.
#[test]
fn sync_virtual_env_warning() -> Result<()> {
let context = TestContext::new("3.12");

let pyproject_toml = context.temp_dir.child("pyproject.toml");
pyproject_toml.write_str(
r#"
[project]
name = "project"
version = "0.1.0"
requires-python = ">=3.12"
dependencies = ["iniconfig"]
"#,
)?;

// We should not warn if it matches the project environment
uv_snapshot!(context.filters(), context.sync().env("VIRTUAL_ENV", context.temp_dir.join(".venv")), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
Resolved 2 packages in [TIME]
Prepared 1 package in [TIME]
Installed 1 package in [TIME]
+ iniconfig==2.0.0
"###);

// Including if it's a relative path that matches
uv_snapshot!(context.filters(), context.sync().env("VIRTUAL_ENV", ".venv"), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
Resolved 2 packages in [TIME]
Audited 1 package in [TIME]
"###);

// Or, if it's a link that resolves to the same path
#[cfg(unix)]
{
use fs_err::os::unix::fs::symlink;

let link = context.temp_dir.join("link");
symlink(context.temp_dir.join(".venv"), &link)?;

uv_snapshot!(context.filters(), context.sync().env("VIRTUAL_ENV", link), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
Resolved 2 packages in [TIME]
Audited 1 package in [TIME]
"###);
}

// But we should warn if it's a different path
uv_snapshot!(context.filters(), context.sync().env("VIRTUAL_ENV", "foo"), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
warning: `VIRTUAL_ENV=foo` does not match the project environment path `.venv` and will be ignored
Resolved 2 packages in [TIME]
Audited 1 package in [TIME]
"###);

// Including absolute paths
uv_snapshot!(context.filters(), context.sync().env("VIRTUAL_ENV", context.temp_dir.join("foo")), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
warning: `VIRTUAL_ENV=foo` does not match the project environment path `.venv` and will be ignored
Resolved 2 packages in [TIME]
Audited 1 package in [TIME]
"###);

// We should not warn if the project environment has been customized and matches
uv_snapshot!(context.filters(), context.sync().env("VIRTUAL_ENV", "foo").env("UV_PROJECT_ENVIRONMENT", "foo"), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
Using Python 3.12.[X] interpreter at: [PYTHON-3.12]
Creating virtualenv at: foo
Resolved 2 packages in [TIME]
Prepared 1 package in [TIME]
Installed 1 package in [TIME]
+ iniconfig==2.0.0
"###);

// But we should warn if they don't match still
uv_snapshot!(context.filters(), context.sync().env("VIRTUAL_ENV", "foo").env("UV_PROJECT_ENVIRONMENT", "bar"), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
warning: `VIRTUAL_ENV=foo` does not match the project environment path `bar` and will be ignored
Using Python 3.12.[X] interpreter at: [PYTHON-3.12]
Creating virtualenv at: bar
Resolved 2 packages in [TIME]
Prepared 1 package in [TIME]
Installed 1 package in [TIME]
+ iniconfig==2.0.0
"###);

let child = context.temp_dir.child("child");
child.create_dir_all()?;

// And `VIRTUAL_ENV` is resolved relative to the project root so with relative paths we should
// warn from a child too
uv_snapshot!(context.filters(), context.sync().env("VIRTUAL_ENV", "foo").env("UV_PROJECT_ENVIRONMENT", "foo").current_dir(&child), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
warning: `VIRTUAL_ENV=foo` does not match the project environment path `[TEMP_DIR]/foo` and will be ignored
Resolved 2 packages in [TIME]
Audited 1 package in [TIME]
"###);

// But, a matching absolute path shouldn't warn
uv_snapshot!(context.filters(), context.sync().env("VIRTUAL_ENV", context.temp_dir.join("foo")).env("UV_PROJECT_ENVIRONMENT", "foo").current_dir(&child), @r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
Resolved 2 packages in [TIME]
Audited 1 package in [TIME]
"###);

Ok(())
}
7 changes: 7 additions & 0 deletions crates/uv/tests/workspace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,7 @@ fn test_uv_run_with_package_virtual_workspace() -> Result<()> {
Success

----- stderr -----
warning: `VIRTUAL_ENV=[VENV]/` does not match the project environment path `.venv` and will be ignored
Using Python 3.12.[X] interpreter at: [PYTHON]
Creating virtualenv at: .venv
Resolved 8 packages in [TIME]
Expand All @@ -402,6 +403,7 @@ fn test_uv_run_with_package_virtual_workspace() -> Result<()> {
Success

----- stderr -----
warning: `VIRTUAL_ENV=[VENV]/` does not match the project environment path `.venv` and will be ignored
Resolved 8 packages in [TIME]
Prepared 2 packages in [TIME]
Installed 2 packages in [TIME]
Expand Down Expand Up @@ -435,6 +437,7 @@ fn test_uv_run_virtual_workspace_root() -> Result<()> {
Success

----- stderr -----
warning: `VIRTUAL_ENV=[VENV]/` does not match the project environment path `.venv` and will be ignored
Using Python 3.12.[X] interpreter at: [PYTHON-3.12]
Creating virtualenv at: .venv
Resolved 8 packages in [TIME]
Expand Down Expand Up @@ -479,6 +482,7 @@ fn test_uv_run_with_package_root_workspace() -> Result<()> {
Success

----- stderr -----
warning: `VIRTUAL_ENV=[VENV]/` does not match the project environment path `.venv` and will be ignored
Using Python 3.12.[X] interpreter at: [PYTHON]
Creating virtualenv at: .venv
Resolved 8 packages in [TIME]
Expand All @@ -504,6 +508,7 @@ fn test_uv_run_with_package_root_workspace() -> Result<()> {
Success

----- stderr -----
warning: `VIRTUAL_ENV=[VENV]/` does not match the project environment path `.venv` and will be ignored
Resolved 8 packages in [TIME]
Prepared 2 packages in [TIME]
Installed 2 packages in [TIME]
Expand Down Expand Up @@ -542,6 +547,7 @@ fn test_uv_run_isolate() -> Result<()> {
Success

----- stderr -----
warning: `VIRTUAL_ENV=[VENV]/` does not match the project environment path `.venv` and will be ignored
Using Python 3.12.[X] interpreter at: [PYTHON-3.12]
Creating virtualenv at: .venv
Resolved 8 packages in [TIME]
Expand Down Expand Up @@ -572,6 +578,7 @@ fn test_uv_run_isolate() -> Result<()> {
Success

----- stderr -----
warning: `VIRTUAL_ENV=[VENV]/` does not match the project environment path `.venv` and will be ignored
Resolved 8 packages in [TIME]
Audited 5 packages in [TIME]
"###
Expand Down
Loading