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

Lock versions for protobuf gen libraries. #3752

Closed
wants to merge 3 commits into from

Conversation

danielmai
Copy link
Contributor

@danielmai danielmai commented Aug 5, 2019

  • Clone golang/protobuf and gogo/protobuf libraries to latest released
    versions:
    • golang/protobuf: v1.3.2
    • gogo/protobuf: v1.2.1
  • Regenerate protos/pb/pb.pb.go.

This change is Reviewable

* Clone golang/protobuf and gogo/protobuf libraries to latest released
  versions:
  * golang/protobuf: v1.3.2
  * gogo/protobuf: v1.2.1
* Regenerate protos/pb/pb.pb.go
@danielmai danielmai requested review from manishrjain and a team as code owners August 5, 2019 19:01
Copy link
Contributor Author

@danielmai danielmai left a comment

Choose a reason for hiding this comment

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

Running make regenerate re-clones the specific version of golang/protobuf and gogo/protobuf in the GOPATH. It does not revert back to the previously cloned state if they already existed in the GOPATH before.

Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @manishrjain)

Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @danielmai and @manishrjain)


protos/depcheck.sh, line 7 at r1 (raw file):

readonly PROTOCMINVER="3.6.1"
readonly PROTOGOREQVER="v1.3.2"
readonly PROTOGOGOREQVER="v1.2.1"

What's the difference between these two constants. The names are pretty similar so it's a bit confusing.
It looks like the versions of go and gogo but a comment would be helpful.

protos/Makefile Outdated

.PHONY: golang_protobuf
golang_protobuf:
@echo "\033[0;31mRemoving $(GOPATH)/src/github.com/golang/protobuf\033[0m"
Copy link
Member

Choose a reason for hiding this comment

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

The color codes are unnecessary and do not make much difference to the output. Can avoid it.

@@ -26,16 +30,31 @@ help:
.PHONY: clean
clean:
@mkdir -p pb && rm -f pb/pb.pb.go
@rm -f $(GOBIN)/protoc-gen-go
Copy link
Member

Choose a reason for hiding this comment

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

regenerate is doing check clean golang_protobuf gogo_protobuf and clean is Phony. So every step will run every time which may be fine because we won't be running make regenerate too often. Could be problematic while I making changes to the proto file though.

@danielmai
Copy link
Contributor Author


protos/Makefile, line 42 at r1 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

The color codes are unnecessary and do not make much difference to the output. Can avoid it.

Done. Removed the colors.

@danielmai
Copy link
Contributor Author


protos/depcheck.sh, line 7 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

What's the difference between these two constants. The names are pretty similar so it's a bit confusing.
It looks like the versions of go and gogo but a comment would be helpful.

Done. Renamed the variables and added comments. Should be clearer now.

@danielmai
Copy link
Contributor Author


protos/Makefile, line 33 at r1 (raw file):

Previously, mangalaman93 (Aman Mangal) wrote…

regenerate is doing check clean golang_protobuf gogo_protobuf and clean is Phony. So every step will run every time which may be fine because we won't be running make regenerate too often. Could be problematic while I making changes to the proto file though.

Every directive in the Makefile is .PHONY. Is there an issue with removing the generated code and protoc-gen-* binaries when running make regenerate?

@mangalaman93 mangalaman93 requested a review from martinmr August 5, 2019 21:22
Copy link
Member

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @danielmai, @manishrjain, and @martinmr)


protos/Makefile, line 33 at r1 (raw file):

Previously, danielmai (Daniel Mai) wrote…

Every directive in the Makefile is .PHONY. Is there an issue with removing the generated code and protoc-gen-* binaries when running make regenerate?

It just make take time to run all the steps again and again.

@danielmai
Copy link
Contributor Author

Protobuf-tooling version locking is done by #5291. It installs the versions specified in go.mod. Closing this PR.

@danielmai danielmai closed this Oct 12, 2020
@danielmai danielmai deleted the danielmai/lock-proto-version branch October 12, 2020 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants