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

Run Protobuild on GitHub Actions #1302

Merged
merged 1 commit into from
Apr 5, 2022
Merged

Conversation

kzys
Copy link
Contributor

@kzys kzys commented Feb 22, 2022

containerd is planning to migrate off from github.com/gogo/protobuf which will affect hcsshim. See containerd/containerd#6564 for the overall progress.

Before that, this commit runs Protobuild in GitHub Actions to make sure all generated files are reproducible from .proto files.

Signed-off-by: Kazuyoshi Kato [email protected]

@ghost
Copy link

ghost commented Feb 22, 2022

CLA assistant check
All CLA requirements met.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@@ -26,7 +27,7 @@ var _ = time.Kitchen
// is compatible with the proto package it is being compiled against.
// A compilation error at this line likely means your copy of the
// proto package needs to be updated.
const _ = proto.GoGoProtoPackageIsVersion2 // please upgrade the proto package
const _ = proto.GoGoProtoPackageIsVersion3 // please upgrade the proto package
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kzys kzys force-pushed the gha-protobuild branch 13 times, most recently from e60508f to c59138c Compare February 22, 2022 05:55
@kzys
Copy link
Contributor Author

kzys commented Feb 22, 2022

This change will need containerd/protobuild#51. I will use Linux in this PR for now.

@kzys kzys force-pushed the gha-protobuild branch 3 times, most recently from 68fbf50 to 6dac14b Compare February 22, 2022 17:28
@kzys kzys marked this pull request as ready for review February 22, 2022 17:31
@kzys kzys requested a review from a team as a code owner February 22, 2022 17:31
@kzys
Copy link
Contributor Author

kzys commented Feb 22, 2022

The tests are all green in my fork. https://github.com/kzys/hcsshim/actions/runs/1883043415

@TBBle TBBle mentioned this pull request Feb 22, 2022
2 tasks
@@ -8,6 +8,50 @@ env:
GOPROXY: off

jobs:
proto:
# TODO: use Windows after merginig https://github.com/containerd/protobuild/pull/51.
Copy link
Contributor Author

@kzys kzys Feb 23, 2022

Choose a reason for hiding this comment

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

@TBBle Should I use Windows for consistency?

Copy link
Contributor

Choose a reason for hiding this comment

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

I honestly don't know if it makes a difference. In fact, I hope it doesn't matter, as that would suggest something else is wrong, if we get different results from this based on platform.

When doing this by hand I did this on Windows myself, but the relevant procedure should be roughly the same, so maybe doing this validation on Linux is an extra level of prevention if a platform-specific difference does sneak in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#51 got merged! Let me update this PR. I agree that generating Go files from proto files is platform-independent. Running Protobuild on different platforms is something Protobuild has to do. So hcsshim doesn't have to do that.

Comment on lines 25 to 29
git clone --depth 1 https://github.com/containerd/containerd.git -b v1.6.0
cd containerd/cmd/protoc-gen-gogoctrd
go build
mkdir $GOPATH/bin
mv protoc-gen-gogoctrd $GOPATH/bin
Copy link
Contributor

@TBBle TBBle Feb 27, 2022

Choose a reason for hiding this comment

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

Is the inability to use

go install github.com/containerd/containerd/cmd/[email protected]

a containerd bug?

I tried this out of curiosity, and it failed with

> go install github.com/containerd/containerd/cmd/[email protected]
go install: github.com/containerd/containerd/cmd/[email protected] (in github.com/containerd/[email protected]):
        The go.mod file for the module providing named packages contains one or
        more replace directives. It must not contain directives that would cause
        it to be interpreted differently than if it were the main module.

but that might be me doing something wrong.

Copy link
Contributor Author

@kzys kzys Feb 28, 2022

Choose a reason for hiding this comment

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

It is sadly "working as expected". golang/go#44840

The good news is that I will remove protoc-gen-gogoctrd soon from the build process, once we remove gogo/protobuf.

@kzys kzys force-pushed the gha-protobuild branch 2 times, most recently from c4a53a2 to b576476 Compare February 28, 2022 17:34
@kzys kzys force-pushed the gha-protobuild branch 13 times, most recently from 430008a to 603dd84 Compare February 28, 2022 19:18
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@dcantah
Copy link
Contributor

dcantah commented Mar 16, 2022

Needs a quick rebase

@dcantah
Copy link
Contributor

dcantah commented Apr 5, 2022

@kzys Ping on the rebase (we added a new shim option here so the proto needs to be regenerated). Thanks!

@kzys kzys force-pushed the gha-protobuild branch 5 times, most recently from cf7c2cc to fa1205f Compare April 5, 2022 19:13
containerd is planning to migrate off from github.com/gogo/protobuf
which will affect hcsshim.
See containerd/containerd#6564 for
the overall progress.

Before that, this commit runs Protobuild in GitHub Actions to
make sure all generated files are reproducible from .proto files.

Signed-off-by: Kazuyoshi Kato <[email protected]>
@kzys kzys force-pushed the gha-protobuild branch from fa1205f to 290c85e Compare April 5, 2022 19:21
@kzys
Copy link
Contributor Author

kzys commented Apr 5, 2022

@dcantah Thanks. Rebased the PR.

Copy link
Contributor

@katiewasnothere katiewasnothere left a comment

Choose a reason for hiding this comment

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

lgtm

@dcantah dcantah merged commit 6b31cba into microsoft:master Apr 5, 2022
anmaxvl pushed a commit that referenced this pull request Feb 7, 2023
princepereira pushed a commit to princepereira/hcsshim that referenced this pull request Aug 29, 2024
containerd is planning to migrate off from github.com/gogo/protobuf
which will affect hcsshim.
See containerd/containerd#6564 for
the overall progress.

Before that, this commit runs Protobuild in GitHub Actions to
make sure all generated files are reproducible from .proto files.

Signed-off-by: Kazuyoshi Kato <[email protected]>
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.

5 participants