Conversation
rbtcollins
left a comment
rbtcollins
left a comment
There was a problem hiding this comment.
I think the 'prefer X under Y circumstance thing can reasonably be added later. I have some review bits that should be addressed.
gz is already implicitly optional in the current code.
kinnison
force-pushed
the
add-zstd-support
branch
from
c502a37 to
b757add
Compare
rbtcollins
left a comment
rbtcollins
left a comment
There was a problem hiding this comment.
More suggestions but nothing that prevents merging.
| } else { | ||
| None | ||
| }; | ||
| let zst_hash = if enable_zst { |
There was a problem hiding this comment.
This to me looks like a loop trying to break free. Something for the future I guess.
There was a problem hiding this comment.
Yeah, it'll be nice to make this more generic and loopy in the future.
| ); | ||
| toml_target.insert(String::from("xz_hash"), toml::Value::String(xz_hash)); | ||
| } | ||
| if let Some(zst_hash) = hash.zst { |
There was a problem hiding this comment.
Can't the dist abstraction write this out ?
There was a problem hiding this comment.
I'll admit I lazily extended what existed already. We can certainly log to refactor this part of the tests.
| xzwriter = xz2::write::XzEncoder::new(outfile, 0); | ||
| &mut xzwriter | ||
| } | ||
| s if s.ends_with(".tar.zst") => { |
There was a problem hiding this comment.
These blocks here look like an enum trying to break free
There was a problem hiding this comment.
Yeah, I wasn't terribly happy about duplicating this code, but again I think the refactor can come later.
Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
If the manifest contains zst_tar and zst_hash then treat them as ZStd compressed data. Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
kinnison
force-pushed
the
add-zstd-support
branch
from
b757add to
100f7ca
Compare
|
I've rebased this and added the refactor of the |
2 participants
This builds atop #2675 and adds support for ZStd compression, both to rustup and to the test suite.
This fixes #2488 and in theory goes some way toward #1858 though given this is a linear preference of zst over xz over gz it's not entirely a fix.
This patch assumes it's okay to insert zstd into manifest v2 files without bumping the manifest version. It follows the
xz_tarpattern withzst_taret al. Also since it builds atop the refactor in #2675 it doesn't require that gz still be present, paving the way for seamless conversion from gz+xz, via some combination of gz+(xz and/or zst) to a pure zst manifest if that is of interest to the infra team.I'd appreciate opinions from @rust-lang/infra on this - should we bump the manifest version since this implicitly makes the gz files optional?