project: use vndr for vendoring by LK4D4 · Pull Request #27912 · moby/moby

LK4D4 · 2016-10-31T19:40:00Z

- What I did
Vendored all dependencies with github.com/LK4D4/vndr and removed hack/vendor.sh. Also, I've fixed some minor stuff:

a mistake in the comment in validate-seccomp
use package name instead of path in go build because pathes doesn't work correctly with AUTO_GOPATH

I tried make validate, make cross and hack/make.sh {dyn}binary in make shell with AUTO_GOPATH=1 and without it.
I think vndr provides slightly better experience than hack/vendor.sh with same features.

- How I did it
Slightly modified hack/vendor.sh to convert it to vendor.conf and ran vndr tool.
- How to verify it
Try to compile docker.
- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

cpuguy83 · 2016-10-31T19:51:29Z

Yes for native vendor dir.

cpuguy83 · 2016-10-31T19:51:38Z

ping @tianon

thaJeztah · 2016-10-31T20:53:45Z

can we keep hack/vendor.sh and make it run vendr? Also put a comment in that file to explain how to do it "the new way"?

We've taught people how to do vendoring for some time, so any pointers we could give people that are used to the existing workflow would help :D

thaJeztah · 2016-10-31T20:54:29Z

We also should leave a comment on these PR's https://github.com/docker/docker/pulls?q=is%3Aopen+is%3Apr+label%3Astatus%2Fneeds-vendoring to explain what needs to be changed

LK4D4 · 2016-10-31T20:54:31Z

@thaJeztah I'd love never write bash again, but if you insist.

thaJeztah · 2016-10-31T20:55:05Z

@LK4D4 I'm sure @tianon can help :)

LK4D4 · 2016-10-31T21:27:27Z

@thaJeztah put hack/vendor.sh back with comment. Where should I add doc about new vendoring process?

thaJeztah · 2016-10-31T21:32:13Z

@LK4D4 no, this is perfect, just what I meant, thanks!

We can remove the file in a while, it's just temporary :)

runcom · 2016-11-01T09:35:17Z

works like a charm, @LK4D4 yeah pls do not remove AUTO_GOPATH 😄 (in reply of an IRC question)

justincormack · 2016-11-01T11:26:56Z

hack/make/validate-default-seccomp

typo , missing p...

justincormack · 2016-11-01T11:29:42Z

Apart from a minor typo, seems like a huge improvement, LGTM.

LK4D4 · 2016-11-01T14:34:06Z

Fixed typo. Thanks!

aaronlehmann · 2016-11-01T21:10:22Z

hack/vendor.sh

Use of Pls here is kind of weird.

Yes, you're right. I'll fix

aaronlehmann · 2016-11-01T21:11:49Z

vendor.conf

Would this transition be a good opportunity to require hashes for each vendored repo? Relying on upstream tags kind of worries me.

I'd like to keep it outside this PR because it would move it back to design review. There is no consensus among maintainers about using tags vs revisions.

Definitely for a separate PR, but my ideal would be both "tag" (if exists, and for readability) and hash. I.e. "0.5.2 -> 0.5.3" gives meaning to a change, but the hash makes sure that an upstream doesn't re-push a tag (which happened!)

tianon

Overall looks sane to me! 👍 ❤️

(just one minor comment)

tianon · 2016-11-01T22:38:02Z

hack/vendor.sh

This $@ should be quoted, ie:

vndr "$@"

LK4D4 · 2016-11-02T14:18:42Z

@tianon @aaronlehmann fixed comments!

LK4D4 · 2016-11-03T18:59:30Z

ping @thaJeztah @vdemeester

vdemeester

LGTM 🐸
Need vndr locally to update the vendor though right ? (we could have a make vendor or something, but could be done in a follow-up PR)

thaJeztah · 2016-11-03T22:23:40Z

moving to merge!

Signed-off-by: Alexander Morozov <[email protected]>

tamird · 2016-11-04T03:08:16Z

This breaks effectively all downstream dependents of this repo with compilation failures like:

pkg/acceptance/cluster/docker.go:285: cannot use port (type "github.com/docker/go-connections/nat".Port) as type "github.com/docker/docker/vendor/github.com/docker/go-connections/nat".Port in map index
pkg/acceptance/cluster/localcluster.go:193: cannot use retryingClient (type retryingDockerClient) as type "github.com/docker/docker/client".APIClient in field value:
    retryingDockerClient does not implement "github.com/docker/docker/client".APIClient (wrong type for ContainerCreate method)
        have ContainerCreate("golang.org/x/net/context".Context, *container.Config, *container.HostConfig, *network.NetworkingConfig, string) (container.ContainerCreateCreatedBody, error)
        want ContainerCreate("github.com/docker/docker/vendor/golang.org/x/net/context".Context, *container.Config, *container.HostConfig, *network.NetworkingConfig, string) (container.ContainerCreateCreatedBody, error)
pkg/acceptance/cluster/localcluster.go:442: cannot use map["github.com/docker/go-connections/nat".Port]struct {} literal (type map["github.com/docker/go-connections/nat".Port]struct {}) as type map["github.com/docker/docker/vendor/github.com/docker/go-connections/nat".Port]struct {} in field value

LK4D4 · 2016-11-04T03:26:13Z

@tamird whoa, that is really unfortunate.
I'm really not sure what to do, looks like new vendoring scheme is very bad for dependencies.

tamird · 2016-11-04T03:28:29Z

Yes, go's vendoring feature is basically terrible for this reason :(

LK4D4 · 2016-11-04T03:28:58Z

Actually I kinda know what to do. Like in etcd, we need to move vendor folder under cmd. Will try tomorrow.

GordonTheTurtle added the status/0-triage label Oct 31, 2016

vdemeester added status/1-design-review and removed status/0-triage labels Oct 31, 2016

justincormack reviewed Nov 1, 2016

View reviewed changes

hack/make/validate-default-seccomp Outdated

Copy link

Contributor

justincormack Nov 1, 2016

Choose a reason for hiding this comment

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

typo , missing p...

justincormack added status/2-code-review and removed status/1-design-review labels Nov 1, 2016

aaronlehmann reviewed Nov 1, 2016

View reviewed changes

tianon reviewed Nov 1, 2016

View reviewed changes

hack/vendor.sh Outdated

Copy link

Member

tianon Nov 1, 2016

Choose a reason for hiding this comment

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

This $@ should be quoted, ie:

vndr "$@"

tianon approved these changes Nov 3, 2016

View reviewed changes

vdemeester approved these changes Nov 3, 2016

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review labels Nov 3, 2016

project: use vndr for vendoring

f2614f2

Signed-off-by: Alexander Morozov <[email protected]>

LK4D4 merged commit c072347 into moby:master Nov 4, 2016

LK4D4 deleted the vndr branch November 4, 2016 01:30

tamird mentioned this pull request Nov 4, 2016

GLOCKFILE: update dependencies cockroachdb/cockroach#10437

Merged

amitkris mentioned this pull request Nov 4, 2016

Add functional support for Docker sub commands on Solaris #28056

Merged

This was referenced Nov 15, 2016

vendor: move vendor dir under cmd #28080

Closed

GLOCKFILE: update dependencies cockroachdb/cockroach#10711

Merged

GLOCKFILE: update dependencies cockroachdb/cockroach#10785

Merged

GLOCKFILE: update dependencies cockroachdb/cockroach#10957

Merged

nishanttotla mentioned this pull request Dec 7, 2016

Use vndr for vendoring docker-archive/classicswarm#2566

Closed

tamird mentioned this pull request Dec 13, 2016

Unable to use some of the docker API due to vendoring issues #29362

Closed

thaJeztah mentioned this pull request Mar 28, 2017

Cannot use client API due to dependencies vendoring #32182

Closed

Conversation

LK4D4 commented Oct 31, 2016

Uh oh!

cpuguy83 commented Oct 31, 2016

Uh oh!

cpuguy83 commented Oct 31, 2016

Uh oh!

thaJeztah commented Oct 31, 2016

Uh oh!

thaJeztah commented Oct 31, 2016

Uh oh!

LK4D4 commented Oct 31, 2016

Uh oh!

thaJeztah commented Oct 31, 2016

Uh oh!

LK4D4 commented Oct 31, 2016

Uh oh!

thaJeztah commented Oct 31, 2016

Uh oh!

runcom commented Nov 1, 2016

Uh oh!

justincormack Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

justincormack commented Nov 1, 2016

Uh oh!

LK4D4 commented Nov 1, 2016

Uh oh!

aaronlehmann Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

LK4D4 Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

aaronlehmann Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

LK4D4 Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

thaJeztah Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

tianon Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Nov 2, 2016

Uh oh!

LK4D4 commented Nov 3, 2016

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Nov 3, 2016

Uh oh!

tamird commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LK4D4 commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tamird commented Nov 4, 2016

Uh oh!

LK4D4 commented Nov 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

tamird commented Nov 4, 2016 •

edited

Loading

LK4D4 commented Nov 4, 2016 •

edited

Loading