Use full requirement when serializing receipt #5494
Conversation
| @@ -396,9 +396,7 @@ fn tool_install_editable() { | |||
| ----- stderr ----- | |||
| "###); | |||
|
|
|||
| // Request `black`. It should retain the current installation. | |||
| // TODO(charlie): This is arguably incorrect, especially because the tool receipt removes the | |||
| // file path. | |||
There was a problem hiding this comment.
This was the motivating example.
crates/uv/tests/tool_install.rs
Outdated
| @@ -380,7 +380,7 @@ fn tool_install_editable() { | |||
| // We should have a tool receipt | |||
| assert_snapshot!(fs_err::read_to_string(tool_dir.join("black").join("uv-receipt.toml")).unwrap(), @r###" | |||
| [tool] | |||
| requirements = ["black @ file://[WORKSPACE]/scripts/packages/black_editable"] | |||
| requirements = [{ name = "black", directory = { install_path = "[WORKSPACE]/scripts/packages/black_editable", lock_path = "[WORKSPACE]/scripts/packages/black_editable", editable = true, url = { url = "file://[WORKSPACE]/scripts/packages/black_editable" } } }] | |||
There was a problem hiding this comment.
If we're comfortable with the general idea here, I will iron out the actual requirement schema.
|
Please just focus on the concept here (example) and ignore the code and exact representation for now. |
|
I think this makes sense to me. My only concern is that I want users to be able to declare tools declaratively eventually. I don't think that's in conflict though — I think the receipt can be considered "machine read / write" and "human inspectable" like the lock file. We can iron out a separate schema for users to declaratively request tools later. |
|
I'm not terribly familiar with tool receipts yet. Can you say more at a high level about what problem this is trying to solve? (Insomuch as I understand what this PR is doing, I think it LGTM.) |
|
Copying my response from Discord: |
charliermarsh
force-pushed
the
charlie/req-toml
branch
from
374186f
to
68f51ba
Compare
charliermarsh
force-pushed
the
charlie/req-toml
branch
from
68f51ba
to
7fc7f17
Compare
|
Ok happy with how this turned out. |
|
Ready for review @zanieb @BurntSushi at your convenience. |
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
If we're using a Git URL here, perhaps we should in the lock file too? Then I could just close #4631.
There was a problem hiding this comment.
That seems ok to me.
There was a problem hiding this comment.
Let me re-review that quickly, I think I lost it.
There was a problem hiding this comment.
Honestly, I'm fine with either!
charliermarsh
force-pushed
the
charlie/portable
branch
from
b4c0913
to
1add8be
Compare
## Summary I need to reuse this in #5494, so want to abstract it out and make it reusable.
charliermarsh
force-pushed
the
charlie/req-toml
branch
from
7fc7f17
to
24382d2
Compare
## Summary Fixes a bug in #5494. The `RequirementSourceWire` representation was ambiguous, and so the order of the fields meant that all variants were mapped to `Registry` when deserializing. (So the snapshots were right, but behaviors were wrong.)
## Summary In #5494, I made breaking changes to the tool receipt format. This would break existing tools for all users. This PR makes the change backwards-compatible by supporting deserialization for the deprecated format. Closes #5680. ## Test Plan Beyond the automated tests, you can run `cargo run tool list` on your existing machine. Before: ``` warning: `uv tool list` is experimental and may change without warning warning: Ignoring malformed tool `black` (run `uv tool uninstall black` to remove) warning: Ignoring malformed tool `poetry` (run `uv tool uninstall poetry` to remove) warning: Ignoring malformed tool `ruff` (run `uv tool uninstall ruff` to remove) ``` After: ``` warning: `uv tool list` is experimental and may change without warning black v0.1.0 - black poetry v1.8.3 - poetry ruff v0.0.60 - ruff ```
Summary
The current receipt doesn't capture quite enough information. For example, it doesn't differentiate between editable and non-editable requirements. This PR instead uses the full
Requirementtype. I think we should use a custom representation like we do in the lockfile, but I'm just using the default representation to demonstrate the idea.