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

Consistent use of BIN_NAME in activation scripts #1577

Merged
merged 1 commit into from
Feb 17, 2024
Merged

Conversation

dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Feb 17, 2024

This PR fixes the bug where the BIN_NAME replacement field wasn't being used in the activator scripts.

fixes: #1518

Test plan

As I don't have a Windows machine, I switched the bin_name value here to point to Scripts on unix platform:

let bin_name = if cfg!(unix) {
"bin"
} else if cfg!(windows) {
"Scripts"
} else {
unimplemented!("Only Windows and Unix are supported")
};

Code diff

```diff
diff --git a/crates/gourgeist/src/bare.rs b/crates/gourgeist/src/bare.rs
index 4c7808d3..0e0b41cf 100644
--- a/crates/gourgeist/src/bare.rs
+++ b/crates/gourgeist/src/bare.rs
@@ -97,9 +97,9 @@ pub fn create_bare_venv(location: &Utf8Path, interpreter: &Interpreter) -> io::R
     // TODO(konstin): I bet on windows we'll have to strip the prefix again
     let location = location.canonicalize_utf8()?;
     let bin_name = if cfg!(unix) {
-        "bin"
-    } else if cfg!(windows) {
         "Scripts"
+    } else if cfg!(windows) {
+        "bin"
     } else {
         unimplemented!("Only Windows and Unix are supported")
     };

I then created the virtual environment as usual and tested out that the path modifications were correct:

$ cargo run --bin uv -- venv
    Finished dev [unoptimized + debuginfo] target(s) in 0.13s
     Running `target/debug/uv venv`
Using Python 3.12.1 interpreter at /Users/dhruv/.pyenv/versions/3.12.1/bin/python3.12
Creating virtualenv at: .venv

$ source .venv/Scripts/activate

$ echo $PATH
/Users/dhruv/work/astral/uv/.venv/Scripts:[...]

$ which python
/Users/dhruv/work/astral/uv/.venv/Scripts/python

I'm not sure how else to test this without having access to a Windows machine

@dhruvmanila dhruvmanila added the bug Something isn't working label Feb 17, 2024
@dhruvmanila dhruvmanila requested a review from zanieb February 17, 2024 06:26
@MichaReiser
Copy link
Member

Could you expand the summary with a test plan?

@T-256
Copy link
Contributor

T-256 commented Feb 17, 2024

Is this also fixes #1251?

@dhruvmanila
Copy link
Member Author

@MichaReiser I updated the PR description with a test plan but, as I don't have a windows machine, it's a bit of a workaround which should test it in a way as expected on a windows machine.

Is this also fixes #1251?

Probably not, \cc @konstin who can confirm once he's back from his PTO on Monday.

Copy link
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

Is this how these look in the upstream activation scripts?

@dhruvmanila
Copy link
Member Author

Is this how these look in the upstream activation scripts?

Yes, I basically copied from there. Sorry, I should've mentioned it in the PR description but here's an example from one of the activator script:

https://github.com/pypa/virtualenv/blob/fa283474fd199e3836f8b2c99510190c6b77e2bc/src/virtualenv/activation/bash/activate.sh#L55

@dhruvmanila dhruvmanila merged commit e4389e5 into main Feb 17, 2024
7 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/bin-name branch February 17, 2024 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent behavior when source .venv/Scripts/activate on Windows
4 participants