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

feat: cmd.exe detection heuristic #2226

Merged
merged 3 commits into from
Mar 6, 2024

Conversation

samypr100
Copy link
Collaborator

@samypr100 samypr100 commented Mar 6, 2024

Summary

Follow up from discussion in #2223

Detect CMD.exe by checking if PROMPT env var is set on windows, otherwise assume it's PowerShell.

Note, this will not work if user modifies their system env vars to include PROMPT by default or if they launch nested PowerShell from Command Prompt (e.g. Developer PowerShell for VS 2022).

Test Plan

Only tested locally, although we try to add some CI tests that specifically use CMD.exe

Command Prompt

Microsoft Windows [Version 10.0.19044.3086]
(c) Microsoft Corporation. All rights reserved.

Z:\Users\samypr100\dev\uv>Z:\Users\samypr100\.cargo\bin\cargo.exe +stable run --color=always -- venv "Foo Bar"
    Finished dev [unoptimized + debuginfo] target(s) in 0.69s
     Running `target\debug\uv.exe venv "Foo Bar"`
Using Python 3.12.2 interpreter at: Z:\Users\samypr100\AppData\Local\Programs\Python\Python312\python.exe
Creating virtualenv at: Foo Bar
Activate with: "Foo Bar\Scripts\activate"

Power Shell

Windows PowerShell
Copyright (C) Microsoft Corporation. All rights reserved.

Try the new cross-platform PowerShell https://aka.ms/pscore6

PS Z:\Users\samypr100\dev\uv>Z:\Users\samypr100\.cargo\bin\cargo.exe +stable run --color=always -- venv "Foo Bar"
    Finished dev [unoptimized + debuginfo] target(s) in 0.63s
     Running `target\debug\uv.exe venv "Foo Bar"`
Using Python 3.12.2 interpreter at: Z:\Users\samypr100\AppData\Local\Programs\Python\Python312\python.exe
Creating virtualenv at: Foo Bar
Activate with: & "Foo Bar\Scripts\activate"

@samypr100 samypr100 marked this pull request as ready for review March 6, 2024 02:49
@@ -255,14 +262,17 @@ fn shlex_posix(executable: impl AsRef<Path>) -> String {
}

/// Quote a path, if necessary, for safe use in `PowerShell`.
fn shlex_windows(executable: impl AsRef<Path>) -> String {
fn shlex_windows(executable: impl AsRef<Path>, shell: Shell) -> String {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should just merge shlex_posix and shlex_windows, and pass in Shell to toggle the behavior -- wdyt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I kinda like the separation between Posix and Windows, but I'm biased 😆

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good :)

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Thanks!

@charliermarsh charliermarsh added enhancement New feature or improvement to existing functionality windows Specific to the Windows platform labels Mar 6, 2024
@charliermarsh charliermarsh enabled auto-merge (squash) March 6, 2024 03:22
@charliermarsh charliermarsh merged commit 2ebcef9 into astral-sh:main Mar 6, 2024
7 checks passed
@samypr100 samypr100 deleted the cmd-detection branch March 6, 2024 03:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement to existing functionality windows Specific to the Windows platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants