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

Initial support for tokio #121

Open
wants to merge 23 commits into
base: sync-async
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
2afb5a1
Add sync feature to Cargo.toml
SapryWenInera May 12, 2024
e0b6be7
Add initial error message for when io logic is missing do to feature …
SapryWenInera May 12, 2024
975fbec
Add sync and tokio features and sync is on default
SapryWenInera May 13, 2024
c056382
Add compile error for missing/conflicting features
SapryWenInera May 13, 2024
8afd567
Begin async support on spec module
SapryWenInera May 13, 2024
5f548a3
Merge remote-tracking branch 'upstream/master' into async_pr
SapryWenInera May 13, 2024
45f918c
Move synchronous code into sync submodule
SapryWenInera May 14, 2024
aae1098
Merge branch 'master' into async_pr
Pr0methean May 16, 2024
a14f6a7
Merge branch 'master' into async_pr
Pr0methean May 17, 2024
62788e2
docs: Add `package.metadata.docs.rs`
sorairolake May 18, 2024
933ccc4
Enable deflate-zlib as well, and keep deflate64 separate
Pr0methean May 18, 2024
57eaa50
ci(fuzz): Update seed corpora
Pr0methean May 18, 2024
4b295d3
Merge pull request #135 from sorairolake/docsrs
Pr0methean May 18, 2024
dc83532
Merge branch 'zip-rs:master' into async_pr
SapryWenInera May 18, 2024
af5752b
Add sync feature to Cargo.toml
SapryWenInera May 12, 2024
f871fff
Add initial error message for when io logic is missing do to feature …
SapryWenInera May 12, 2024
951184a
Add sync and tokio features and sync is on default
SapryWenInera May 13, 2024
ff97edd
Add compile error for missing/conflicting features
SapryWenInera May 13, 2024
8129f2e
Finished rebasing to upstream/sync-async branch
SapryWenInera May 18, 2024
e5af56f
Begin async support on spec module
SapryWenInera May 13, 2024
c95da50
Move synchronous code into sync submodule
SapryWenInera May 14, 2024
2e6053e
Updated CI
SapryWenInera May 29, 2024
794a1d0
Sync feature enablement
SapryWenInera May 29, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ jobs:
matrix:
os: [ubuntu-latest, macOS-latest, windows-latest]
rustalias: [stable, nightly, msrv]
feature_flag: ["--all-features", "--no-default-features", ""]
feature_flag: ["--no-default-features --features sync_all", "--no-default-features --features tokio_all", "--no-default-features --features sync", "--no-default-features --features tokio", ""]
Copy link
Member

@Pr0methean Pr0methean May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please make it possible to build with both sync and tokio? To do this, you can invoke the traits with an explicit self parameter, e.g. Read::read_exact(self, buf) and AsyncRead::read_exact(self, buf) instead of self.read_exact(buf). Since methods with the same name don't conflict when they're specified in different traits, this should be the only change needed.

Copy link
Member

@Pr0methean Pr0methean Jun 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I found a crate that does something similar to this, but with proc macros: https://crates.io/crates/async-generic. Looks like it may still be worth layering a few regular macros on top, for when async-generic functions call other async-generic functions.

include:
- rustalias: stable
rust: stable
Expand Down
27 changes: 26 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,25 @@ edition = "2021"
exclude = ["tests/**", "examples/**", ".github/**", "fuzz/**"]
build = "src/build.rs"

[package.metadata.docs.rs]
features = [
"aes-crypto",
"bzip2",
"chrono",
"deflate",
"deflate64",
"deflate",
"deflate-zlib",
"deflate-zlib-ng",
"lzma",
"time",
"zstd",
]
# Can be enabled when <https://github.com/zopfli-rs/zopfli/issues/42> is fixed
# all-features = true
no-default-features = true
rustdoc-args = ["--cfg", "docsrs"]

[workspace.dependencies]
time = { version = "0.3.36", default-features = false }

Expand All @@ -43,6 +62,8 @@ zstd = { version = "0.13.1", optional = true, default-features = false }
zopfli = { version = "0.8.0", optional = true }
deflate64 = { version = "0.1.8", optional = true }
lzma-rs = { version = "0.3.0", default-features = false, optional = true }
true = { version = "0.1.0", optional = true }
tokio = { version = "1.37.0", optional = true }

[target.'cfg(any(all(target_arch = "arm", target_pointer_width = "32"), target_arch = "mips", target_arch = "powerpc"))'.dependencies]
crossbeam-utils = "0.8.19"
Expand All @@ -68,11 +89,14 @@ deflate = ["flate2/rust_backend", "_deflate-any"]

# DEPRECATED: previously enabled `flate2/miniz_oxide` which is equivalent to `flate2/rust_backend`
deflate-miniz = ["deflate", "_deflate-any"]

deflate-zlib = ["flate2/zlib", "_deflate-any"]
deflate-zlib-ng = ["flate2/zlib-ng", "_deflate-any"]
deflate-zopfli = ["zopfli", "_deflate-any"]
lzma = ["lzma-rs/stream"]
sync = []
sync_all = ["aes-crypto", "chrono", "deflate", "deflate-miniz", "deflate-zlib", "deflate-zlib-ng", "deflate-zopfli", "lzma", "sync","time"]
tokio_all = ["aes-crypto", "chrono", "deflate", "deflate-miniz", "deflate-zlib", "deflate-zlib-ng", "deflate-zopfli", "lzma","time", "tokio"]
tokio = ["tokio/io-util"]
unreserved = []
default = [
"aes-crypto",
Expand All @@ -82,6 +106,7 @@ default = [
"deflate-zlib-ng",
"deflate-zopfli",
"lzma",
"sync",
"time",
"zstd",
]
Expand Down
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
5 changes: 5 additions & 0 deletions src/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,9 @@ fn main() {
if var("CARGO_FEATURE_DEFLATE_MINIZ").is_ok() && var("CARGO_FEATURE__ALL_FEATURES").is_err() {
println!("cargo:warning=Feature `deflate-miniz` is deprecated; replace it with `deflate`");
}
#[cfg(not(any(feature = "sync", feature = "tokio")))]
compile_error!("Missing Required feature");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update ci.yml to enable sync wherever it has --no-default-features, or else convert it to a no-sync feature (which would be unidiomatic but have the advantage of being backward-compatible for more users).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the tokio feature is properly working in tests I will update the ci. Currently that is broken.

Copy link
Member

@Pr0methean Pr0methean May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for letting me know; but if it's going to take longer than about another 3 days, then I'd also greatly appreciate a CI-able update once or twice per week so we could get an idea of how the PR was progressing toward a releasable state where all further work could be deferred to follow-up PRs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I will try doing it this week after I some of my college exams.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SapryWenInera Have you finished your exams? If not, when do you expect to be able to address the comments on this PR? An ETA would be helpful not only for me, but also for the authors of the other major PRs, given the likelihood of merge conflicts.


#[cfg(all(feature = "sync", feature = "tokio"))]
compile_error!("The features sync and tokio cannot be used together")
Copy link
Member

@Pr0methean Pr0methean May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This restriction may turn out to be a problem for some users. It's possible to import two configurations of a crate twice by renaming one, but then they won't recognize each other's struct types or traits. I can think of two solutions:

  • (my preference) Give the async methods different names (e.g. parse_async) instead. If they're used in the same calling code, move that code to a macro and use method-scoped type names (e.g. type Read = AsyncRead; and macro parameters to differentiate them. See the example in my separate comment.
  • Make a separate crate for the Tokio features, and another for the shared core.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking of pushing the old synchronous code in a module of read named sync and then the async code would be in a tokio module, this would avoid conflicts between the codebases, by the end of the week i'm probably gonna push a PR for that since it doesn't require any feature or ci changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good, but will there be a shared-core module as well?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that the structs and enums can be shared but the actual logic can be separate. This should avoid import conflicts and if there are code changes in the sync part it should not conflict with the async part. Do u think this or the macro one is better?

Copy link
Member

@Pr0methean Pr0methean May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I favor sharing as much code as possible, based on the Don't Repeat Yourself principle. But the modular design should be compatible with the macro approach: define the macros in the shared core module, and invoke them in the sync and tokio modules.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SapryWenInera Could you please prioritize this issue for a fix, so that I can run CI on your work in progress and use the results to estimate how much longer this PR will take?

}
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
//! | ZipCrypto deprecated encryption | ✅ | ✅ |
//!
//!
#![cfg_attr(docsrs, feature(doc_auto_cfg))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

#![warn(missing_docs)]
#![allow(unexpected_cfgs)] // Needed for cfg(fuzzing) on nightly as of 2024-05-06
pub use crate::compression::{CompressionMethod, SUPPORTED_COMPRESSION_METHODS};
Expand Down
18 changes: 5 additions & 13 deletions src/read.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
//! Types for reading ZIP archives

/// Module for code with synchronous logic
#[cfg(feature = "sync")]
pub mod sync;

/// Module for code with asynchronous logic
#[cfg(feature = "tokio")]
pub mod tokio;

#[cfg(feature = "aes-crypto")]
use crate::{aes::AesReaderValid, types::AesVendorVersion};
use crate::{crc32::Crc32Reader, types::ZipFileData, zipcrypto::ZipCryptoReaderValid};
Expand Down Expand Up @@ -121,7 +126,6 @@ pub(crate) struct CentralDirectoryInfo {
mod test {
use crate::ZipArchive;
use std::io::Cursor;
use tempdir::TempDir;

#[test]
fn invalid_offset() {
Expand Down Expand Up @@ -316,16 +320,4 @@ mod test {
let mut file = reader.by_index(0).unwrap();
assert_eq!(file.read(&mut decompressed).unwrap(), 12);
}

#[test]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you deleting this?

fn test_is_symlink() -> std::io::Result<()> {
let mut v = Vec::new();
v.extend_from_slice(include_bytes!("../tests/data/symlink.zip"));
let mut reader = ZipArchive::new(Cursor::new(v)).unwrap();
assert!(reader.by_index(0).unwrap().is_symlink());
let tempdir = TempDir::new("test_is_symlink")?;
reader.extract(&tempdir).unwrap();
assert!(tempdir.path().join("bar").is_symlink());
Ok(())
}
}
1 change: 1 addition & 0 deletions src/read/stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use std::fs;
use std::io::{self, Read};
use std::path::{Path, PathBuf};

#[cfg(feature = "sync")]
use super::sync::{central_header_to_zip_file_inner, read_zipfile_from_stream};

/// Stream decoder for zip.
Expand Down
Loading