Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove extra slash in proto makefile and remove bindings #174

Merged
merged 6 commits into from
Aug 11, 2021

Conversation

buschbapti
Copy link
Contributor

The makefile in protobuf contained an extra / causing errors during the command

mkdir -p $(CPP_BUILD_DIR); protoc --proto_path=$(PROTO_PATH) --cpp_out=$(CPP_BUILD_DIR) $(PROTO_SOURCES)

The folder was actually not created.

Also, it is fair to say that we can actually remove the bindings from Github. The proper way of building them is actually to copy protobuf from the ghcr.io/epfl-lasa/control-libraries/development-dependencies:latest image:

# install google dependencies
COPY --from=ghcr.io/epfl-lasa/control-libraries/development-dependencies:latest /usr/local/include/google /usr/local/include/google
COPY --from=ghcr.io/epfl-lasa/control-libraries/development-dependencies:latest /usr/local/lib/libproto* /usr/local/lib
COPY --from=ghcr.io/epfl-lasa/control-libraries/development-dependencies:latest /usr/local/bin/protoc /usr/local/bin
RUN ldconfig

Personally I prefer to avoid compiled stuff on Github and this solution avoids having to build the lengthy protobuf so that seems a good compromise.

@domire8
Copy link
Contributor

domire8 commented Aug 10, 2021

Good for me, do you want to wait for Enricos review? If thats the case, I would appreciate if the run.sh script and the README would be updated as well. I think this is in the scope of this PR.
If you want this fixed fast, I can give you my okay now.

Copy link
Member

@eeberhard eeberhard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. The compiler protoc stripped the extra slash and handled it fine, but mkdir did not.

Regarding the binding source not being version controlled, I agree. Initially I saw the installation of protoc as a burden since it really does take a while to build, but now that I put it in the base dependencies image it's much easier for a user to generate the necessary code themselves (and in fact the CI regenerate these bindings fresh anyway). I still think if C++ libproto runtime could be installed easily without needing protoc, it would not be bad practice to version control the generated bindings. And, pulling a >2GB image just to generate <<1MB of code has its own disadvantages. But, overall, the clarity of the diff will be better if the bindings are removed 👍

I agree with @domire8 that the run.sh should also be deleted, and the protocol/README needs a few lines changed as a result. Finally, I would ask you to add BUILD_DIR (protocol/protobuf/bindings) to gitignore.

Thanks for the help in streamlining this as you are integrating clproto for downstream use

@domire8
Copy link
Contributor

domire8 commented Aug 11, 2021

And, pulling a >2GB image just to generate <<1MB of code has its own disadvantages. But, overall, the clarity of the diff will be better if the bindings are removed +1

I should not answer because you should be on holidays but I think Baptiste's final intention would be to host a lightweight image (FROM alpine and then install protobuf) such that you can simply pull this small image and then copy the google dependencies from there.

@buschbapti
Copy link
Contributor Author

Alright thank you both for the feedback. Indeed as @domire8 mentioned I think we should host some lightweight images based on alpine distribution to build and distribute such libraries. This will also facilitate the maintenance of versions. We can do that later and for now stick with the development image.

domire8
domire8 previously approved these changes Aug 11, 2021
Copy link
Contributor

@domire8 domire8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good job cleaner 😄

@buschbapti buschbapti merged commit 1fb1f47 into develop Aug 11, 2021
@buschbapti buschbapti deleted the fix/proto_makefile branch August 11, 2021 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants