Add vendoring to containerd master #390

Merged
merged 2 commits into from Jan 11, 2017

Projects

None yet

8 participants

@estesp
Member
estesp commented Dec 16, 2016 edited

Fixes #381

Initial vendor list validated with empty $GOPATH
and only master checked out; followed by make
and verified that all binaries build properly.
Updates require github.com/LK4D4/vndr tool.

Signed-off-by: Phil Estes estesp@linux.vnet.ibm.com

@runcom
Member
runcom commented Dec 16, 2016 edited
@crosbymichael
Member

LGTM

@estesp
Member
estesp commented Dec 16, 2016

Updated to handle Fedora btrfs build issue just solved in stevvooe/go-btrfs#1

@runcom
Member
runcom commented Dec 16, 2016

@estesp can you remove the make setup from the Makefile? I didn't want users to do that before realizing vendors are already in.

@stevvooe
Contributor

@runcom No, make setup just installs tools.

@stevvooe
Contributor

LGTM

@runcom
Member
runcom commented Dec 16, 2016

@runcom No, make setup just installs tools.

I must be blind, sorry for the noise..

@mlaventure
Contributor

The gen.go files need to be modified to have the correct path to gogoproto/gogo.proto or make generate will fails

@stevvooe
Contributor

@mlaventure Typically, we can just add something like -I /usr/local/include/ or some other common locations for protobuf paths.

@mlaventure
Contributor

I don't think there's currently a standard for .proto location.

Also, I'd rather take it from the vendoring since we're introducing it, it'll always be there and at the right version

@estesp
Member
estesp commented Dec 16, 2016 edited

Speaking of that @mlaventure after fixing that I find an interesting host-specific potential difficulty..my ubuntu 16.04 system has a /usr/bin/protoc that is obviously older than expected as it doesn't understand proto3 specification. I can easily solve this, but wonder if at some point we should wrap the build in a Dockerfile for those who don't have the ability, time, or patience to get the host system "just right"?

Edit: for future Ubuntu users interest, this file is provided by the package protobuf-compiler:

$ dpkg-query -S /usr/bin/protoc
protobuf-compiler: /usr/bin/protoc
@estesp
Member
estesp commented Dec 16, 2016

@crosbymichael @mlaventure please check my work on the protoc updates.. I assume because the imported protos are not exactly matching what someone else used, a file descriptor array was updated, but all actual generated code is the same.

@stevvooe
Contributor

@estesp Ideally, we want to avoid having the build rely on docker. Lose the garbage system packages (seriously, I don't know what they are thinking when they package these and make them so useless) and just get the package from https://github.com/google/protobuf/releases. The zip looks like this:

$ tar tvf protoc-3.1.0-linux-x86_64.zip
drwxrwxrwx  0 0      0           0 Sep 26 17:15 include/
drwxrwxrwx  0 0      0           0 Sep 26 17:15 include/google/
drwxrwxrwx  0 0      0           0 Sep 26 17:15 include/google/protobuf/
-rwxrwxrwx  0 0      0        4545 Sep 26 17:15 include/google/protobuf/timestamp.proto
-rwxrwxrwx  0 0      0        2263 Sep 26 17:15 include/google/protobuf/source_context.proto
-rwxrwxrwx  0 0      0        5236 Sep 26 17:15 include/google/protobuf/any.proto
-rwxrwxrwx  0 0      0        7077 Sep 26 17:15 include/google/protobuf/api.proto
-rwxrwxrwx  0 0      0       33431 Sep 26 17:15 include/google/protobuf/descriptor.proto
-rwxrwxrwx  0 0      0        3781 Sep 26 17:15 include/google/protobuf/struct.proto
-rwxrwxrwx  0 0      0        3745 Sep 26 17:15 include/google/protobuf/wrappers.proto
drwxrwxrwx  0 0      0           0 Sep 26 17:15 include/google/protobuf/compiler/
-rwxrwxrwx  0 0      0        7655 Sep 26 17:15 include/google/protobuf/compiler/plugin.proto
-rwxrwxrwx  0 0      0        2422 Sep 26 17:15 include/google/protobuf/empty.proto
-rwxrwxrwx  0 0      0        4230 Sep 26 17:15 include/google/protobuf/duration.proto
-rwxrwxrwx  0 0      0        7854 Sep 26 17:15 include/google/protobuf/field_mask.proto
-rwxrwxrwx  0 0      0        5788 Sep 26 17:15 include/google/protobuf/type.proto
drwxrwxrwx  0 0      0           0 Sep 26 17:15 bin/
-rwxrwxrwx  0 0      0     4114152 Sep 25 18:23 bin/protoc
-rwxrwxrwx  0 0      0         538 Sep 26 17:15 readme.txt

To install, do the following:

tar xvf protoc-3.1.0-linux-x86_64.zip -C /usr/local

You can use the brew package on Mac OS, because they don't exhibit the sloppy packaging of distro upstreams, but the same thing will work without brew if you get the darwin version.

api/execution/execution.proto
@@ -2,7 +2,7 @@ syntax = "proto3";
package containerd.v1;
-import "google/protobuf/empty.proto";
+import "ptypes/empty/empty.proto";
@stevvooe
stevvooe Dec 16, 2016 Contributor

This is not right. You need to use the canonical path or you break everything.

api/execution/gen.go
@@ -1,3 +1,3 @@
package execution
-//go:generate protoc -I.:../..:../../../../../github.com/gogo/protobuf:/usr/local/include --gogoctrd_out=plugins=grpc,import_path=github.com/docker/containerd/api/execution,Mgogoproto/gogo.proto=github.com/gogo/protobuf/gogoproto,Mgoogle/protobuf/descriptor.proto=github.com/gogo/protobuf/protoc-gen-gogo/descriptor:. execution.proto
+//go:generate protoc -I.:${PWD}/vendor/github.com/golang/protobuf:${PWD}/vendor/github.com/gogo/protobuf:${PWD}/vendor/github.com/gogo/protobuf/protobuf:/usr/local/include --gogoctrd_out=plugins=grpc,import_path=github.com/docker/containerd/api/execution,Mgogoproto/gogo.proto=github.com/gogo/protobuf/gogoproto,Mgoogle/protobuf/descriptor.proto=github.com/gogo/protobuf/protoc-gen-gogo/descriptor:. execution.proto
@stevvooe
stevvooe Dec 16, 2016 edited Contributor

Follow my directions on the other issue here and it will work.

@stevvooe
Contributor

Please, I know everyone is excited, but don't mess with the protobufs unless you know what you're doing. You'll probably just make things worse (speaking from experience).

@stevvooe
Contributor

-LGTM

@crosbymichael
Member

@stevvooe i think with the new github feature you could push to @estesp 's branch changes to fix the proto stuff since it's magic

@estesp
Member
estesp commented Dec 17, 2016

@stevvooe PR is fixed; no more weird changes to protobufs, and developer's will have to install from upstream protobuf release in /usr/local/include for now. I assume we could add a Makefile prep target that pulls it to a local directory, or commit the current release to a "project" directory if we don't want that burden on the developer.

@AkihiroSuda

How about mentioning the version of vndr and protoc in some doc?

vendor.conf
+github.com/beorn7/perks 4c0e84591b9aa9e6dcfdf3e020114cd81f89d5f9
+# matttproud/golang_protobuf_extensions; v1.0.0 release
+github.com/matttproud/golang_protobuf_extensions v1.0.0
+# go-units from Docker; latest release = 0.3.1
@AkihiroSuda
AkihiroSuda Dec 17, 2016 Contributor

nit: I think the notation of "latest" can quickly become outdated. Also, mentioning the version tag in the comment seems too verbose.

So, how about:

# go-units from Docker; latest release at 12/16/2016
@stevvooe
Contributor

I assume we could add a Makefile prep target that pulls it to a local directory, or commit the current release to a "project" directory if we don't want that burden on the developer.

I really don't mind burdening the developer here. We just need solid docs for getting protobuf installed.

api/execution/gen.go
@@ -1,3 +1,3 @@
package execution
-//go:generate protoc -I.:../..:../../../../../github.com/gogo/protobuf:/usr/local/include --gogoctrd_out=plugins=grpc,import_path=github.com/docker/containerd/api/execution,Mgogoproto/gogo.proto=github.com/gogo/protobuf/gogoproto,Mgoogle/protobuf/descriptor.proto=github.com/gogo/protobuf/protoc-gen-gogo/descriptor:. execution.proto
+//go:generate protoc -I.:${PWD}/vendor/github.com/gogo/protobuf:/usr/local/include --gogoctrd_out=plugins=grpc,import_path=github.com/docker/containerd/api/execution,Mgogoproto/gogo.proto=github.com/gogo/protobuf/gogoproto,Mgoogle/protobuf/descriptor.proto=github.com/gogo/protobuf/protoc-gen-gogo/descriptor:. execution.proto
@stevvooe
stevvooe Dec 20, 2016 Contributor

So, shell doesn't get evaluated in go:generate.

This needs to be

-I.:../../vendor:../../vendor/github.com/gogo/protobuf:../../../../../..:/usr/local/include

This creates the following:

  1. Local path import.
  2. Vendor path for project.
  3. gogo/protobuf as a top-level path space (i think this is right).
  4. The current GOPATH as a path space.
  5. /usr/local/include as a path space.

I know this is weird but it keeps the import paths for the individual protobuf files from becoming non-canonical when used as dependencies.

@estesp
estesp Dec 20, 2016 Member

@stevvooe just FYI, it does appear via the Makefile or with a bare go generate on the command line, ${PWD} is being evaluated in both cases. However, it may not matter that much as there will still be lots of ../ paths even with ${PWD} available. So I will update with the path set from above and hopefully that will get this PR ready to move forward with.

@AkihiroSuda AkihiroSuda referenced this pull request Dec 20, 2016
Merged

Add travis config #398

@estesp
Member
estesp commented Dec 20, 2016

@stevvooe updated with the new paths; PTAL

@estesp
Member
estesp commented Dec 20, 2016

Created #400 for simple quick-start, including the need to get the protoc release and install it in /usr/local/

@estesp
Member
estesp commented Jan 9, 2017

ping @stevvooe ; I know the fixups to the prior review happened late in the year, so just wanting to restart the discussion on this and get it in if we are all good with the updates.

@crosbymichael
Member

Yep, lets get vendoring figured out this week

vendor.conf
+# logrus, v0.11.0 latest release
+github.com/sirupsen/logrus v0.11.0
+# go-btrfs from stevvooe; following master HEAD on 12/16/2016
+github.com/stevvooe/go-btrfs a5487675780517f5c3e7dd697c70157466b8dcae
@samuelkarp
samuelkarp Jan 11, 2017 Contributor

Can you update go-btrfs to the current HEAD? stevvooe/go-btrfs#4 is now used in the tests.

@estesp
estesp Jan 11, 2017 Member

Sure, will update

@stevvooe
Contributor

LGTM after updating go-btrfs.

estesp added some commits Dec 16, 2016
@estesp estesp Add vendoring to containerd master
Initial vendor list validated with empty $GOPATH
and only master checked out; followed by `make`
and verified that all binaries build properly.
Updates require github.com/LK4D4/vndr tool.

Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com>
dd9309c
@estesp estesp Update protoc generation to use vendored protos
Use vendored-in protos in the project path rather
than expecting developers to have them in a local
path on the host. This made a generated change
in the FileDescriptor content, but everything else
matches and binaries are building properly.

Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com>
f867156
@estesp
Member
estesp commented Jan 11, 2017

Re-pushed with update to latest go-btrfs; also updated vendor.conf comment style based on @AkihiroSuda comment about duplicating version info in comment and vendor configuration data.

@samuelkarp

LGTM

@dqminh
Contributor
dqminh commented Jan 11, 2017

LGTM

@stevvooe stevvooe merged commit 23a644d into docker:master Jan 11, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
dco-signed All commits are signed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment