[Issue #11637] Use suffix on uvx binary when searching for uv binary #11925
Conversation
| let Some(file_name_str) = os_file_name.to_str() else { | ||
| return Err(std::io::Error::new( | ||
| std::io::ErrorKind::NotFound, | ||
| "Could not determine the file name of the `uvx` binary", | ||
| )); | ||
| }; | ||
| let Some(file_name_no_ext) = file_name_str.strip_suffix(std::env::consts::EXE_SUFFIX) else { | ||
| return Err(std::io::Error::new( | ||
| std::io::ErrorKind::NotFound, | ||
| "Could not determine the file name of the `uvx` binary", | ||
| )); | ||
| }; | ||
| let Some(uvx_suffix) = file_name_no_ext.strip_prefix("uvx") else { | ||
| return Err(std::io::Error::new( | ||
| std::io::ErrorKind::NotFound, | ||
| "Could not determine the file name of the `uvx` binary", | ||
| )); | ||
| }; |
There was a problem hiding this comment.
Should all these error cases have different error messages? Seems helpful to distinguish between them if someone actually encounters these failures.
Does it make sense to use a io:Error here? NotFound seems weird in this context, though I'm not sure if it's worth complicating the run signature.
There was a problem hiding this comment.
I wasn't sure how much of the details we wanted to expose to users. I updated the code to have distinct error messages for each potential error situation, switched a couple of places to use InvalidData as the error kind instead of NotFound, and removed one of the error cases entirely in favor of using a fallback value.
|
I think this makes sense to me. Thanks for taking the time to contribute it. Do you think we should fallback to |
|
Falling back to plain
|
|
This assumes that |
|
Ok, it now does the following:
|
|
@zanieb Is there anything more I can do at this point to help get this fix/change merged? |
|
I don't think so, sorry — just about finding time to review. @Gankra can you own this? |
Gankra
left a comment
There was a problem hiding this comment.
I like the idea, but if we're willing to fallback to uv then it doesn't make sense to consider it an error for the uvx binary to have a weird/unparseable name. At most we could emit a warning?
24d1435 to
4088451
Compare
4088451 to
f1b9878
Compare
|
@Gankra I updated the code to emit a warning when falling back, and also to fall back when there is an issue in finding the extension for the |
This is a rebased and updated version of #11925 based on my review (I didn't have permission to push to their branch). For posterity I've preserved their commits but my final commit essentially rewrites the whole thing anyway. Fixes #11637 --------- Co-authored-by: Chris Lieb <clieb@bitsighttech.com>
|
Ah, sorry for the race, I've merged my impl! I should have mentioned I was going to push up a cleanup (incredible response time on a PR this old, wow!). Thanks for the great idea/contribution! |
Summary
When running uvx that has been installed using a pipx suffix, the corresponding uv binary cannot be found due to uvx always looking for an executable called
uvon the PATH, while it has instead been installed using a suffix. This change makes it so that uvx discovers the suffix to its name and then searches for a uv binary with the same suffix. For example, if uvx is calleduvx@0.6.2, then it will search foruv@0.6.2instead ofuv.Test Plan
uvanduvxbinaries from the PATHmainuvx->uvx@0.6.2anduv->uv@0.6.2uvx@0.6.2and observe that it produces the following output:uv@0.6.2anduvx@0.6.2binariesuvxand observe that it behaves like normal (maintains current behavior)uvx->uvx@0.6.2anduv->uv@0.6.2uvx@0.6.2and observe that it behaves like normal (exhibits correct behavior for suffixed executable names)uv@0.6.2->uvuvx@0.6.2and observe that it emits a warning about falling back to bareuv, followed by normaluvxoutputuvx@0.6.2and observe that it presents an error message stating thatuvcould not be found either with the suffixed name or with the bare nameResolves #11637