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

Support for nonseekable Writer #114

Closed
wants to merge 8 commits into from

Conversation

izderadicka
Copy link

@izderadicka izderadicka commented Oct 5, 2019

This is my initial attempt for #16 implementation.
Although the actual change is not big:
a) if stream is not seekable set 4th bit in general flags in file local header
b) when file is finished do not update file local header, but instead write data descriptor record.

However significant work was need to make ZipWriter to work with both Seek and non Seek streams and also keep current interface and functionality untouched. This should be non-breaking change - all current functionality works as it is. Only there is new associated method to create ZipWriter::new_streaming , which accepts steam that does not support Seek and through it new functionality will be available.

The key change is introduction of WriterWrapper struct, which calculates output stream position for nonseekable stream (based on number of bytes written there), but for seekable stream uses wrapped stream functionality - seek function. It also has method can_seek, which enables to decide (after file is finished) if we can update local file header.

I made one basic test and also tested result with unzip utility in linux, where it worked. I guess more tests and more work is needed, just wanted to check if this can be a way forward for this functionality.

As previous solution was messy I looked for help in community
and  came up with simpler and  more clear solution
now to generalize both  seekable and unseekable stream.
Also added one more test - when created into unseekable stream,
it can be extracted then from same data,
now presented as seekable stream.
@Plecra
Copy link
Member

Plecra commented Jun 21, 2020

This is very nice, thankyou! I don't see any reason not to merge this, but it's got a couple blockers: Could you rebase it on master and run rustfmt over your changes?

@izderadicka
Copy link
Author

@Plecra What about this - I see that build on 1.24 is failing but it is on miniz_oxide v0.4.0 which is dependency in code not used by this change.

@Plecra
Copy link
Member

Plecra commented Jul 8, 2020

Yes, it's unfortunately blocking the build at the moment. I should be able to manually review this soon

@Plecra Plecra mentioned this pull request Aug 24, 2020
@Plecra
Copy link
Member

Plecra commented Nov 15, 2020

@izderadicka I'll review these changes within the next week, and hopefully add them to an upcoming release. Lmk if there are any more changes you'd like to make first 😄

@nazar-pc
Copy link

Any updates on when maintainers will have time to upstream this?

@nazar-pc
Copy link

nazar-pc commented Apr 2, 2021

Just a quick update, currently using code from this PR and it seems to work really well. Thanks Ivan!

@Plecra
Copy link
Member

Plecra commented May 11, 2021

Annoyingly, there are too many changes to master to merge this right now. I'd like to revisit the feature soon, and this PR proves we can do that in 0.6 without breaking changes. Thankyou for the contribution 😄

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.

3 participants