-
Notifications
You must be signed in to change notification settings - Fork 899
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
Support locking relative paths #4205
Conversation
crates/uv/tests/lock.rs
Outdated
@@ -173,8 +173,8 @@ fn lock_sdist_registry() -> Result<()> { | |||
[[distribution]] | |||
name = "project" | |||
version = "0.1.0" | |||
source = "editable+file://[TEMP_DIR]/" | |||
sdist = { url = "file://[TEMP_DIR]/" } | |||
source = "editable+" |
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 is ugly, do we know something better?
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.
Perhaps just editable
? And adjust the parser to make a trailing +
as optional (or even an 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.
I think my preference is not using {}+{}
at all (either editable = "."
or split this source
into two fields), but for now i've added a editable+.
as workaround.
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.
Is this tracked somewhere?
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.
Added an item to #3611, i'm not yet deep enough here though to be clear on what the solution is.
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! This LGTM modulo the quirks you mentioned.
The { path = "" }
is unfortunate though. I'm not quite sure what to do about that. Maybe just sdist = {}
is okay?
crates/uv/tests/lock.rs
Outdated
source = "editable+file://[TEMP_DIR]/" | ||
sdist = { url = "file://[TEMP_DIR]/" } | ||
source = "editable+" | ||
sdist = { path = "" } |
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 is also a little weird too. I'm not quite sure what to do otherwise here. You could skip serializing empty paths, but then you'd wind up with sdist = {}
, which is also weird...
crates/uv/tests/lock.rs
Outdated
@@ -173,8 +173,8 @@ fn lock_sdist_registry() -> Result<()> { | |||
[[distribution]] | |||
name = "project" | |||
version = "0.1.0" | |||
source = "editable+file://[TEMP_DIR]/" | |||
sdist = { url = "file://[TEMP_DIR]/" } | |||
source = "editable+" |
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.
Perhaps just editable
? And adjust the parser to make a trailing +
as optional (or even an error).
d1f013b
to
9d8a49a
Compare
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!
crates/uv-resolver/src/lock.rs
Outdated
Registry(Url), | ||
Git(Url, GitSource), | ||
Direct(Url, DirectSource), | ||
Path(PathWire), |
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.
Hmmm I wonder if you could use PathBuf
directly here and then just use serialize_with
/deserialize_with
functions: https://serde.rs/field-attrs.html#deserialize_with. That would let you skip defining a new intermediate PathWire
type.
(Feel free to get this merged and experiment with it in a follow-up if you want. I'm fine with this as-is.)
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.
Good idea, this removes a lot of boilerplate
By splitting `path` into a lockable, relative (or absolute) and an absolute installable path and by splitting between urls and paths by dist type, we can store relative paths in the lockfile.
b6ab82b
to
a95e6fa
Compare
By splitting
path
into a lockable, relative (or absolute) and an absolute installable path and by splitting between urls and paths by dist type, we can store relative paths in the lockfile.