add provides-extras to lockfile #11063
Conversation
|
I still need to add a snapshot test where this is non-empty. |
crates/uv-resolver/src/lock/mod.rs
Outdated
| @@ -1786,6 +1786,16 @@ impl Package { | |||
| .collect::<Result<_, _>>() | |||
| .map_err(LockErrorKind::RequirementRelativePath)? | |||
| }; | |||
| let provides_extras = if id.source.is_immutable() { | |||
| Vec::new() | |||
There was a problem hiding this comment.
Something we need to consider: if we want to start using this to enforce that a given extra exists (at install time, by reading from the lockfile), we won't be able to tell the difference between "empty" and "absent". So we either need to not do that, or bump the lockfile version so we can tell the difference, or write these even when empty.
There was a problem hiding this comment.
Do you have a preference on which approach? "bump lockfile version" sounds principled but dunno if it's a Big Deal.
There was a problem hiding this comment.
I'm not sure. @zanieb, do you have an opinion? Bumping the lockfile version means that older uv versions will reject newer lockfiles (but we can still read older lockfiles in newer uv versions).
There was a problem hiding this comment.
We might just want to always write this (and always write requires-dist too). It's not, like, a huge change... Since we only show this for local packages anyway.
There was a problem hiding this comment.
In addition to always writing this we'd also need to make this like... an Option<Vec<ExtraName>> to distinguish whether we found it in the lockfile, right?
There was a problem hiding this comment.
I was thinking that we'd continue to omit it for immutable sources, is that doable? It should massively reduce the diff.
There was a problem hiding this comment.
It did remove a lot of them, yeah. (updated the commit)
There was a problem hiding this comment.
Generally in favor of less breaking lockfile changes :) it's decent timing with 0.6 coming up, but from what I understand this doesn't justify the churn?
There was a problem hiding this comment.
It's just to enable more validation on cli arguments, yeah. Not really worth a break.
There was a problem hiding this comment.
Yeah I don't think we should bump for it. I do wonder if we should treat requires-dist the same way, though: always write it, even if empty?
I think users will likely be annoyed that the lockfile is churning more (even without a bump) in that you're now getting different results if you lock with a later version (even though it should be backwards-compatible), but I don't know how we avoid that.
| .expect("metadata is present") | ||
| .provides_extras | ||
| .clone() | ||
| }; |
There was a problem hiding this comment.
I think you need to change to_toml to write these out. The impl is manual.
Gankra
force-pushed
the
gankra/lock-extra
branch
3 times, most recently
from
c88bee2 to
58d63de
Compare
Gankra
force-pushed
the
gankra/lock-extra
branch
from
11d6ebd to
487d5f5
Compare
this task is extremely haunted on this PR and I wish I had any reason to think this PR breaks it... |
god worst reason to die too |
I think @zanieb fixed this on |
## Summary This is an alternative to the approach we took in #11063 whereby we always included `provides-extra` and `requires-dist`, since we needed some way to differentiate between "no extras" and "lockfile was generated by a uv version that didn't include extras". Instead, this PR adds a minor version (called a "revision") to the lockfile that we can use to indicate support for this feature. While lockfile version bumps are backwards-incompatible, older uv versions _can_ read lockfiles with a later revision -- they just won't understand all the data. In a future major version bump, we could simplify things and change the schema to use a (major, minor) format instead of these two separate fields. But this is the only way to do it that's backwards-compatible with existing uv versions. --------- Co-authored-by: Zanie Blue <[email protected]>
Fixes #10953