*: bump gRPC dependencies #5075

simonpasquier · 2019-01-07T16:33:10Z

This change updates the gRPC dependencies to more recent versions:

github.com/gogo/protobuf => v1.2.0
github.com/grpc-ecosystem/grpc-gateway => v1.6.3
google.golang.org/grpc => v1.17.0

In addition scripts/genproto.sh leverages Go modules information instead of
hardcoding SHA1 commits. This ensures that the code is generated from
the exact same sources.

cc @cstyan

This change updates the gRPC dependencies to more recent versions: * github.com/gogo/protobuf => v1.2.0 * github.com/grpc-ecosystem/grpc-gateway => v1.6.3 * google.golang.org/grpc => v1.17.0 In addition scripts/genproto.sh leverages Go modules information instead of hardcoding SHA1 commits. This ensures that the code is generated from the exact same sources. Signed-off-by: Simon Pasquier <spasquie@redhat.com>

tomwilkie · 2019-01-07T18:30:40Z

Doesn't work for me :-(

$ bash -x ./scripts/genproto.sh 
+ set -e
+ set -u
+ [[ ./scripts/genproto.sh =~ scripts/genproto\.sh ]]
++ protoc --version
+ [[ libprotoc 3.5.1 =~ 3\.5\.1 ]]
+ echo 'installing plugins'
installing plugins
+ INSTALL_PKGS='golang.org/x/tools/cmd/goimports github.com/gogo/protobuf/protoc-gen-gogofast github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger'
+ for pkg in '${INSTALL_PKGS}'
+ GO111MODULE=on
+ go install -mod=vendor golang.org/x/tools/cmd/goimports
+ for pkg in '${INSTALL_PKGS}'
+ GO111MODULE=on
+ go install -mod=vendor github.com/gogo/protobuf/protoc-gen-gogofast
+ for pkg in '${INSTALL_PKGS}'
+ GO111MODULE=on
+ go install -mod=vendor github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway
+ for pkg in '${INSTALL_PKGS}'
+ GO111MODULE=on
+ go install -mod=vendor github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger
+ PROM_ROOT=/Users/twilkie/Documents/src/github.com/prometheus/prometheus
+ PROM_PATH=/Users/twilkie/Documents/src/github.com/prometheus/prometheus/prompb
++ GO111MODULE=on
++ go list -f '{{ .Dir }}' -m github.com/gogo/protobuf
+ GOGOPROTO_ROOT=
+ GOGOPROTO_PATH=:/protobuf
++ GO111MODULE=on
++ go list -f '{{ .Dir }}' -m github.com/grpc-ecosystem/grpc-gateway
+ GRPC_GATEWAY_ROOT=
+ DIRS=prompb
+ echo 'generating code'
generating code
+ for dir in '${DIRS}'
+ pushd prompb
~/Documents/src/github.com/prometheus/prometheus/prompb ~/Documents/src/github.com/prometheus/prometheus
+ protoc --gogofast_out=plugins=grpc:. -I=. -I=:/protobuf -I=/Users/twilkie/Documents/src/github.com/prometheus/prometheus/prompb -I=/third_party/googleapis remote.proto rpc.proto types.proto
--proto_path passed empty directory name.  (Use "." for current directory.)

Will have a look at why...

scripts/genproto.sh

tomwilkie · 2019-01-07T18:38:12Z

It seems go list doesn't provide a way to use -mod=vendor...

tomwilkie · 2019-01-07T18:47:35Z

No, looks like for some reason the github.com/gogo/protobuf and github.com/grpc-ecosystem/grpc-gateway modules aren't being fetched...

tomwilkie · 2019-01-07T18:50:43Z

Yes, it appears we need to go get them, but that as long as we don't do a -u this should fetch the pinned versions.

tomwilkie · 2019-01-07T18:56:06Z

This does the trick:

diff --git a/scripts/genproto.sh b/scripts/genproto.sh
index 2c2b75bd..bec0a53c 100755
--- a/scripts/genproto.sh
+++ b/scripts/genproto.sh
@@ -16,11 +16,14 @@ if ! [[ $(protoc --version) =~ "3.5.1" ]]; then
 fi
 
 echo "installing plugins"
-INSTALL_PKGS="golang.org/x/tools/cmd/goimports github.com/gogo/protobuf/protoc-gen-gogofast github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger"
-for pkg in ${INSTALL_PKGS}; do
+for pkg in golang.org/x/tools/cmd/goimports github.com/gogo/protobuf/protoc-gen-gogofast; do
     GO111MODULE=on go install -mod=vendor "$pkg"
 done
 
+for pkg in github.com/gogo/protobuf github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger; do
+    GO111MODULE=on go get "$pkg"
+done
+
 PROM_ROOT="${PWD}"
 PROM_PATH="${PROM_ROOT}/prompb"
 GOGOPROTO_ROOT="$(GO111MODULE=on go list -f '{{ .Dir }}' -m github.com/gogo/protobuf)"

Although I need to test that this doesn't try and update gomodules again..

scripts/genproto.sh

tomwilkie

Need to include the go get (if that does what I think) and remove the -- for this to work on a mac.

cstyan · 2019-01-08T02:22:29Z

Works for me with @tomwilkie's changes.

Without that diff I got different for what looks like the same issue, missing dependencies.

prometheus branch  update-genproto-script [$?] at ☸ops-tools1-gke-us-east4 ➜ make proto
>> generating code from proto files
installing plugins
go: finding github.com/ghodss/yaml v1.0.0
go: finding github.com/grpc-ecosystem/grpc-gateway v1.6.3
go: finding github.com/prometheus/tsdb v0.3.2-0.20181219094047-6d489a1004dc
go: finding github.com/evanphx/json-patch v4.1.0+incompatible
go: finding github.com/simonpasquier/klog-gokit v0.1.0
go: finding google.golang.org/grpc v1.17.0
go: finding k8s.io/apimachinery v0.0.0-20181127025237-2b1284ed4c93
go: finding k8s.io/client-go v2.0.0-alpha.0.0.20181121191925-a47917edff34+incompatible
go: finding k8s.io/api v0.0.0-20181213150558-05914d821849
go: finding sigs.k8s.io/yaml v1.1.0
go: finding github.com/go-logfmt/logfmt v0.4.0
go: finding golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3
generating code
~/go/src/github.com/prometheus/prometheus/prompb ~/go/src/github.com/prometheus/prometheus
/third_party/googleapis: warning: directory does not exist.
google/api/annotations.proto: File not found.
rpc.proto: Import "google/api/annotations.proto" was not found or had errors.
make: *** [Makefile.common:207: proto] Error 1

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

brancz · 2019-01-08T09:48:19Z

cc @bwplotka

simonpasquier · 2019-01-08T10:46:19Z

@cstyan @tomwilkie please test again on Mac. I've added a step to CircleCI that runs make proto and bails out if it fails or the generated code has changed.

bwplotka

Generally LGTM, just one question and one suggestion.

Also I guess it is preferable/nicer to update dependencies in separate PR to actuall change in script.. but up to you.

bwplotka · 2019-01-08T10:34:40Z

.circleci/config.yml

+         chmod +x /tmp/bin/protoc
+         echo 'export PATH=/tmp/bin:$PATH' >> $BASH_ENV
+         source $BASH_ENV
+         make proto
    - run: git diff --exit-code


I assume this is to detected if assets, lint and now proto generation was done and commited, right? Maybe worth to wrap this with some message, like:

Suggested change

- run: git diff --exit-code

- run: git diff --exit-code || echo "\n Not linted code, assets not updated or proto not generated.

It would be nice what exactly is not generated as well.. ;p But this will work for a start (:

scripts/genproto.sh

bwplotka · 2019-01-08T10:52:33Z

Yes, it appears we need to go get them, but that as long as we don't do a -u this should fetch the pinned versions.

@tomwilkie let's test this, because according to docs go get (even without -u) fetches latest semver version.

go get foo updates to the latest version with a semver tag. (This is equivalent to go get foo@latest — in other words, @latest is the default if no @ version is specified).

(ref: https://github.com/golang/go/wiki/Modules#how-to-upgrade-and-downgrade-dependencies)

... But maybe docs are misleading and they meant -u there!

bwplotka · 2019-01-08T10:54:26Z

No, looks like for some reason the github.com/gogo/protobuf and github.com/grpc-ecosystem/grpc-gateway modules aren't being fetched...

Any ideas why they are not already in vendor though?

tomwilkie · 2019-01-08T11:29:49Z

Any ideas why they are not already in vendor though?

They are, but we need the proto files, and they are not.

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

cstyan · 2019-01-08T17:32:17Z

@simonpasquier getting a checksum mismatch, that rootcert package dependency is indirect so maybe downstream someone updated their version?

prometheus branch  update-genproto-script [$?]  ➜ GO111MODULE=on go mod download
go: verifying github.com/hashicorp/go-rootcerts@v0.0.0-20160503143440-6bb64b370b90: checksum mismatch
        downloaded: h1:9HVkPxOpo+yO93Ah4yrO67d/qh0fbLLWbKqhYjyHq9A=
        go.sum:     h1:VBj0QYQ0u2MCJzBfeYXGexnAl17GsH1yidnoxCqqD9E=

@cstyan @tomwilkie please test again on Mac.

Just Tom is on macos.

simonpasquier · 2019-01-09T08:52:00Z

@cstyan this is either because you're not using Go 1.11.4 or because you need to clean you module cache (go clean -modcache). See golang/go#29278 (comment) for the gory details (Go modules still have some rough edges...).

simonpasquier · 2019-01-10T15:11:10Z

@cstyan @tomwilkie let me know if it works for you now.

cstyan · 2019-01-10T15:37:46Z

@simonpasquier yeah works for me now

tomwilkie · 2019-01-15T13:04:28Z

I had to do a go clean -modcache, and I keep getting checksum mismatch errors on the go mod download, but I don't want to hold up the change due to what looks like go bugs. LGTM

simonpasquier · 2019-01-15T14:31:49Z

@tomwilkie are you building with Go 1.11.4? Previous 1.11.x versions can compute different hashes for projects that have symlinks.

tomwilkie · 2019-01-15T14:52:54Z

@tomwilkie are you building with Go 1.11.4? Previous 1.11.x versions can compute different hashes for projects that have symlinks.

go 1.11.2; after the update it works nicely.

simonpasquier mentioned this pull request Jan 7, 2019

update remote write path proto so that Labels/Timeseries can't be nil #4957

Merged

tomwilkie reviewed Jan 7, 2019

View reviewed changes

scripts/genproto.sh Outdated Show resolved Hide resolved

tomwilkie reviewed Jan 7, 2019

View reviewed changes

scripts/genproto.sh Outdated Show resolved Hide resolved

tomwilkie requested changes Jan 7, 2019

View reviewed changes

simonpasquier added 4 commits January 8, 2019 09:42

Run 'make proto' in CI

fe6e904

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

Revert tabs -> spaces change

2b4bcfe

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

Fix 'make proto' step

b49c34b

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

'go get' grpc/protobuf dependencies

8e98ae2

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

bwplotka reviewed Jan 8, 2019

View reviewed changes

Prepopulate cache with go mod download

10583da

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

simonpasquier mentioned this pull request Jan 10, 2019

Inconsistency in generation of .proto files prometheus/alertmanager#1702

Closed

mxinden mentioned this pull request Jan 14, 2019

Added version locking for protobuf extensions in genproto.sh prometheus/alertmanager#1707

Merged

tomwilkie approved these changes Jan 15, 2019

View reviewed changes

simonpasquier merged commit 375ad11 into prometheus:master Jan 15, 2019

simonpasquier deleted the update-genproto-script branch January 15, 2019 14:32

pmalek-sumo mentioned this pull request Feb 3, 2021

Bump prometheus dependency to v2.21.0 influxdata/telegraf#8795

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: bump gRPC dependencies #5075

*: bump gRPC dependencies #5075

simonpasquier commented Jan 7, 2019

tomwilkie commented Jan 7, 2019

tomwilkie commented Jan 7, 2019

tomwilkie commented Jan 7, 2019

tomwilkie commented Jan 7, 2019

tomwilkie commented Jan 7, 2019

tomwilkie left a comment

cstyan commented Jan 8, 2019

brancz commented Jan 8, 2019

simonpasquier commented Jan 8, 2019

bwplotka left a comment

bwplotka Jan 8, 2019

bwplotka commented Jan 8, 2019 •

edited

Loading

bwplotka commented Jan 8, 2019

tomwilkie commented Jan 8, 2019

cstyan commented Jan 8, 2019

simonpasquier commented Jan 9, 2019

simonpasquier commented Jan 10, 2019

cstyan commented Jan 10, 2019

tomwilkie commented Jan 15, 2019

simonpasquier commented Jan 15, 2019

tomwilkie commented Jan 15, 2019

	- run: git diff --exit-code
	- run: git diff --exit-code \|\| echo "\n Not linted code, assets not updated or proto not generated.

*: bump gRPC dependencies #5075

*: bump gRPC dependencies #5075

Conversation

simonpasquier commented Jan 7, 2019

tomwilkie commented Jan 7, 2019

tomwilkie commented Jan 7, 2019

tomwilkie commented Jan 7, 2019

tomwilkie commented Jan 7, 2019

tomwilkie commented Jan 7, 2019

tomwilkie left a comment

Choose a reason for hiding this comment

cstyan commented Jan 8, 2019

brancz commented Jan 8, 2019

simonpasquier commented Jan 8, 2019

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka Jan 8, 2019

Choose a reason for hiding this comment

bwplotka commented Jan 8, 2019 • edited Loading

bwplotka commented Jan 8, 2019

tomwilkie commented Jan 8, 2019

cstyan commented Jan 8, 2019

simonpasquier commented Jan 9, 2019

simonpasquier commented Jan 10, 2019

cstyan commented Jan 10, 2019

tomwilkie commented Jan 15, 2019

simonpasquier commented Jan 15, 2019

tomwilkie commented Jan 15, 2019

bwplotka commented Jan 8, 2019 •

edited

Loading