vendor: switch to dep

[dt]

So big question: is this a good idea?

dep is alpha and I think the dep developers would be the first to say it isn't
perfect: it still has plenty of bugs, unpolished cases and known-issues. That
said, the question to answer in evaluating if we should switch is not if dep is
perfect, but rather is if it is better, in our usage, than glide.

The primary motivator may well be the ability to dep ensure foo@rev. This has
been our single biggest issue with glide in practice, and it looks like dep will
offer a significant improvement here, since it only bumps foo (and anything else
needed to bump foo), not the world.

The biggest argument against switching now is probably dep's immaturity: dep is
still alpha and under pretty active design and development, so we'll certainly
find rough edges and bugs. We'll need to update our usage patterns as it evolves
upstream (though we'll obviously vendor it). If we waited, presumably more of
those bugs would be found and fixed before we ever hit them.

Two counter-arguments to that though are 1) we've found and filed bugs in every
tool we tried (govendor, gvt, glide and now dep), so waiting does not seems to
help much and 2) someone has to find those bugs. Indeed, it might actually
easier for us than many to be early adopters and debuggers, since as a FOSS
project, we can more easily link directly code samples, PRs, CI builds, etc.

Also, if we do identify places where dep could better serve our user-cases, by
giving feedback and examples early, we have better chance of dep's development
being able to take those into account and serve them more elegantly.

[rjnn]

This change is 

[benesch]

If I could LGTM on concept alone, I would.

[tamird]

Looks good to me, with a few comments. Is this sane to do?

---

Reviewed 13 of 13 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

Gopkg.toml, line 104 at r1 (raw file):

  revision = "7248742ae7127347a52ab9d215451c213b7b59da"

[[constraint]]

Comment?

---

vendor, line 1 at r1 (raw file):

Subproject commit 1f7655249788aa052d76a959be5b717c62703561

how come the content of vendor changed? I was imagining the diff would be -glide, +dep, but it looks like more than that.

---

pkg/cmd/github-post/testdata/stress-fatal, line 0 at r1 (raw file):
I think this was accidental.

---

Comments from Reviewable

[dt]

Whoops, sorry folks, I should have clarified that this isn't ready yet, I just wanted to see the CI's reaction.

[dt]

@tamird still not looking to get this mergeable right away (they might be refactoring the cli commands soon or something?) but I went ahead and wrote some words about why this might be sane, or at least not less sane than sticking with glide, in the PR desc.

[tamird]

If this works, I'm 👍 on it. The PR description (with typos etc fixed) should become the commit message and we should just merge this after an announcement on the forum.

Glide certainly seems like mostly abandonware now, so again, +1.

---

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[dt]

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

vendor, line 1 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

how come the content of vendor changed? I was imagining the diff would be -glide, +dep, but it looks like more than that.

Yeah, I flattened all our glide.lock entries into one giant dep ensure command, in the hope that that would produce zero vendor changes... but dep seems to strip vendor: looks like it produces 288MB/17k files, whereas our current glide-produce vendor is 323MB/21k files.

We could try asking glide to do the same, to observe the glide-vs-dep diff (or lack thereof) in isolation.

---

Comments from Reviewable

[tamird]

On Thu, Jun 1, 2017 at 2:16 PM, David Taylor ***@***.***> wrote: We could try asking glide to do the same, to observe the glide-vs-dep diff (or lack thereof) in isolation.

That'd be useful, I think.

[dt]

re-running the dep conversion on top of a vendor trimmed using glide i -v, eliminates a fair amount of the change, but not all of it: looks like a few libs are at a different version -- indeed, in Gopkg.lock, it shows e.g. genproto at aa2eb687b4d3e17154372564ad8d6bf11c3cf21f, but the initial ensure used to import glide.lock passed google.golang.org/genproto@bb3573be0c484136831138976d444b8754777aff. Still trying to figure out why.

[dt]

Edited Gopkg.lock by hand to set the initial version to match glide.lock and re-ran ensure on top of the stripped vendor -- the diff in vendor looks like what I'd expect now. I guess this is... RFAL? I'm still going to try to figure out why my initial mega-ensure didn't work as I expected, but hey, this is green now, and the diff looks about right.

[benesch]

The diff looks just about right to me, but where did github.com/petar/GoLLRB come from?

[dt]

GoLLRB comes in via a standalone test util (pkg main) in google/btree, which comes in via etcd.

[benesch]

Right, but why only with dep? Different import resolution mechanism from Glide?

[dt]

yeah, I'll bet glide ignored non-imported pkg main

[tamird]

My guess is that glide brought in the transitive closure of dependencies, taking extra steps to remove the delta between that and the transitive closure of repositories. Dep just brings in the repositories unmolested?

…

On Jun 9, 2017 07:53, "Nikhil Benesch" ***@***.***> wrote: Right, but why only with dep? Different import resolution mechanisms? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16275 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsPPZfM6Mye4qOFksJl9oEaOBLp_GTks5sCVxcgaJpZM4NtaKN> .

[benesch]

In that case, LGTM!

[dt]

So, indeed, it looks like dep ensure foo@rev doesn't respect rev if you don't actually depend on foo directly. I asked @sdboyer about this and it sounds like that might be expected and/or explicitly disallowed in some near future.

w.r.t the new GoLLRB dep: google/btree, which we get via etcd, depends on it -- the import is in a file that's // +build ignore'ed and package main, so maybe that's why glide was ignoring it, but I'm not sure.

[tamird]

Thanks, I'll have a look at this soon, but feel free to merge without my LGTM.

…

On Jun 9, 2017 12:38, "David Taylor" ***@***.***> wrote: So, indeed, it looks like dep ensure ***@***.*** doesn't respect rev if you don't actually depend on foo directly. I asked @sdboyer <https://github.com/sdboyer> about this and it sounds like that might be expected and/or explicitly disallowed in some near future. w.r.t the new GoLLBR dep: google/btree, which we get via etcd, depends on it -- the import is in a file that's // +build ignore'ed and package main, so maybe that's why glide was ignoring it, but I'm not sure. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#16275 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsPO7D-tmx4Dhr--sW6lClm5lcFN7yks5sCZ9RgaJpZM4NtaKN> .

[sdboyer]

w.r.t the new GoLLBR dep: google/btree, which we get via etcd, depends on it -- the import is in a file that's // +build ignore'ed and package main, so maybe that's why glide was ignoring it, but I'm not sure.

IIRC glide does ignore these, but dep does not (for now), so yeah, that's probably why it showed up.

You can always put github.com/petar/GoLLRB in the ignored list in Gopkg.toml 😄

[dt]

Huh, actually we might need to "constrain" every dependency to branch=master if we want update to pick up upstream changes in between tagged releases, since in the absence of any explicit constraint, when dep ensure -update picks "the lastest version allowed", it picks the latest tagged release before master, and it looks like several upstreams do not tag frequently at all.

This is really unfortunate -- if we need excplicit constraints for every dependency just to make them updateable, we a) dramatically increase the chance of spurious conflicts (where previously we'd have happily deferred to an upstream's preference) and b) we have a de-normalization of our dep closure that is not mechanically maintained -- even though dep discovers edges on the fly from imports, when an import is added or removed, we now need to maintain this denomalization.

[tamird]

Maybe that's fine for now? If a project does tag its releases maybe we don't always want master?

…

On Jun 9, 2017 16:17, "David Taylor" ***@***.***> wrote: Huh, actually we might need to "constrain" every dependency to branch=master if we want update to pick up upstream changes in between tagged releases, since in the absence of any explicit constraint, when dep ensure -update picks "the lastest version allowed", it picks the latest tagged release before master, and it looks like several upstreams do not tag frequently at all. This is really unfortunate -- if we need excplicit constraints for every dependency just to make them updateable, we a) dramatically increase the chance of spurious conflicts (where previously we'd have happily deferred to an upstream's preference) and b) we have a de-normalization of our dep closure that is not mechanically maintained -- even though dep discovers edges on the fly from imports, when an import is added or removed, we now need to maintain this denomalization. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#16275 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsPF2tCXBiHXDi4RY9UdV_HkVj3oxaks5sCdKhgaJpZM4NtaKN> .

[sdboyer]

This is really unfortunate -- if we need excplicit constraints for every dependency just to make them updateable,

A small quibble, maybe, but the constraint type is probably more defining the track for updates. This is why I'm not necessarily the biggest fan of leaving deps unconstrained (golang/dep#213) - it allows for subtle track-switching in ways that might be surprising to users.

we a) dramatically increase the chance of spurious conflicts (where previously we'd have happily deferred to an upstream's preference) and b) we have a de-normalization of our dep closure that is not mechanically maintained -- even though dep discovers edges on the fly from imports, when an import is added or removed, we now need to maintain this denomalization.

Hmm...interesting. If this is your goal, then I think what you really might be after is preferred versions, maybe? They could allow you to specify less, but have a bit more stability by deferring to what your dependencies have locked.

It would be good to include this overall context in golang/dep#738 😄

[tamird]

---

Reviewed 1 of 13 files at r2, 12 of 12 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable