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

Add CMD support #1523

Merged
merged 3 commits into from
Feb 17, 2024
Merged

Add CMD support #1523

merged 3 commits into from
Feb 17, 2024

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Feb 16, 2024

Sumamry

This PR adds the activation.bat, deactivation.bat and pyenv.bat files to add support for using uv from CMD.

This PR further fixes an issue with our trampoline implementation where calling an executable like black failed:

(venv) C:\Users\Micha\astral\test>where black
C:\Users\Micha\astral\test\.venv\Scripts\black.exe

(venv) C:\Users\Micha\astral\test>black
C:\Users\Micha\AppData\Local\Programs\Python\Python312\python.exe: can't open file 'C:\\Users\\Micha\\astral\\test\\black': [Errno 2] No such file or directory

The issue was that CMD doesn't extend black to its full path before passing it to the trampoline and our trampoline generated the command <python> black instead of <python> .venv/Scripts/black, and Python can't find black in the project directory.

This PR fixes this by using the full executable name (that we already parsed out to discover the Python version). This adds one complication, we need to preserve the arguments without repeating the executable name that is the first argument.
One option is to use CommandLineToArgvW and then serialize the arguments 1.. to a string again. I decided against that. Win32 API calls are easy to get wrong. That's why I implemented the parsing rules specified in CommandLineToArgvW to skip the first argument.

Fixes #1471

Test Plan

bat.file.support.mp4

Powershell continues to work

powershell.mp4

I haven't been able to test the aarch binaries.

@MichaReiser MichaReiser marked this pull request as draft February 16, 2024 18:40
@MichaReiser MichaReiser added the compatibility Compatibility with a specification or another tool label Feb 16, 2024
@zanieb zanieb added the windows Specific to the Windows platform label Feb 16, 2024
Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

Nice! I left a few comments where I think unsafe can be removed, but some do appear necessary. With that said, this seems somewhat more brutal than we probably really need. If the trampoline could use std, then I don't think we'd need to re-roll arg parsing/quoting ourselves. But that's a bigger issue and we shouldn't block fixing this issue on it.

use core::mem::MaybeUninit;
use core::{
ffi::CStr,
ptr::{addr_of, addr_of_mut, null, null_mut},
};

use alloc::{ffi::CString, vec, vec::Vec};
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, how come you moved this? I'd group it with core above since it's part of the standard library.

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 hit organize imports in RustRover and I guess, that's what came out. I don't mind reverting

crates/uv-trampoline/src/bounce.rs Outdated Show resolved Hide resolved
);
unsafe {
ExitProcess(1);
}
Copy link
Member

Choose a reason for hiding this comment

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

Bah. I assume we need to do this because uv-trampoline doesn't have std?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably, but I don't know. I only moved this line (I'm trying to keep my changes minimal)

});
buffer.push(b'\0');

unsafe { CString::from_vec_with_nul_unchecked(buffer) }
Copy link
Member

Choose a reason for hiding this comment

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

I'd drop the unsafe here too. (Although I do believe it is correct.) And replace it with CString::new(buffer).expect("no interior NUL bytes").

Copy link
Member Author

@MichaReiser MichaReiser Feb 17, 2024

Choose a reason for hiding this comment

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

The null terminator on line 138 should guarantee the null termination ;)

I tried to keep the implementation consistent with the existing code that exclusively use the unchecked variation because of what's mentioned here

Miscellaneous tips:
- `cargo-bloat` is a useful tool for checking what code is ending up in the
final binary and how much space it's taking. (It makes it very obvious whether
you've pulled in `core::fmt`!)
- Lots of Rust built-in panicking checks will pull in `core::fmt`, e.g., if you
ever use `.unwrap()` then suddenly our binaries double in size, because the
`if foo.is_none() { panic!(...) }` that's hidden inside `.unwrap()` will
invoke `core::fmt`, even if the unwrap will actually never fail.
`.unwrap_unchecked()` avoids this. Similar for `slice[idx]` vs
`slice.get_unchecked(idx)`.

I don't mind reconsidering the decision to prefer checked versions over unchecked ones but we might want to do this separately because we should look at the generated artifact size etc.

For example, I changed the unchecked call in find_python_exe to CString::from_vec_with_nul(buffer).expect("String to be correctly null terminated") and it increases the binary size from 16896 to 23040. From what I understand, the size of the executables is a primary concern of this library.

The size is slightly smaller for CString::new(buffer).expect("String to be correctly null terminated") but it still increases to 22528. That's why I think this must be a more holistic decision where we decide to favor safety over binary size (it's still tiny)

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind reconsidering the decision to prefer checked versions over unchecked ones but we might want to do this separately because we should look at the generated artifact size etc.

I reluctantly agree that this is probably the wise course of action for now.

Yeah, binary size is a primary concern for this particular library but I would like to eventually litigate whether it's a primary concern for us.

crates/uv-trampoline/src/bounce.rs Outdated Show resolved Hide resolved
}

// TODO copy tests from MSDN for parsing
fn skip_one_argument(arguments: &[u8]) -> &[u8] {
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 tried to write tests for this function but failed to get this crate to compile with cargo test.... I ended up coping the implementation into another crate and do some smoke testing (and I found and fixed a bug in the implementation)

@MichaReiser MichaReiser marked this pull request as ready for review February 17, 2024 19:15
@charliermarsh charliermarsh merged commit b296c04 into main Feb 17, 2024
7 checks passed
@charliermarsh charliermarsh deleted the bat-activation-script branch February 17, 2024 21:47
@charliermarsh
Copy link
Member

(Gonna merge this based on comments + responses so it can be part of v0.1.4.)


@set "VIRTUAL_ENV={{ VIRTUAL_ENV_DIR }}"

@set "VIRTUAL_ENV_PROMPT=venv"
Copy link
Member

Choose a reason for hiding this comment

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

I think this might mean that the prompt in a cmd shell always has (venv) prepended to it when the virtual environment is activated, even when the virtual environment is in a directory with a different name:

image

It looks like the CPython activate.bat script has some more complex logic around this (but I know nothing about .bat files)

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps #1570 addresses this?

Copy link
Member

Choose a reason for hiding this comment

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

Right, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility Compatibility with a specification or another tool windows Specific to the Windows platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No activate.bat Created with venv Command on Windows
5 participants