gzp icon indicating copy to clipboard operation
gzp copied to clipboard

Support writers without static lifetimes

Open tofay opened this issue 1 year ago • 2 comments

I've been trying to use this crate with https://github.com/containers/ocidir-rs to create container images. Getting that working required two changes to gzp:

  • the ability to return the underlying writer from Zwriter::finish
  • the ability to use borrowed writers in ParCompressBuilder::from_borrowed_writer

The latter is accomplished by allowing users to provide a https://doc.rust-lang.org/stable/std/thread/struct.Scope.html alongside the writer - the writer thread is then spawned using this scope.

tofay avatar Oct 01 '24 12:10 tofay

Hi @tofay - I'm sorry for not seeing this earlier. Do you have an example branch of your fork of this in use that I could look at? I think I get what you're saying, I'm just trying to understand the motivation for wanting this. I'm a bit reticent to add a lifetime parameter to an already fairly complex type, but I'm not against it if the use case is compelling!

sstadick avatar Feb 20 '25 14:02 sstadick

Hi @tofay - I'm sorry for not seeing this earlier. Do you have an example branch of your fork of this in use that I could look at? I think I get what you're saying, I'm just trying to understand the motivation for wanting this. I'm a bit reticent to add a lifetime parameter to an already fairly complex type, but I'm not against it if the use case is compelling!

I think this is a solid change, though I understand the hesitancy of adding lifetimes..

I find the problem with requiring a static lifetime here is that without it, using the output of a compressed stream is somewhat difficult. Constructing a reference to write to that is static is hard when you want to use the resource later. This seems to be also shown in the test included in the first pull request (I can't think of a way to get the same behavior in the current implementation without reconstructing the reference to the file).

I also ran into the problem of getting the data out of the compressor here and the best I could come up with was using a channel to pass the data out which I think comes with some overhead (not benchmarked).

Some1and2-XC avatar Apr 17 '25 20:04 Some1and2-XC

https://github.com/tofay/gnoci/commit/5ab6d2024c9ab19617d6a91dc1be668612ace9d7 is an example. I'm creating container images with https://github.com/containers/ocidir-rs. That uses cap-std, and the writes of container images are done using a non-static lifetime. To speed up compression I can use gzp, but I can't use the existing ParCompressBuilder::from_writer as is. With my branch of gzp I can do all the writing within a thread scope.

tofay avatar Jun 29 '25 20:06 tofay

@sstadick please take a look at https://github.com/tofay/gnoci/commit/5ab6d2024c9ab19617d6a91dc1be668612ace9d7 to understand my use case here?

tofay avatar Sep 17 '25 10:09 tofay

Could you update the CHANGELOG with an entry for this addition?

Thank you for pinging me with the use case example and for persisting in asking about this! The example really helped me understand why it's useful. Let's get this in.

Once this is merged in, I'll take a pass at updating dependencies / generally doing some project maintenance and then make a release.

Thanks! I've now added the changelog entry.

tofay avatar Sep 18 '25 18:09 tofay

Thanks @tofay! I will try to take a look at the failing lints and tests this week. The lints are for sure not something on your end, and I'm pretty sure the failing tests are also not related to your changes.

sstadick avatar Sep 22 '25 17:09 sstadick

@tofay see https://github.com/tofay/gzp/pull/1

sstadick avatar Oct 07 '25 17:10 sstadick

@tofay see v2.0.0

sstadick avatar Oct 07 '25 20:10 sstadick

@Some1and2-XC see v2.0.0

sstadick avatar Oct 07 '25 20:10 sstadick