Use reparse points to detect Windows installer shims #2284
Conversation
charliermarsh
force-pushed
the
charlie/win
branch
from
0253ada to
4bb4b39
Compare
| let mut path_encoded = path | ||
| .as_os_str() | ||
| .encode_wide() | ||
| .chain(std::iter::once(0)) |
There was a problem hiding this comment.
Previously, this wouldn't work for one of the two shims. But adding the null terminator seems to fix it on my machine.
There was a problem hiding this comment.
Adding the null termination seems correct. OsStr is not null terminated https://doc.rust-lang.org/std/ffi/struct.OsString.html
charliermarsh
force-pushed
the
charlie/win
branch
2 times, most recently
from
fc4efc5 to
60937ee
Compare
charliermarsh
added
bug
charliermarsh
force-pushed
the
charlie/win
branch
2 times, most recently
from
b0dabce to
c8e9a79
Compare
charliermarsh
force-pushed
the
charlie/win
branch
from
c8e9a79 to
9581d67
Compare
There was a problem hiding this comment.
Thanks for fixing this. I should have tested this workflow when improving windows support. And nice find with the null terminator
Have you considered upstreaming this back to Rye.
But what about https://twitter.com/charliermarsh/status/1765750403461689701
| std::ptr::null_mut(), | ||
| 0, | ||
| buf.as_mut_ptr().cast(), | ||
| buf.len() as u32 * 2, |
There was a problem hiding this comment.
Nit: not a 100 sure but I think we could use mem::size_of here (or size_of_value) to avoid the * 2
| CloseHandle(reparse_handle); | ||
| } | ||
|
|
||
| success && String::from_utf16_lossy(&buf).contains("\\AppInstallerPythonRedirector.exe") |
There was a problem hiding this comment.
Should this buf be sliced to bytes returned? It might not matter because it only tests startswidth but might still be good practice
| if !path.is_absolute() { | ||
| return false; | ||
| } | ||
|
|
||
| // The path must point to something like: | ||
| // `C:\Users\crmar\AppData\Local\Microsoft\WindowsApps\python3.exe` |
There was a problem hiding this comment.
Can we reuse the uv-fs function instead of reimplementing the check?
There was a problem hiding this comment.
They're actually different checks (sigh). The one in uv-fs needs to accept python3.12.exe and friends. The one here only accepts python3.exe and python.exe, since those are the only ones that get added as app redirects.
| return false; | ||
| } | ||
| // Ex) `WindowsApps` | ||
| if !components |
There was a problem hiding this comment.
This can be simplified to using components.ends_with("Microsoft/WindowsApps") after having done the fuzzy python.exe check
| @@ -130,7 +130,7 @@ fn find_python( | |||
| for name in possible_names.iter().flatten() { | |||
| if let Ok(paths) = which::which_in_global(&**name, Some(&path)) { | |||
| for path in paths { | |||
| if cfg!(windows) && windows::is_windows_store_shim(&path) { | |||
| if windows::is_windows_store_shim(&path) { | |||
There was a problem hiding this comment.
I don't know how to make code suggestions on ipad…
I would add #[cfg(windows)] to the if block instead of having an empty non-windows implementation in the windows module
There was a problem hiding this comment.
Sigh, I originally did this but the cross-platform Clippy warnings were so brutal.
This does NOT apply to Windows Store Python. |
Summary
This PR enables use of the Windows Store Pythons even with
pyis not installed. Specifically, we need to ensure that thepython.exeandpython3.exeexecutables installed into theC:\Users\crmar\AppData\Local\Microsoft\WindowsAppdirectory are used when they're not "App execution aliases" (which merely open the Windows Store, to help you install Python).When
pyis installed, this isn't strictly necessary, since the "resolved" executables are discovered viapy. These look likeC:\Users\crmar\AppData\Local\Microsoft\WindowsApps\PythonSoftwareFoundation.Python.3.11_qbs5n2kfra8p0\python.exe.Closes #2264.
Test Plan
py.cargo run venv --python python.exeandcargo run venv --python python3.exe,uvexited with a failure that no Python could be found.cargo run venv --python python3.10.exealso succeeded.