Check that generated protocol buffer code is up to date #2217

euanh · 2018-07-03T15:05:12Z

make check will now regenerate the .pb.go files and fail if the results are different from what is checked into the repository.

euanh · 2018-07-03T15:06:34Z

@fcrisciani As discussed in #2193. I can hook this up to the main build target if you prefer.

fcrisciani · 2018-07-03T17:48:45Z

@euanh wondering if from a CI point of view, makes sense to separate the protobuf generation as a separate workflow and run it in parallel with the rest of them after the builder. what do you think?

fcrisciani · 2018-07-03T17:49:47Z

Makefile

 dockerbuildargs ?= --target dev - < Dockerfile
-dockerargs ?= --privileged -v $(shell pwd):/go/src/github.com/docker/libnetwork -w /go/src/github.com/docker/libnetwork
+dockerargs ?= --privileged -v $(shell pwd):/go/src/github.com/docker/libnetwork -w /go/src/github.com/docker/libnetwork ${dockerargs_extra}
+dockerargs_extra ?=


don't see this as being used, is it still needed?

Yes, as I said in the commit message it's there to make it easy to add extra arguments to the docker command line, for instance to bind mount an extra volume on a development machine.

fcrisciani · 2018-07-03T20:48:32Z

.circleci/config.yml

  working_directory: ~/go/src/github.com/docker/libnetwork
  docker:
-    - image: 'circleci/golang:1.10'
+    - image: 'circleci/golang:1.10.2'


this image is irrelevant, everything is built within a container, we can probably take something smaller also

Yes. It would be better not to have Go in the base image at all, so there is no chance that a new build job accidentally uses it.

The image we use comes from here: https://github.com/circleci-public/circleci-dockerfiles. There isn't a minimal image in that repo. We could try https://hub.docker.com/r/circleci/slim-base/ but I don't know if it's actively maintained. CircleCI requires a few utilities in the image https://circleci.com/docs/2.0/custom-images so we could make an Alpine-based image, but then we would have to maintain it.

euanh · 2018-07-04T13:02:59Z

@fcrisciani I can put it in a separate workflow if you prefer. However keeping it in make check could make it more likely that you will run it on your own machine before submitting.

fcrisciani · 2018-07-05T21:58:06Z

@euanh it's ok to keep it then in the check, and the rename of the workflow makes sense.
Something seems still to not work properly in the check

euanh · 2018-07-06T09:12:37Z

@fcrisciani Yes, I haven't pushed my fix for check yet. It's broken here because in CI we don't copy the .git directory into the libnetworkbuild container, so my approach of rebuilding the .pb.go files and then running git diff won't work. (We copy rather than bind mounting because we are using the docker executor, for which bind mounts are disallowed: https://discuss.circleci.com/t/why-circleci-2-0-does-not-support-mounting-folders/11605)

selansen · 2018-07-06T14:36:30Z

we specifically made that change for circle2.0 (coping the source) for the same reason you mentioned above. If you want to copy other files (.git), you can make changes in .dockerignore file and remove .git from the file. that will help you.

euanh · 2018-07-06T15:30:51Z

@selansen Yes, however adding .git to the build container means copying an ever-growing amount of extra data - currently 27MB. It would be better if we didn't have to do that.

fcrisciani · 2018-07-06T15:53:20Z

@euanh what if the CI will generate the protobuf in a tmp directory and then you do the diff from there?

euanh · 2018-07-06T16:07:55Z

@fcrisciani That's what I have. :) It's currently just implemented with a rather ugly rule which I'm tidying up.

fcrisciani · 2018-07-06T16:45:17Z

@euanh no git diff, but diff

euanh · 2018-07-06T16:50:22Z

@fcrisciani Yes, I meant that's what I have at the moment on my machine, but I haven't pushed it yet.

euanh · 2018-07-06T16:53:07Z

The top change overloads the .protoc -> .pb.go rule to have a 'check mode'. The command line is duplicated but that's easy to fix. Having an if statement in there at all is a bit ugly - I also had a version which built the .pb.go files to /tmp by overriding the GOGO_OUTDIR. After that I could just loop over them and compare.

fcrisciani · 2018-07-06T17:01:03Z

Makefile

+%.pb.go: %.proto
+	@if [ ${PROTOC_CHECK} ]; then \
+	protoc -I=. -I=/go/src -I=/go/src/github.com/gogo/protobuf -I=/go/src/github.com/gogo/protobuf/protobuf --gogo_out=/tmp $< ; \
+	diff -q $@ /tmp/$@ >/dev/null || (echo "👹 $@ is out of date; please run 'make protobuf' and check in updates" && exit 1) ; \


what if we pass the --gogo_out path directly as an ENV variable like we do for dockerbuildargs (https://github.com/docker/libnetwork/pull/2217/files#diff-1d37e48f9ceff6d8030570cd36286a61L9)
that way we don't need PROTOC_CHECK, locally you will always want to regenerate them eventually.

or you can also use the notion of the CIRCLE_CI env variable that is already passed and set at the top of the makefile the output path so no need to specify other env variables

I would prefer for make check to work in the same way whether you run it in CircleCI or on your own machine, so I can't use the CIRCLE_CI variable.

I also tried passing --gogo_out path as a variable, but then you still have to decide whether or not to run diff so it doesn't help much.

Signed-off-by: Euan Harris <euan.harris@docker.com>

Add documentation and move protobuf target into Build section Signed-off-by: Euan Harris <euan.harris@docker.com>

Outside the build container, run: make protobuf Inside the build container, run: make protobuf-local Signed-off-by: Euan Harris <euan.harris@docker.com>

'make check' will now fail if the files produced by re-running protoc differ from those which are checked into the repository. Signed-off-by: Euan Harris <euan.harris@docker.com>

fcrisciani

LGTM

fcrisciani reviewed Jul 3, 2018

View reviewed changes

euanh force-pushed the protobuf-check branch 2 times, most recently from c252f7f to b7413ec Compare July 4, 2018 12:54

euanh force-pushed the protobuf-check branch from b7413ec to 62f83b3 Compare July 6, 2018 16:51

fcrisciani reviewed Jul 6, 2018

View reviewed changes

euanh force-pushed the protobuf-check branch from 62f83b3 to 7f9ec8d Compare July 10, 2018 15:22

euanh added 4 commits July 10, 2018 17:01

circleci: Rename 'lint' to 'check' to match build target

f00ea14

Signed-off-by: Euan Harris <euan.harris@docker.com>

Makefile: Document and organize into sections

fcdd600

Add documentation and move protobuf target into Build section Signed-off-by: Euan Harris <euan.harris@docker.com>

Makefile: Add protobuf-local target, runnable within build container

30ce4d7

Outside the build container, run: make protobuf Inside the build container, run: make protobuf-local Signed-off-by: Euan Harris <euan.harris@docker.com>

Makefile: Add check for out of date protocol buffer code

b7cf819

'make check' will now fail if the files produced by re-running protoc differ from those which are checked into the repository. Signed-off-by: Euan Harris <euan.harris@docker.com>

euanh force-pushed the protobuf-check branch from 7f9ec8d to b7cf819 Compare July 10, 2018 16:02

fcrisciani approved these changes Jul 11, 2018

View reviewed changes

fcrisciani merged commit ccc3a85 into moby:master Jul 11, 2018

euanh deleted the protobuf-check branch July 23, 2018 14:51

thaJeztah mentioned this pull request Jun 9, 2019

[18.03 backport] backport CI, Makefile and Dockerfile fixes and improvements #2393

Merged

Check that generated protocol buffer code is up to date #2217

Check that generated protocol buffer code is up to date #2217

Uh oh!

Conversation

euanh commented Jul 3, 2018

Uh oh!

euanh commented Jul 3, 2018

Uh oh!

fcrisciani commented Jul 3, 2018

Uh oh!

fcrisciani Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

euanh Jul 4, 2018

Choose a reason for hiding this comment

Uh oh!

fcrisciani Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

euanh Jul 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

euanh commented Jul 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fcrisciani commented Jul 5, 2018

Uh oh!

euanh commented Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

selansen commented Jul 6, 2018

Uh oh!

euanh commented Jul 6, 2018

Uh oh!

fcrisciani commented Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

euanh commented Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fcrisciani commented Jul 6, 2018

Uh oh!

euanh commented Jul 6, 2018

Uh oh!

euanh commented Jul 6, 2018

Uh oh!

fcrisciani Jul 6, 2018

Choose a reason for hiding this comment

Uh oh!

fcrisciani Jul 6, 2018

Choose a reason for hiding this comment

Uh oh!

euanh Jul 10, 2018

Choose a reason for hiding this comment

Uh oh!

euanh Jul 10, 2018

Choose a reason for hiding this comment

Uh oh!

fcrisciani left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

euanh Jul 4, 2018 •

edited

Loading

euanh commented Jul 4, 2018 •

edited

Loading

euanh commented Jul 6, 2018 •

edited

Loading

fcrisciani commented Jul 6, 2018 •

edited

Loading

euanh commented Jul 6, 2018 •

edited

Loading