-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Update Windows trampolines for compatibility with Python 3.7 and earlier #8649
base: main
Are you sure you want to change the base?
Conversation
Thanks for contributing! Unfortunately, I also worked on the trampoline today and this will conflict with #8637 – I presume that can be reconciled though. I'm a +1 on this. We'll want to hear from @konstin as well. cc @samypr100 if you're interested, I know you've done a fair bit of work on the trampolines. |
I am a bit confused since I thought the trampolines were written when 3.7 was not EOL — did we break support at some point? |
I'm not sure, but at the time the trampoline in posy was created, 3.7 must have been somewhat old, so it may simply have gone unnoticed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm -1 on supporting Python 3.7. Python 3.7 was already EOL when uv was released, and users should upgrade asap rather than patching the support into tools. Otherwise the code looks good.
Previously raised issue: #2445. I can verify uv fully works on Python 3.7 on Windows and the only issue remained is that trampoline issue. |
-1 for 3.7 support. |
That's correct both 3.7 and 3.8 EOL, but I think uv is still able to support them as in: uv/crates/uv-python/src/discovery.rs Lines 1767 to 1770 in 9fa4fea
|
One problem here is that I need to be able to sniff executables to tell if they're trampolines. I feel like that's trivial with the magic number at the end but kind of annoying here? If we're inverting the format I guess I'd expect
right? |
It would be difficult to put the magic number of the UV in a trivial position, since the magic number of the exe must be placed at the beginning of the file and the EOCD at the end. I think it is possible to insert the magic number in the last 2 bytes of the EOCD (where the length of the comment is stored). Whether this will work is implementation-dependent, but a normal zip implementation should work fine.
|
Yeah sorry, I forgot that there's the entire trampoline executable itself at the front.
We could hard-code this, I guess. Or pad to a specific size. I basically need to be able to inspect the trampoline, as in uv/crates/uv-trampoline-builder/src/lib.rs Lines 39 to 123 in 9cb7bcf
Notably with #8637 there isn't any zip payload at all in this variant, which perhaps simplifies things — we just won't be able to inspect the script variants. |
Anyway, I will read changes in #8637 . |
Generally speaking I'd assume you'd always want the magic at the |
# Conflicts: # crates/uv-install-wheel/src/wheel.rs # crates/uv-trampoline/src/bounce.rs # crates/uv-trampoline/tests/harness.rs # crates/uv-trampoline/trampolines/uv-trampoline-aarch64-console.exe # crates/uv-trampoline/trampolines/uv-trampoline-aarch64-gui.exe # crates/uv-trampoline/trampolines/uv-trampoline-i686-console.exe # crates/uv-trampoline/trampolines/uv-trampoline-i686-gui.exe # crates/uv-trampoline/trampolines/uv-trampoline-x86_64-console.exe # crates/uv-trampoline/trampolines/uv-trampoline-x86_64-gui.exe
If we parse the EOCD and read the magic number in the middle of the file, the probability of false positives is as low as with the current method (or more). |
One idea: In the case of UVSC, embedding "UV" in the comment length section of the EOCD and using the EOCD's magic number (PK\005\006) for detection allows for a robust and reliable method that only requires static reading. |
# Conflicts: # crates/uv/tests/it/tool_install.rs
I neglected this for a while, but I've now updated it to match the latest main!
In the end, whether or not we need 3.7 support is up to the maintainers, but I’ve made it so that we can merge this anyway. |
# Conflicts: # crates/uv-trampoline/trampolines/uv-trampoline-aarch64-console.exe # crates/uv-trampoline/trampolines/uv-trampoline-aarch64-gui.exe # crates/uv-trampoline/trampolines/uv-trampoline-i686-console.exe # crates/uv-trampoline/trampolines/uv-trampoline-i686-gui.exe # crates/uv-trampoline/trampolines/uv-trampoline-x86_64-console.exe # crates/uv-trampoline/trampolines/uv-trampoline-x86_64-gui.exe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, two comments that also can add them as follow-up.
@@ -156,9 +167,11 @@ impl LauncherKind { | |||
/// If the file cannot be read, an [`io::Error`] is returned. If the path is not a launcher, | |||
/// `None` is returned. | |||
#[allow(clippy::cast_possible_wrap)] | |||
pub fn try_from_file(file: &mut File) -> Result<Option<Self>, Error> { | |||
pub fn try_from_file(file: &mut File, skip_bytes: usize) -> Result<Option<Self>, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function return type could be Result<Self, Error>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your review and suggestion!
However, since it is LauncherKind::try_from_file that determines whether the file is actually a launcher, I believe returning Ok(None) makes sense when the file is not recognized as one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe returning Ok(None) makes sense when the file is not recognized as one.
I think a meaningful Error
variant also does the job.
- let Ok(_) = file.seek(io::SeekFrom::End(
- -((skip_bytes + MAGIC_NUMBER_SIZE) as i64),
- )) else {
- return Ok(None);
- };
+ file.seek(io::SeekFrom::End(
+ -((skip_bytes + MAGIC_NUMBER_SIZE) as i64),
+ )).map_err(Error::InvalidLauncherSeek)?; # <-- I think this is more proper return than `Ok(None)`
...
- Ok(Self::try_from_bytes(buffer))
+ Self::try_from_bytes(buffer).ok_or(Error::InvalidMagic)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting that, rather than just changing LauncherKind::try_from_path, we should also implement Launcher::try_from_path so that it does not return an Option in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error handling changes a little, but the operation is fine, so I switched to this method.
# Conflicts: # crates/uv-trampoline/trampolines/uv-trampoline-aarch64-console.exe # crates/uv-trampoline/trampolines/uv-trampoline-aarch64-gui.exe # crates/uv-trampoline/trampolines/uv-trampoline-i686-console.exe # crates/uv-trampoline/trampolines/uv-trampoline-i686-gui.exe # crates/uv-trampoline/trampolines/uv-trampoline-x86_64-console.exe # crates/uv-trampoline/trampolines/uv-trampoline-x86_64-gui.exe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, it looks great!
For reviewers' interests: uv somehow exposed its support of Python 3.7 via uv python
where Python 3.7 is lowest version of available distributions.
crates/uv-trampoline/README.md
Outdated
The intended use is: | ||
|
||
- take your Python script, name it `__main__.py`, and pack it into a `.zip` file. Then concatenate | ||
that `.zip` file onto the end of one of our prebuilt `.exe`s. | ||
- After the zip file content, write the path to the Python executable that the script uses to run | ||
- First, place our prebuilt `.exe` content at the top of the file. | ||
- After the exe file content, write the path to the Python executable that the script uses to run | ||
the Python script as UTF-8 encoded string, followed by the path's length as a 32-bit little-endian | ||
integer. | ||
- At the very end, write the magic number `UVUV` in bytes. | ||
- Write the magic number 'UVSC' or 'UVPY' in bytes. | ||
- Finally, rename your Python script as `__main__.py`, compress it into a `.zip` file, and append | ||
this `.zip` file to the end of one of our prebuilt `.exe` files. | ||
|
||
| `launcher.exe` | | ||
| :-------------------------: | | ||
| `<zipped python script>` | | ||
| `<path to python.exe>` | | ||
| `<len(path to python.exe)>` | | ||
| `<b'U', b'V', b'U', b'V'>` | | ||
| `<zipped python script>` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to have them separated to correctly show them in table:
### How do you use it as Python mirror?
Basically, this invokes the provided `python.exe` executable.
The intended use is:
- First, place our prebuilt `.exe` content at the top of the file.
- After the exe file content, write the path to the Python executable that the script uses to run
the Python script as UTF-8 encoded string, followed by the path's length as a 32-bit little-endian
integer.
- Write the magic number 'UVPY' in bytes.
| `launcher.exe` |
| :-------------------------: |
| `<path to python.exe>` |
| `<len(path to python.exe)>` |
| `<b'U', b'V', b'P', b'Y'>` |
### How do you use it as Script runner?
Basically, this looks up `python.exe` (for console programs) and invokes
`python.exe path\to\the\<the .exe>`.
The intended use is:
- First, place our prebuilt `.exe` content at the top of the file.
- After the exe file content, write the path to the Python executable that the script uses to run
the Python script as UTF-8 encoded string, followed by the path's length as a 32-bit little-endian
integer.
- Write the magic number 'UVSC' in bytes.
- Finally, rename your Python script as `__main__.py`, compress it into a `.zip` file, and append
this `.zip` file to the end of one of our prebuilt `.exe` files.
| `launcher.exe` |
| :-------------------------: |
| `<path to python.exe>` |
| `<len(path to python.exe)>` |
| `<b'U', b'V', b'S', b'C'>` |
| `<zipped python script>` |
Summary
uv-trampoline uses Python's zipimport feature in a slightly hacky way to provide exe files, but the current method causes errors in Python 3.7 and earlier.
Specifically, uv-trampoline stores the Python executable path and other information as archive comments in the zip file. However, zipimport before Python 3.7 does not support archive comments (https://docs.python.org/3/library/zipimport.html#:~:text=Changed%20in%20version%203.8%3A%20Previously%2C%20ZIP%20archives%20with%20an%20archive%20comment%20were%20not%20supported ).
When I actually run the installed exe with Python 3.7 on Windows, I get the following puzzling error.
This PR will support Python 3.7 and earlier by moving the path to Python, etc. (as well as the exe file content) before the zip portion.
Is this necessary?
Of course, Python 3.7 is EOL.
But it is still used in practice and as long as it is installable with uv, it is a good thing to support it. The change to support it is simple and will not change the cost of maintenance much.
Also, if this change is unacceptable, I think we should display an error indicating that the exe is not supported in Python 3.7 or earlier. In that case I am still willing to create a PR.
Test Plan
Simply install and run some tool using Python 3.7.
I added this for now because I don't really understand the uv code based testing method, but I will fix it if there are any problems.