Skip to content

Move .proto file and add caching of protoc and protoc-gen-go during build#46

Merged
bwplotka merged 1 commit intoprometheus:masterfrom
bufdev:update
Jun 7, 2021
Merged

Move .proto file and add caching of protoc and protoc-gen-go during build#46
bwplotka merged 1 commit intoprometheus:masterfrom
bufdev:update

Conversation

@bufdev
Copy link
Copy Markdown
Contributor

@bufdev bufdev commented Jun 7, 2021

This does a lot of stuff, but primarily deals with the #44 and #45 problems.

This does change the outputted go package - let me know if this is not OK.

This also makes sure that a consistent version of protoc and protoc-gen-go are used. This repository is using a very old version of both, but we want to be consistent. I would recommend moving to buf instead, and upgrading to a newer version of protoc-gen-go, and can help with that if you want.

@bufdev
Copy link
Copy Markdown
Contributor Author

bufdev commented Jun 7, 2021

If we had the pen here, there's some other clean ups I'd do to metrics.proto as well, but this PR is just about moving stuff.

We can also make it so that the path to the go package doesn't change, let me know if that's a requirement.

@beorn7
Copy link
Copy Markdown
Member

beorn7 commented Jun 7, 2021

That looks very cool.

We can also make it so that the path to the go package doesn't change, let me know if that's a requirement.

Yes, I think that's very important. I think we need to ensure that nothing changes from the Go side. And AFAICS, in this PR only the path to the Go package is a relevant change.

@beorn7
Copy link
Copy Markdown
Member

beorn7 commented Jun 7, 2021

@newhook could you verify that everything looks still good from your side with this PR in? (Modulo not changing the path to the Go package.)

@bufdev
Copy link
Copy Markdown
Contributor Author

bufdev commented Jun 7, 2021

Updated.

The diff is because you have a bug in this repository currently - your go.mod specifies v1.2.0 for github.com/golang/protobuf, but this does not match with the file you had generated (which I can tell based on the outputted file and years of experience) - I'm going to match the output file, and then update your go.mod as your minumum version requirement for golang/protobuf is a bug.

@bufdev
Copy link
Copy Markdown
Contributor Author

bufdev commented Jun 7, 2021

I fixed your go.mod and go.sum, and matched the minimum version as v1.3.5, which is what you were generating. This is correct now (the diffs on metrics.pb.go are now as expected).

@bufdev
Copy link
Copy Markdown
Contributor Author

bufdev commented Jun 7, 2021

I also added a simple go build ./go target to the Makefile, as a sanity check when you rebuild.

In a follow-up, I can move you to buf which will make this a lot easier.

Copy link
Copy Markdown
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

Looks good to me (modulo spelling nits, see comments). Thank you very much @bufdev .

@bwplotka @kakkoyun I think this should just work perfectly from the Go perspective. Could you have a quick look and confirm?

…uild

Signed-off-by: bufdev <bufdev-github@buf.build>
PROTOC := tmp/versions/protoc/$(PROTOC_VERSION)
PROTOC_BIN := tmp/bin/protoc
PROTOC_INCLUDE := tmp/include/google
$(PROTOC):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While we are here.. can we just use... Buf? (:

https://github.com/openproto/protoconfig/blob/main/Makefile

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As commented here, we'll do it in a follow-up

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okey, thanks!

Copy link
Copy Markdown
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Thanks!

I have a proposition, can we use bingo and buf and 3 lines of Makefile instead of 81 lines? 🙈

Otherwise, it makes sense.

# Need to be on a previous version that doesn't cause the updated WKT go_package values to be added.
PROTOC_VERSION := 3.13.0
# This has been around for a while.
PROTOC_GEN_GO_VERSION := v1.3.5
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In theory we could use bingo too, to avoid so much things in Makefile to maintain: https://github.com/openproto/protoconfig/blob/main/go/Makefile#L52

@bwplotka bwplotka merged commit 147c58e into prometheus:master Jun 7, 2021
@bwplotka
Copy link
Copy Markdown
Member

bwplotka commented Jun 7, 2021

I have a proposition, can we use bingo and buf and 3 lines of Makefile instead of 81 lines? see_no_evil

We can do that in follow ups

@beorn7
Copy link
Copy Markdown
Member

beorn7 commented Jun 7, 2021

Yeah, let's not do too much in one (bin)go…

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