Skip to content
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

Please consider supporting the current release of async_zip #14

Open
musicinmybrain opened this issue Apr 15, 2024 · 4 comments
Open

Please consider supporting the current release of async_zip #14

musicinmybrain opened this issue Apr 15, 2024 · 4 comments

Comments

@musicinmybrain
Copy link
Contributor

I was investigating packaging this crate for Fedora Linux as a dependency for a potential uv package.

It would be very helpful if async_http_range_reader could support the current release of async_zip, 0.0.17, instead of requiring 0.0.15. Release notes for 0.0.16 are here and for 0.0.17 are here.


I tried to start a PR for updating at least to 0.0.16. I needed to make a couple of changes because as of 0.0.16, StoredZipEntry now implements Deref (but no longer has an entry() method):

--- a/src/lib.rs
+++ b/src/lib.rs
@@ -707,7 +707,7 @@ mod test {
                 .file()
                 .entries()
                 .iter()
-                .map(|e| e.entry().filename().as_str().unwrap_or(""))
+                .map(|e| e.filename().as_str().unwrap_or(""))
                 .collect::<Vec<_>>(),
             vec![
                 "metadata.json",
@@ -735,7 +735,7 @@ mod test {
         // Get the size of the entry plus the header + size of the filename. We should also actually
         // include bytes for the extra fields but we don't have that information.
         let size =
-            entry.entry().compressed_size() + 30 + entry.entry().filename().as_bytes().len() as u64;
+            entry.compressed_size() + 30 + entry.filename().as_bytes().len() as u64;
 
         // The zip archive uses as BufReader which reads in chunks of 8192. To ensure we prefetch
         // enough data we round the size up to the nearest multiple of the buffer size.

Unfortunately, this isn’t enough to make the tests pass:

    Finished test [unoptimized + debuginfo] target(s) in 0.04s
     Running unittests src/lib.rs (target/debug/deps/async_http_range_reader-579b4a2869383b07)

running 6 tests
test sparse_range::test::test_sparse_range ... ok
test test::test_not_found ... ok
test test::async_range_reader::case_2 ... ok
test test::async_range_reader::case_1 ... ok
test test::async_range_reader_zip::case_2 ... FAILED
test test::async_range_reader_zip::case_1 ... FAILED

failures:

---- test::async_range_reader_zip::case_2 stdout ----
thread 'test::async_range_reader_zip::case_2' panicked at src/lib.rs:766:9:
assertion `left == right` failed
  left: 3
 right: 2
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- test::async_range_reader_zip::case_1 stdout ----
thread 'test::async_range_reader_zip::case_1' panicked at src/lib.rs:766:9:
assertion `left == right` failed
  left: 3
 right: 2


failures:
    test::async_range_reader_zip::case_1
    test::async_range_reader_zip::case_2

test result: FAILED. 4 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.13s
@baszalmstra
Copy link
Collaborator

Yeah unfortunately the update to async-rs removes buffering while reading the central archive which results in more range requests. I still have to figure out the best way to properly fix this.

@charliermarsh Did you end up finding a proper fix for uv? Or are you using a workaround?

@charliermarsh
Copy link
Contributor

@baszalmstra - Unfortunately I ended up reverting that specific change in my own fork: charliermarsh/rs-async-zip#2

@baszalmstra
Copy link
Collaborator

@musicinmybrain Would it be possible to use charlies fork for the PR?

@musicinmybrain
Copy link
Contributor Author

Yes, we’re able to bundle git dependencies in cases like this where it’s necessary/justifiable. I appreciate the link to the PR, since it clearly explains why this is needed.

I am investigating whether we should create a compat package for async_zip 0.0.15 just to support packaging async_http_range_reader separately, or whether under the circumstances it should just be bundled alongside the forked async_zip.

Thanks for helping explain the situation. I do hope that there will be a way to go back to https://crates.io/crates/async_zip at some point in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants