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

generated files: use "go install" to install binaries #2305

Merged
merged 1 commit into from
Aug 13, 2021

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Aug 13, 2021

Now that this repository moved to go1.16, we can use 'go install' to install these binaries, as mentioned in #2146

Before:

#11 [gobuild-base 4/6] RUN git clone https://github.com/gogo/protobuf.git /go/src/github.com/gogo/protobuf 	&& cd /go/src/github.com/gogo/protobuf 	&& git checkout -q v1.3.2 	&& go install ./protoc-gen-gogo ./protoc-gen-gogofaster ./protoc-gen-gogoslick
#11 0.061 Cloning into '/go/src/github.com/gogo/protobuf'...
#11 DONE 12.7s

#12 [gobuild-base 5/6] RUN git clone https://github.com/golang/protobuf.git /go/src/github.com/golang/protobuf 	&& cd /go/src/github.com/golang/protobuf 	&& git checkout -q v1.3.5 	&& go install ./protoc-gen-go
#12 0.093 Cloning into '/go/src/github.com/golang/protobuf'...
#12 DONE 2.7s

After:

#11 [gobuild-base 4/6] RUN --mount=target=/root/.cache,type=cache GO111MODULE=on go install 	github.com/gogo/protobuf/protoc-gen-gogo@v1.3.2 	github.com/gogo/protobuf/protoc-gen-gogofaster@v1.3.2 	github.com/gogo/protobuf/protoc-gen-gogoslick@v1.3.2
#11 0.200 go: downloading github.com/gogo/protobuf v1.3.2
#11 DONE 7.4s

#12 [gobuild-base 5/6] RUN --mount=target=/root/.cache,type=cache GO111MODULE=on go install 	github.com/golang/protobuf/protoc-gen-go@v1.3.5
#12 0.162 go: downloading github.com/golang/protobuf v1.3.5
#12 DONE 2.3s

@@ -5,6 +5,8 @@ FROM golang:1.16-buster AS gobuild-base
# https://github.com/golang/protobuf/blob/v1.3.5/.travis.yml#L15
ARG PROTOC_VERSION=3.11.4
ARG GOGO_VERSION=v1.3.2
ARG PROTOBUF_VERSION=v1.3.5
Copy link
Member

Choose a reason for hiding this comment

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

nit, but arg should be right before the command that use them. So instead move the protoc/gogo arg to the correct place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes, I can move these to where they're used (there's pros/cons for each; having them at the top allows to view all versions at a glance, so I though that was the intent). I saw the majority was defined at the top, so thought to move it there.

Let me update.

Now that this repository moved to go1.16, we can use 'go install' to install
these binaries.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the generated_go_install branch from c0b78ba to bd4b02d Compare August 13, 2021 15:55
@thaJeztah
Copy link
Member Author

Updated 👍 I also thought PROTOBUF_VERSION=v1.3.5 had to be updated (as previously it was kept in sync with the go.mod), but doing so failed the build;

#16 2.266 util/stack/generate.go
#16 2.271 protoc-gen-go: unable to determine Go import path for "stack.proto"
#16 2.271
#16 2.271 Please specify either:
#16 2.271 	• a "go_package" option in the .proto source file, or
#16 2.271 	• a "M" argument on the command line.
#16 2.271
#16 2.271 See https://developers.google.com/protocol-buffers/docs/reference/go-generated#package for more information.
#16 2.271
#16 2.272 --go_out: protoc-gen-go: Plugin failed with status code 1.
#16 2.273 util/stack/generate.go:3: running "protoc": exit status 1

Then I found cfb73e5, so looks like we can't / mustn't (currently) update. 😅

@tonistiigi tonistiigi merged commit e8a3bb3 into moby:master Aug 13, 2021
@thaJeztah thaJeztah deleted the generated_go_install branch August 13, 2021 17:01
@tonistiigi
Copy link
Member

Then I found cfb73e5, so looks like we can't / mustn't (currently) update

Yes, we need to look at that separately

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.

2 participants