search

fussybeaver / bollard

Docker daemon API in Rust
Apache License 2.0
901 stars 134 forks source link

Custom Build Outputs `--output` #450

Closed levinwinter closed 2 months ago

levinwinter commented 2 months ago

Adds the ability to specify the exporter (currently tar and local). Details and a more thorough description is in the issue. Let me know what you think! I'm not so sure about codegen/proto/src/lib.rs... Feedback is welcome :)

Edit: I'm not sure how to deal with existing files/directories. If you have suggestions I can explicitly handle that case!

Closes #415

levinwinter commented 2 months ago

I'm quite sure that some edge-cases are not handled well for the local exporter (i.e. implementation of fsutil). I think it's good enough for what I need, but let me know what sort of maturity you expect.

fussybeaver commented 2 months ago

Fab! This must have been quite a headache to get to work. I will need to take some time to test your branch locally.

I am a little bit surprised that we've needed to circumvent filesend protobuf generated code. I'm wondering whether that's a lacking functionality in tonic or something that buildkit is doing.. did you find any issues / tickets in other repos about this?

levinwinter commented 2 months ago

You are absolutely right, it was quite a headache :P

I did some research and could find this issue: https://github.com/moby/buildkit/issues/2109. What they describe there matches my understanding! fsutil can use a "raw" gRPC stream and it seems to care little what is written in the protobuf definition. Unfortunately, both versions of the service (i.e. sending/receiving Packet or BytesMessage) would be required to support both the local and tar exporter, respectively. So we can't just change the definition to use fsutil.types.Packet like in the issue's PR.

And as for using "raw" gRPC streams in tonic, I couldn't find anything online. I was also thinking whether the existing generated code could be reused in some way (since the implementations are almost the same, except a few lines), but I also didn't manage to do that.

fussybeaver commented 2 months ago

This looks good, and happy to merge it after addressing a small cleanup.

We will need to add some additional local / tar export types for the "driver"-based API similar to what we use for the OCI exporter, but happy to address that down the road in a subsequent PR...

levinwinter commented 2 months ago

Sounds good! I refactored the include! things. If you think it's okay, feel free to merge :)