Support installing additional executables in uv tool install#7592
Support installing additional executables in uv tool install#7592lucab wants to merge 20 commits intoastral-sh:mainfrom
uv tool install#7592Conversation
zanieb
left a comment
There was a problem hiding this comment.
Nice! This seems like a great start.
crates/uv/tests/tool_install.rs
Outdated
| Installed 11 executables: ansible, ansible-config, ansible-connection, ansible-console, ansible-doc, ansible-galaxy, ansible-inventory, ansible-playbook, ansible-pull, ansible-test, ansible-vault | ||
| Installed 1 executable: ansible-lint | ||
| Installed 1 executable: ansible-community |
There was a problem hiding this comment.
We'll probably want to collapse these and/or include the "from" package
There was a problem hiding this comment.
Do you have a preference on how this should look like?
For the moment I've updated it to:
Installed 11 additional executables from `ansible-core`: ansible, ansible-config, ansible-connection, ansible-console, ansible-doc, ansible-galaxy, ansible-inventory, ansible-playbook, ansible-pull, ansible-test, ansible-vault
Installed 1 additional executable from `ansible-lint`: ansible-lint
Installed 1 executable: ansible-community
I'm absolutely not sold on this being the final form.
There was a problem hiding this comment.
The part that stands out to me there is that it's a bit weird to say "additional" before we show the main group of executables we installed.
I think I'd go with whatever is decent and simple here, then revisit the output entirely. For example, I think this should probably match the style of our install commands? Like:
Installed 13 executables
+ ansible (from ansible-core)
+ ansible-config (from ansible-core)
+ ansible-lint
+ ansible-community
...
If the package name matches the executable name I'd probably omit the "from" portion.
Anyway, I think that is separate from the goal here and we should avoid going down that road in this pull request.
crates/uv/tests/tool_install.rs
Outdated
| .arg("--i-want-ponies") | ||
| .arg("ansible-core") | ||
| .arg("--i-want-ponies") | ||
| .arg("ansible-lint") | ||
| .arg("ansible==9.3.0") |
There was a problem hiding this comment.
Minor note: This is probably an expensive test because the package is big. We might want to find a smaller package to keep the test case minimal.
There was a problem hiding this comment.
Many dependencies were coming from ansible-lint, which is an extra package providing an additional entrypoint. I replaced that with black, as it covers the same scenario with fewer transitive deps.
If you have an idea about a better combo than ansible and ansible-core, I'll happily replace them here.
67a3ac0 to
e40e3ff
Compare
49cb25e to
3a05460
Compare
| + pycparser==2.21 | ||
| + pyyaml==6.0.1 | ||
| + resolvelib==1.0.1 | ||
| Installed 11 executables from `ansible-core`: ansible, ansible-config, ansible-connection, ansible-console, ansible-doc, ansible-galaxy, ansible-inventory, ansible-playbook, ansible-pull, ansible-test, ansible-vault |
There was a problem hiding this comment.
There is a funny issue here, it seems that on Windows only there is a different sorting for the strings ansible and ansible-<whatever>:
- Installed 11 executables from `ansible-core`: ansible, ansible-config, ansible-connection, ansible-console, ansible-doc, ansible-galaxy, ansible-inventory, ansible-playbook, ansible-pull, ansible-test, ansible-vault
+ Installed 11 executables from `ansible-core`: ansible-config, ansible-connection, ansible-console, ansible-doc, ansible-galaxy, ansible-inventory, ansible-playbook, ansible-pull, ansible-test, ansible-vault, ansibleThere was a problem hiding this comment.
I had to abuse CI a bit to get to the bottom of this (sorry if I flooded you with notifications).
It turns out there is a .exe suffix (filtered out in test output) and thus ansible < ansible-config(.exe) < ansible.exe (due to the ASCII order of - vs .).
The same suffix is also present in the receipt, making its ordering OS-dependant.
There was a problem hiding this comment.
I'm used to Charlie force-pushing 1000 times per pull request so no worries.
Ah that's really interesting. Good sleuthing. I was about to suggest trimming the suffix for sorts but it looks like you're on it in 0c9f450 — should you use the std::env::consts::EXE_SUFFIX const there?
The filtering can be quite misleading like that sometimes. I wonder if we can output an unfiltered snapshot on failure too? cc @konstin the snapshot macro master.
ab8e780 to
0c9f450
Compare
uv tool installuv tool install
|
I think this should be in a complete state now, and there are functional tests for install/upgrade/uninstall flows. The main remaining thing to decide is the name of the flag, in order to finalize the PR. |
d291b85 to
0f39fcb
Compare
0f39fcb to
fe56b74
Compare
|
Thanks! I'll try to review this tomorrow. edit: I had some time to give it a quick look now |
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub from: Option<String>, |
There was a problem hiding this comment.
Will adding this break old tool installs, i.e., if it's missing?
There was a problem hiding this comment.
The only other location from is used is in upgrade.rs to collect dependency packages (excluding the main tool) that have entry points. If from is None, then only entry points from site packages are installed in install_executables.
|
Anything is left here to do? from the latest review seems to be implied that it's ready. |
|
Yeah there's still work to do here. i.e., #7592 (comment) is important. I'd have to go deeper on the topic again to see if there's more. Are you interested in picking it up? |
|
Bumping this as "very nice to have". If there is testing to perform, I'm willing to help out, but my knowledge of this codebase is low and my available time is limited. |
This enhances
uv tool installin order to allow installing executables from multiple packages in a single call.Closes #6314