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

Vendor protobuf tools #1592

Closed
wants to merge 2 commits into from
Closed

Conversation

helsaawy
Copy link
Contributor

@helsaawy helsaawy commented Dec 1, 2022

Add protobuild and protoc-gen-gogoctrd to tools.go so they are tracked and vendored.
This way, protobufs are always generated with the same version, and protoc-gen-gogoctrd can be installed directly via go install, instead of needing to pull containerd/containerd and install from there.

Simplify GitHub CI protobuf job.

Add empty after section to Protobuild.toml to suppress errors about missing directories usr/local/include and /usr/include

Remove unneeded gogoproto/gogo.proto in protobuf files, which also raises protobuild warnings.

Add protobuf file to gitignore so it does not affect git diff results.

Signed-off-by: Hamza El-Saawy hamzaelsaawy@microsoft.com

@helsaawy helsaawy requested a review from a team as a code owner December 1, 2022 23:37
@helsaawy helsaawy force-pushed the proto-vendor-tools branch 2 times, most recently from bd7a007 to 25d038b Compare December 2, 2022 20:29
@helsaawy
Copy link
Contributor Author

helsaawy commented Dec 9, 2022

rebased to fix merge conflict

Add `protobuild` and `protoc-gen-gogoctrd` to `tools.go` so they are
tracked and vendored.
This way, protobufs are always generated with the same version, and
`protoc-gen-gogoctrd` can be installed directly via `go install`.

Simplify GitHub CI protobuf job.

Add empty `after` section to Protobuild to suppress errors about missing
directories `usr/local/include` and `/usr/include`

Add protobuf file to gitignore so it does not affect `git diff` results.

Remove unneeded `gogoproto/gogo.proto` in protobuf files.

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
@@ -37,6 +37,9 @@ rootfs-conv/*
deps/*
out/*

# protobuf files
/protobuf/*
Copy link
Contributor

Choose a reason for hiding this comment

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

should we also have "protobuf/*" without the root slash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

iirc, i ran into an issue where a vendored module had a folder called protobuf, and it caused all kinds of shenanigans 😅

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
@helsaawy helsaawy marked this pull request as draft April 5, 2023 18:19
@helsaawy
Copy link
Contributor Author

obe by #1706

@helsaawy helsaawy closed this Jul 19, 2023
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