Skip to content
This repository has been archived by the owner on Jun 2, 2024. It is now read-only.

Implement a high-level, easy-to-use streaming decoder that recover all information from a zip #316

Merged
merged 19 commits into from
Feb 1, 2023

Conversation

NobodyXu
Copy link
Contributor

Resolved #314

Signed-off-by: Jiahao XU [email protected]

@NobodyXu NobodyXu marked this pull request as ready for review July 22, 2022 09:15
@NobodyXu
Copy link
Contributor Author

@zamazan4ik @Plecra Can you guys review this please?

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Aug 1, 2022

@zamazan4ik @Plecra Pinging

@zamazan4ik
Copy link
Contributor

Sorry for the delay. Since it is kinda important change, @Plecra is the only who can review it.

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Aug 1, 2022

Sorry for the delay. Since it is kinda important change, @Plecra is the only who can review it.

I do want review from @Plecra , though this PR isn't actually that complex.

Most of the work includes:

  • refactoring: Move ZipFile::enclosed_name and ZipFile::unix_mode to ZipFileData.
  • refactoring: Create central_header_to_zip_file_inner that does not check signature (so that I can use it in the new stream decoder).
  • Create ZipStreamReader using central_header_to_zip_file_inner, read_zipfile_from_stream and ZipFileData

@zamazan4ik You definitely can give me review on this one, anything will be welcome.

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Aug 2, 2022

@zamazan4ik BTW, can you approve the workflow?

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Aug 2, 2022

@zamazan4ik Thanks, I will fix them.

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Aug 2, 2022

@zamazan4ik I have fixed the error in build, however I cannot fix the errors in clippy because many of them are out of the scope of this PR.

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Aug 2, 2022

Oops, my naive fixed doesn't work.
I've forced push a new commit.

@zamazan4ik Is there any way to approve CI for all new future commits from me?

@zamazan4ik
Copy link
Contributor

I do not know. I have no control over repo settings for CI.

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Aug 2, 2022

@zamazan4ik No worries, thanks for taking your time!
Can you approve it again?

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Aug 2, 2022

@zamazan4ik Thanks again!

@Plecra
Copy link
Member

Plecra commented Oct 13, 2022

@NobodyXu hi there, had a look at your trouble with binstall and id like to get to work on making this right :) my health continues to hound me, but I'm going to make a point of at least taking time out each week to respond to pending messages, and ill make sure @zamazan4ik has the ability to make releases through me.

ive had a skim through your PR, and I certainly see the utility. most of the work we have been managing to get done is removing accidental stability promises in the API which had been paralysing the implementation. that's my only hangup with including these APIs, as they increase the interdependence between each of the features in the crate.

would it suit you to include this in a separate unstable module, and require that it is only used through a pinned version (that's zip = "=0.6.4" presumably)? If that'll be alright, this PR looks top and we can certainly bring it into tree. If not, the code certainly looks reasonable and clearly has a use case in your crate :) so I'll defer to zamazan to decide what's best.

@NobodyXu
Copy link
Contributor Author

would it suit you to include this in a separate unstable module, and require that it is only used through a pinned version (that's zip = "=0.6.4" presumably)? If that'll be alright, this PR looks top and we can certainly bring it into tree. If not, the code certainly looks reasonable and clearly has a use case in your crate :) so I'll defer to zamazan to decide what's best.

I'm ok to put it into unstable first, as it seems to still have some issues to be resolved when testing it in cargo-binstall.

@Plecra
Copy link
Member

Plecra commented Feb 1, 2023

I'll merge this once we've moved all the new API into zip::unstable::*. It's top of the todo list once I've gone through the issue tracker 😀

that does not require `reader` to implement `io::Seek`.

Signed-off-by: Jiahao XU <[email protected]>
As in streaming mode, there is no way to ignore the leading junk.

Signed-off-by: Jiahao XU <[email protected]>
NobodyXu and others added 4 commits February 1, 2023 17:33
@Plecra Plecra merged commit e32db51 into zip-rs:master Feb 1, 2023
@NobodyXu NobodyXu deleted the streaming branch February 2, 2023 00:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decompress from stream
3 participants