Skip to content

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.

// 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

@TBBle TBBle mentioned this pull request Feb 22, 2022
2 tasks

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
@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

@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