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

Container-based proto compilation #136

Closed
wants to merge 7 commits into from

Conversation

n0shut
Copy link

@n0shut n0shut commented Feb 18, 2023

Hi @robshakir @marcushines

I would like to propose a hermetic way to compile protos used in this repository which is based on the container approach.

Currently, the compile-protos.sh script has no pinned dependencies resulting in users defaulting to whatever latest/oldest version they have installed on their machines when updating the protos.

Not only it adds unnecessary churn to the PRs, it also makes the process overly complicated. The dev machine should have protos installed in the specified directories and we see that people struggle with that - #134.

In the newly added run.sh I introduced a container-based workflow where I leverage ghcr.io/srl-labs/protoc container that has pinned dependencies for protoc and the relevant grpc plugins - https://github.com/srl-labs/protoc-container/blob/main/Dockerfile

Using this container every user of gnmi repo will be guaranteed to compile the protos in one and only possible way, without the need to keep local dependencies and maintain their lifecycle.

At the time of this writing the latest version of protoc and plugins were used. I hope you will find that useful.

In #137 I generated the Go/Py bindings for you to verify how it works and potentially try it yourself to ensure that it works the same for everyone in a consistent way.

# to compile all protos
./run.sh compile-all

# to compile selected protos
./run.sh compile-go-gnmi-ext

use cmd variables

added newline
Copy link
Contributor

@robshakir robshakir left a comment

Choose a reason for hiding this comment

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

This looks great, thanks @n0shut. One quick question in the review.


DOCKER_CMD="docker run -v $(pwd):/in \
-v $(pwd)/proto:/in/github.com/openconfig/gnmi/proto \
ghcr.io/srl-labs/protoc:0.0.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

For my edification, how does this map to the protoc version that is in the container?

Copy link
Contributor

Choose a reason for hiding this comment

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

@robshakir it doesn't direclty map to the protoc version, as there are more than just protoc in the package (namely grpc plugins for both py and go version).
Therefore, the idea is that ghcr.io/srl-labs/protoc:0.0.3 is tagged in a semver fashion and the image tag matches the tag in the git repo for which the Dockerfile reveals the used versions: https://github.com/srl-labs/protoc-container/blob/v0.0.3/Dockerfile

Copy link
Contributor

Choose a reason for hiding this comment

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

ACK - no problem. I was just wondering whether it is possible to directly map these together so that it's clearer, but it's written into the pb.go as to what versions are used so I don't see this is a huge issue.

The only downside here is that we stay pinned to that 0.0.3 version until an explicit change is made. If there's a semver convention here - could we consider tagging something like 1.x.x-latest? (It'd be nice to stay close to HEAD for the generated files.)

Copy link
Author

@n0shut n0shut Feb 21, 2023

Choose a reason for hiding this comment

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

@robshakir good call, I see that the upstream changed the logic to include protoc__protoc-gen-go version in the image tag, so that it's clear which sw components versions are used.

https://github.com/srl-labs/protoc-container#versioning

I have therefore update the branch in 4a11050 to reflect that.

Now it is explicit which version of protoc and protoc-gen-go are used in the image.

Copy link
Author

Choose a reason for hiding this comment

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

I can also port it to gnoi repo where it is even more of a pressing issue.

compile-protos-docker.sh Outdated Show resolved Hide resolved
@n0shut
Copy link
Author

n0shut commented Apr 1, 2023

Hi @robshakir @wenovus
I have updated this PR by introducing ./run.sh script that embeds purpose-built functions for each lang/proto.
I have updated the initial PR comment to reflect that, but in essense what you can now do is:

# to compile all protos
./run.sh compile-all

# to compile selected protos
./run.sh compile-go-gnmi-ext

@n0shut n0shut requested a review from robshakir April 1, 2023 20:34
@hellt
Copy link
Contributor

hellt commented Jan 17, 2024

tsk tsk tsk
still using this user unfriendly compile_protos.sh that has no reproducibility and heavily dependent on hosts' configuration

@robshakir
Copy link
Contributor

Thanks for this -- let me look again this week. Unfortunately, I was on an extended leave during the period you previously requested review.

@hellt
Copy link
Contributor

hellt commented Jan 17, 2024

No worries @robshakir, I didn't mean to push, just a slight nudge. I can update it with the updated versions of protoc and go plugins

Copy link
Contributor

@robshakir robshakir left a comment

Choose a reason for hiding this comment

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

I'd propose to also leave compile_protos.sh present as well -- since there are some environments where security restrictions mean that docker cannot be run (e.g., locations that do not allow a virtual machine to be run).

set -o pipefail

# container image tag, to see all available tags go to
# https://github.com/srl-labs/protoc-container/pkgs/container/protoc
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this container something that you expect to continue to be updated?

Copy link
Author

Choose a reason for hiding this comment

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

yes, I just bumped it to a more recent version tuple 227be07

run.sh Outdated
function compile-go-gnmi {
${DOCKER_CMD} ash -c "${PROTOC_GO_CMD} proto/gnmi/gnmi.proto"
echo "finished Go compilation for gnmi.proto"

Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) is this newline needed (here and below)?

Copy link
Author

Choose a reason for hiding this comment

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

fixed, thanks

@robshakir
Copy link
Contributor

@marcushines - PTAL too.

@n0shut
Copy link
Author

n0shut commented Jan 20, 2024

compile_protos.sh brought back, @robshakir

@n0shut
Copy link
Author

n0shut commented May 7, 2024

closing as stale

@n0shut n0shut closed this May 7, 2024
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