search

uber / prototool

Your Swiss Army Knife for Protocol Buffers
MIT License
5.04k stars 345 forks source link

Add protoc-gen-swift to Docker image #502

Closed surajbarkale closed 4 years ago

surajbarkale commented 5 years ago

This PR adds protoc-gen-swift tool from https://github.com/apple/swift-protobuf to the docker image. Since swift does not support static linking, I had to explicitly copy all .so dependencies into the final image. Since all other tools are statically linked, copying over glibc works.

I have tested locally by adding generate clause in prototool.yaml:

generate:
  plugins:
    - name: swift
      output: swift

Please let me know if any explicit tests have to be added.

I understand this PR may be rejected because of future conflicts with glibc. However, this will help someone who is using prototool and needs to generate swift bindings.

CLAassistant commented 5 years ago

CLA assistant check
All committers have signed the CLA.

orkunkl commented 4 years ago

Hello! What is the current status of this PR? This feature would be really cool.

smaye81 commented 4 years ago

Apologies for the delay. I have been a bit out of the loop/busy. I will take a look at this tonight. Thank you for putting this up.

smaye81 commented 4 years ago

Looks good. Can you run make dockerall to verify the container builds correctly and that the Docker tests all work?

surajbarkale commented 4 years ago

I have added tests and fixed issues with make dockerall. However, there is now a substantial change in the Dockerfile.

surajbarkale commented 4 years ago

I also found out that statically linked swift executables do not work on Linux! https://bugs.swift.org/browse/SR-648

smaye81 commented 4 years ago

Yeah so I'd really rather not add this level of complexity to the Dockerfile all for the sake of adding in Swift. Our preference is for you to build your own image based on uber/prototool anyway.

See here: https://github.com/uber/prototool/blob/dev/docs/docker.md

surajbarkale commented 4 years ago

Here is the gist of changes I have made to the image:

  1. Bumped GRPC version to 1.25.0 and protobuf version to 3.11.2. Running make dockerall on master is failing because the existing versions are no longer available in alpine:edge.

  2. Use alpine:3.11 instead of alpine:edge. Since alpine 3.10 contains older version of GRPC and protobuf than master.

  3. Changed go version to 1.13 since the go.mod file in master specifies this version.

  4. Move all version definitions to start of the Dockerfile and reuse these in build steps. This is a cleanup and makes it easier to check build versions.

  5. Split builder image into base & builder image. Otherwise the swift builder was always re-run.

  6. Added swift builder image as an independent image.

Changes 1, 2 & 3 are needed to fix make dockerall on master. Change 4 is an enhancement (you may disagree). Changes 5 & 6 bring in swift support.

I am documenting this here in case anyone wants to reuse code in this PR. Please feel free to close this PR if you don't want to merge it.

smaye81 commented 4 years ago

I think Steps 1-4 are valid. I am in the process of actually upgrading dependencies now. The dev branch should have most go.mod and protoc updates, but the Bazel build is failing, so I need to work that out. So, I would say Steps 1-3 I can take care of.

Step 4 is also a valid thing. I'd definitely welcome a PR of that if possible.

And yeah, I think I'd rather not do Steps 5-6 right now. Apologies, but at least you can use that code to build your own image based on the Prototool image.

smaye81 commented 4 years ago

Going to close this PR. If you decide to implement Step 4 in our above convo, please open a new PR.