Enforce extension validity at parse time #5888
Conversation
charliermarsh
added
the
internal
|
I think this is a good change but it's more aggressive. It means we might error if a registry serves distribution kinds that we don't support, even if we'd never read them. |
|
Oh nevermind, we wouldn't fail in that case due to some other gating. |
crates/uv/tests/pip_install.rs
Outdated
| + tqdm==4.66.0 (from https://files.pythonhosted.org/packages/a5/d6/502a859bac4ad5e274255576cd3e15ca273cdb91731bc39fb840dd422ee9/tqdm-4.66.0-py3-none-any.whl) | ||
| + urllib3==2.2.1 (from https://files.pythonhosted.org/packages/a2/73/a68704750a7679d0b6d3ad7aa8d4da8e14e151ae82e6fee774e6e0d05ec8/urllib3-2.2.1-py3-none-any.whl) | ||
| error: Failed to parse metadata from built wheel | ||
| Caused by: Expected direct URL dependency to include an extension: `https://example.org/does/not/exist` |
There was a problem hiding this comment.
Can we say "file extension"? Why did this succeed previously?
charliermarsh
force-pushed
the
charlie/ext
branch
2 times, most recently
from
34c73c0 to
c569843
Compare
| Deserialize, | ||
| rkyv::Archive, | ||
| rkyv::Deserialize, | ||
| rkyv::Serialize, |
There was a problem hiding this comment.
The existing definition was:
#[derive(
Clone,
Debug,
PartialEq,
Eq,
Serialize,
Deserialize,
rkyv::Archive,
rkyv::Deserialize,
rkyv::Serialize,
)]
#[archive(check_bytes)]
#[archive_attr(derive(Debug))]
pub enum SourceDistExtension {
Zip,
TarGz,
TarBz2,
}I think this is safe without a cache bump? @BurntSushi?
There was a problem hiding this comment.
Hmmmmmmmmmm. Likely fine, but I actually do not believe it is guaranteed. rkyv doesn't have great docs on what kinds of changes are compatible with existing serialized data. I would generally assume that any change requires a cache bump, even if in some cases, it isn't in practice required.
charliermarsh
force-pushed
the
charlie/ext
branch
2 times, most recently
from
f394275 to
2407eea
Compare
| { | ||
| Ok(Self::TarXz) | ||
| } | ||
| "zst" |
There was a problem hiding this comment.
Thanks, that's wrong!
| Deserialize, | ||
| rkyv::Archive, | ||
| rkyv::Deserialize, | ||
| rkyv::Serialize, |
There was a problem hiding this comment.
I tried to find where rkyv was used with DistExtension, but couldn't. Do we need rkyv support for DistExtension? If not, I'd drop the trait impls and archive annotations below.
| Deserialize, | ||
| rkyv::Archive, | ||
| rkyv::Deserialize, | ||
| rkyv::Serialize, |
There was a problem hiding this comment.
Hmmmmmmmmmm. Likely fine, but I actually do not believe it is guaranteed. rkyv doesn't have great docs on what kinds of changes are compatible with existing serialized data. I would generally assume that any change requires a cache bump, even if in some cases, it isn't in practice required.
charliermarsh
force-pushed
the
charlie/ext
branch
from
2407eea to
598aedf
Compare
charliermarsh
force-pushed
the
charlie/ext
branch
3 times, most recently
from
17ef91a to
ffb1e23
Compare
|
I bumped the rkyv cache, to be conservative. |
charliermarsh
force-pushed
the
charlie/ext
branch
from
ffb1e23 to
dc7f3c8
Compare
charliermarsh
force-pushed
the
charlie/ext
branch
from
dc7f3c8 to
9c16728
Compare
This PR adds a `DistExtension` field to some of our distribution types, which requires that we validate that the file type is known and supported when parsing (rather than when attempting to unzip). It removes a bunch of extension parsing from the code too, in favor of doing it once upfront. Closes #5858.
Summary
This PR adds a
DistExtensionfield to some of our distribution types, which requires that we validate that the file type is known and supported when parsing (rather than when attempting to unzip). It removes a bunch of extension parsing from the code too, in favor of doing it once upfront.Closes #5858.