Add vendoring to containerd master #390

estesp · 2016-12-16T17:06:28Z

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 · 2016-12-16T17:08:07Z

This must be removed also https://github.com/docker/containerd/blob/master/Makefile#L37

crosbymichael · 2016-12-16T17:27:23Z

LGTM

estesp · 2016-12-16T17:43:58Z

Updated to handle Fedora btrfs build issue just solved in containerd/btrfs#1

runcom · 2016-12-16T17:48:39Z

@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 · 2016-12-16T18:09:04Z

@runcom No, make setup just installs tools.

stevvooe · 2016-12-16T18:09:07Z

LGTM

runcom · 2016-12-16T18:09:45Z

@runcom No, make setup just installs tools.

I must be blind, sorry for the noise..

mlaventure · 2016-12-16T19:27:43Z

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

stevvooe · 2016-12-16T19:35:21Z

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

mlaventure · 2016-12-16T19:38:11Z

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 · 2016-12-16T19:40:28Z

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 · 2016-12-16T20:24:38Z

@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 · 2016-12-16T20:54:03Z

@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.

stevvooe · 2016-12-16T20:54:59Z

api/execution/execution.proto

@@ -2,7 +2,7 @@ syntax = "proto3";

 package containerd.v1;

-import "google/protobuf/empty.proto";
+import "ptypes/empty/empty.proto";


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

stevvooe · 2016-12-16T20:55:39Z

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


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

stevvooe · 2016-12-16T20:56:24Z

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 · 2016-12-16T20:56:57Z

-LGTM

crosbymichael · 2016-12-16T21:38:38Z

@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 · 2016-12-17T02:07:46Z

@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?

AkihiroSuda · 2016-12-17T15:40:24Z

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


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 · 2016-12-20T00:55:02Z

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.

stevvooe · 2016-12-20T00:58:52Z

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


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:

Local path import.

Vendor path for project.

gogo/protobuf as a top-level path space (i think this is right).

The current GOPATH as a path space.

/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.

@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.

estesp · 2016-12-20T04:08:20Z

@stevvooe updated with the new paths; PTAL

estesp · 2016-12-20T05:10:19Z

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

estesp · 2017-01-09T19:59:20Z

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 · 2017-01-09T23:30:48Z

Yep, lets get vendoring figured out this week

samuelkarp · 2017-01-11T21:40:33Z

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


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

Sure, will update

stevvooe · 2017-01-11T21:56:25Z

LGTM after updating go-btrfs.

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>

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>

estesp · 2017-01-11T22:01:16Z

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 · 2017-01-11T22:11:14Z

LGTM

estesp force-pushed the init-vendoring branch from 67fb15b to 2b4b7cf Compare December 16, 2016 17:43

stevvooe reviewed Dec 16, 2016

View reviewed changes

estesp force-pushed the init-vendoring branch from c525de3 to 4a63706 Compare December 17, 2016 02:03

AkihiroSuda reviewed Dec 17, 2016

View reviewed changes

stevvooe reviewed Dec 20, 2016

View reviewed changes

AkihiroSuda mentioned this pull request Dec 20, 2016

Add travis config #398

Merged

estesp force-pushed the init-vendoring branch from 4a63706 to 10512a6 Compare December 20, 2016 04:07

samuelkarp reviewed Jan 11, 2017

View reviewed changes

estesp added 2 commits January 11, 2017 16:59

Add vendoring to containerd master

dd9309c

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>

estesp force-pushed the init-vendoring branch from 10512a6 to f867156 Compare January 11, 2017 21:59

samuelkarp approved these changes Jan 11, 2017

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vendoring to containerd master #390

Add vendoring to containerd master #390

estesp commented Dec 16, 2016 •

edited

Loading

runcom commented Dec 16, 2016 •

edited

Loading

crosbymichael commented Dec 16, 2016

estesp commented Dec 16, 2016

runcom commented Dec 16, 2016

stevvooe commented Dec 16, 2016

stevvooe commented Dec 16, 2016

runcom commented Dec 16, 2016

mlaventure commented Dec 16, 2016

stevvooe commented Dec 16, 2016

mlaventure commented Dec 16, 2016

estesp commented Dec 16, 2016 •

edited

Loading

estesp commented Dec 16, 2016

stevvooe commented Dec 16, 2016

stevvooe Dec 16, 2016

stevvooe Dec 16, 2016 •

edited

Loading

stevvooe commented Dec 16, 2016

stevvooe commented Dec 16, 2016

crosbymichael commented Dec 16, 2016

estesp commented Dec 17, 2016

AkihiroSuda left a comment

AkihiroSuda Dec 17, 2016

stevvooe commented Dec 20, 2016

stevvooe Dec 20, 2016

estesp Dec 20, 2016

estesp commented Dec 20, 2016

estesp commented Dec 20, 2016

estesp commented Jan 9, 2017

crosbymichael commented Jan 9, 2017

samuelkarp Jan 11, 2017

estesp Jan 11, 2017

stevvooe commented Jan 11, 2017

estesp commented Jan 11, 2017

samuelkarp left a comment

dqminh commented Jan 11, 2017

Add vendoring to containerd master #390

Add vendoring to containerd master #390

Conversation

estesp commented Dec 16, 2016 • edited Loading

runcom commented Dec 16, 2016 • edited Loading

crosbymichael commented Dec 16, 2016

estesp commented Dec 16, 2016

runcom commented Dec 16, 2016

stevvooe commented Dec 16, 2016

stevvooe commented Dec 16, 2016

runcom commented Dec 16, 2016

mlaventure commented Dec 16, 2016

stevvooe commented Dec 16, 2016

mlaventure commented Dec 16, 2016

estesp commented Dec 16, 2016 • edited Loading

estesp commented Dec 16, 2016

stevvooe commented Dec 16, 2016

stevvooe Dec 16, 2016

Choose a reason for hiding this comment

stevvooe Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

stevvooe commented Dec 16, 2016

stevvooe commented Dec 16, 2016

crosbymichael commented Dec 16, 2016

estesp commented Dec 17, 2016

AkihiroSuda left a comment

Choose a reason for hiding this comment

AkihiroSuda Dec 17, 2016

Choose a reason for hiding this comment

stevvooe commented Dec 20, 2016

stevvooe Dec 20, 2016

Choose a reason for hiding this comment

estesp Dec 20, 2016

Choose a reason for hiding this comment

estesp commented Dec 20, 2016

estesp commented Dec 20, 2016

estesp commented Jan 9, 2017

crosbymichael commented Jan 9, 2017

samuelkarp Jan 11, 2017

Choose a reason for hiding this comment

estesp Jan 11, 2017

Choose a reason for hiding this comment

stevvooe commented Jan 11, 2017

estesp commented Jan 11, 2017

samuelkarp left a comment

Choose a reason for hiding this comment

dqminh commented Jan 11, 2017

estesp commented Dec 16, 2016 •

edited

Loading

runcom commented Dec 16, 2016 •

edited

Loading

estesp commented Dec 16, 2016 •

edited

Loading

stevvooe Dec 16, 2016 •

edited

Loading