-
Notifications
You must be signed in to change notification settings - Fork 760
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 uvx
alias for uv tool run
#4632
Conversation
It turns out cargo-dist supports aliases! but I don't think that's actually what we want here, we have more control this way. |
I think we'd follow this with some special-casing to the help menu to display |
This is great, but one early concern: will this double the size of the wheel, by shipping two separate binaries that are mostly identical? |
@charliermarsh great question — ideally we'd make this binary teeeenie tiny. I'll investigate that but it sounds like I'll need to duplicate a lot more code? |
Mm yeah each of these is going to be large...
Maybe a cargo-dist alias is the way to go. |
@zanieb can't Edit: Or, potentially, an alias, and simply detect |
Yeah I think we're going to need to do something like that. I think the binary size increase in unacceptable for us and there's no reason for
|
Alright, this is nice and spicy.
|
let result = run(); | ||
match result { | ||
// Fail with 2 if the status cannot be cast to an exit code | ||
Ok(status) => u8::try_from(status.code().unwrap_or(2)).unwrap_or(2).into(), |
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.
This feels a little dubious, but is probably fine in practice. The documentation suggests the status code is just truncated to a u8 on Unix anyway? Definitely open to alternative approaches here.
let uv = bin.join("uv"); | ||
let args = ["tool", "run"] |
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.
In the future, this might invoke a hidden uv x
so we can special case the help menu and such?
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.
How did you think about the tradeoffs between this approach and using a symlink?
I'm actually shocked that this is 456 KB, so big! I wonder if we can strip it? https://github.com/johnthagen/min-sized-rust?tab=readme-ov-file#strip-symbols-from-binary
On unix definitely, a lot of binaries rely on moonlighting as different binaries, and i found it to also work on windows. Given that cargo-dist supports them, can we build on their experience? |
Closes #4476
Originally, this used the changes in #4642 to invoke
main()
from auvx
binary. This had the benefit ofuvx
being entirely standalone at the cost of doubling our artifact size. We think that's the incorrect trade-off.Instead, we assume
uvx
is always next touv
and create a tiny binary (<1MB) that invokesuv
in a child process. This seems preferable to acargo-dist
alias because we have more control over it. This binary should "just work" for all of our cargo-dist distributions and installers, but we'll need to add a new entry point for our PyPI distribution. I'll probably tackle support there separately?This includes some small overhead:
I presume there may be some other downsides to a child process? The wrapper is a little awkward. We could consider
execv
but this is complicated across platforms. An example implementation of that over in monotrail.