vendor: adding dependency mgmt with dep

[dmmcquay]

Adds dep dependency management.

• Change makefile according to new
• Solve github.com/huin/goupnp import issue (currently manually added files)

fixes #2388

[dmmcquay]

@lukechampine could help me understand what you did here NebulousLabs/go-upnp@026fadc ?

[lukechampine]

ah. So, we vendored https://github.com/huin/goupnp, but found that it was in need of some fixes. We applied the fixes to the vendored code. So now it's out of sync with the actual repo.
What's usually done in this circumstance? You can try to get the upstream maintainer to merge your changes, but that's not always an option. We could also fork goupnp like we've done with other projects (e.g. bolt)

[dmmcquay]

Yeah, I looked at the difference with github.com/huin/goupnp and it basically is that we pass in context.Context to a function call that no longer takes one (or maybe never did, idk):

vendor/github.com/NebulousLabs/go-upnp/upnp.go:134:67: too many arguments in call to internetgateway1.NewWANPPPConnection1Clients
        have (context.Context)
        want ()

I could make a small patch to the lines that are complaining (L134 and L138) but I am not familiar with the code enough to know if the ctx are actually used. From what I can tell, they are.

I personally don't like the idea of forking cause it makes us responsible for its upkeep, but that is just my opinion.

What do you think @lukechampine? Do you think we can remove the contexts in those lines in go-upnp?

[lukechampine]

we added the context recently in order to enable cancellation of port forwarding. So we can't just strip it out.
I guess this ties back into the dependency management discussion. What should we do when we want to make modifications to one of our dependencies?

[dmmcquay]

@lukechampine depends how often you think you'll be making those changes. If it is something that would benefit the package for others, I think contributing back to the package would be the correct thing to do. If it is something frequent or a drastic change, then forking makes more sense to me.

[dmmcquay]

Makefile updates are complete and github.com/huin/goupnp import issue solved (NebulousLabs/go-upnp#9).

[starius]

I think we should wait for golang/dep#944 to reduce the size of committed vendor/ directory. They are working exactly on removing tests and all non-Go files except README and LICENSE.

[starius]

Also, can Travis script verify that:

1. Gopkg.toml matches code (i.e. covers all dependencies and has no unnecessary projects)
2. Gopkg.lock matches Gopkg.toml (i.e. has most recent versions for projects using branch="master") and code (i.e. "packages" field covers all dependencies and has no unnecessary packages).
3. Vendor dir matches Gopkg.lock

?

The script should return error if the failure is a result of the pull request. Otherwise just a warning. If this is hard to achieve, then return error if the failure happens due to requirements 1 and 3 (because 2 can start failing itself as a result of push to a dependency repo).

[sdboyer]

noticed this crosslink from one of our issues, so lemme answer @starius' questions 😄

Gopkg.toml matches code (i.e. covers all dependencies and has no unnecessary projects)

this is the "ineffectual constraints" problem, which we've had a longstanding issue open to address in hopes that someone would pick it up. i suppose it'd be possible to do via a separate script, though i think it may be a struggle.

Gopkg.lock matches Gopkg.toml (i.e. has most recent versions for projects using branch="master")

there may be some nuance here, but if you really want this flow - where you don't accept a commit unless all (or d you only care about branches?) of its dependencies are at the latest possible one allowable by constraints - then the best thing to do may be to run dep ensure -update and just see if the Gopkg.lock changes at all.

code (i.e. "packages" field covers all dependencies and has no unnecessary packages).

you probably won't get much value out of this check - unless you've got particular requirements around special build tags, it's basically not possible for dep to get this wrong. or IOW if dep does get it wrong, it's a critical bug in dep that we'll have fixed very quickly; otherwise, a discrepancy is more likely to indicate a bug in the verification script.

Vendor dir matches Gopkg.lock

this will be quite onerous to do externally, but is something we're aiming to have done reasonably soon - golang/dep#121.

[starius]

Hey! Thanks for follow-up! If I understand correctly, the only challenging part is the first one (Gopkg.toml matches code). All other can be implemented by regenerating Gopkg.lock and vendor dir from scratch and checking if anything changed at all. With regard to the "Gopkg.toml matches code" it seems to be also not really challenging. The idea of the algorithm is as follows: generate new Gopkg.toml (in memory) from scratch and check if the sets of projects are the same in the generated Gopkg.toml and real Gopkg.toml.

…

On Thu, Oct 26, 2017 at 4:44 AM, sam boyer ***@***.***> wrote: noticed this crosslink from one of our issues, so lemme answer @starius <https://github.com/starius>' questions 😄 Gopkg.toml matches code (i.e. covers all dependencies and has no unnecessary projects) this is the "ineffectual constraints" problem, which we've had a longstanding issue open to address in hopes that someone would pick it up. i suppose it'd be possible to do via a separate script, though i think it may be a struggle. Gopkg.lock matches Gopkg.toml (i.e. has most recent versions for projects using branch="master") there may be some nuance here, but if you really want this flow - where you don't accept a commit unless all (or d you only care about branches?) of its dependencies are at the latest possible one allowable by constraints - then the best thing to do may be to run dep ensure -update and just see if the Gopkg.lock changes at all. code (i.e. "packages" field covers all dependencies and has no unnecessary packages). you probably won't get much value out of this check - unless you've got particular requirements around special build tags, it's basically not possible for dep to get this wrong. or IOW if dep does get it wrong, it's a critical bug in dep that we'll have fixed very quickly; otherwise, a discrepancy is more likely to indicate a bug in the verification script. Vendor dir matches Gopkg.lock this will be quite onerous to do externally, but is something we're aiming to have done reasonably soon - golang/dep#121 <golang/dep#121>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2394 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA4KW-QIRUi6hspBcPAu9yh9wVNO4yCuks5swAAigaJpZM4PseqX> .

-- Best regards, Boris Nagaev

[sdboyer]

The idea of the algorithm is as follows: generate new
Gopkg.toml (in memory) from scratch and check if the sets of projects are
the same in the generated Gopkg.toml and real Gopkg.toml.

hmm...maybe? so you'd mv Gopkg.toml and then dep init again, then compare the difference there? i can't point to a specific thing, but dep init is trying to do a very difficult task (convert the varied the universe of ways people have organized their Go projects in the past over to dep's model), so it may not be quite as precise as what you're hoping for.

i have a draft of a blog post that i'll get around to...sometime soon here i hope, which illustrates how i hope CI workflows could work. it wouldn't be terribly difficult to stitch together the behavior described in there - it's just that it's one of many, many priorities, and we've not gotten to it yet.

[tbenz9]

Reading the history it looks like the primary reason this PR stagnated was to wait for golang/dep#944, which has now been merged into dep v0.4. We may want to revisit this PR; especially since we now vendor bbolt.

[dmmcquay]

@tbenz9 @lukechampine sounds good to me. Let me know the direction you want to move in and I can update the PR.

[lukechampine]

@dmmcquay interesting timing... Russ Cox just posted this: https://research.swtch.com/vgo-intro

So the state of dependency management has been thrown into disarray once more. It looks like there will be a painless upgrade path from dep, so perhaps we should continue going down that route. Even if the vgo stuff is accepted, it won't become official until Go 1.12 at the earliest, which is a year away.

[tbenz9]

Even if we choose the vgo route when its rolled into an official Go release seems like we should still implement the dep solution provided ASAP.

[sdboyer]

yep - dep will still provide you a smoother vector into vgo than your current state, whatever from vgo ultimately ends up taking (still very unclear)

[lukechampine]

Now that the vgo proposal has been accepted, I think we should make a renewed effort to manage our dependencies with vgo.

@dmmcquay -- thanks for your work on this, and sorry for our sluggishness. If you're interested in migrating Sia to vgo, we'd be grateful for that. I expect that it won't be too difficult, but you never know. Otherwise, I'll go ahead and start working on it myself.

[dmmcquay]

@lukechampine would be glad to do it. I haven't played around with vgo yet and would be a good excuse to!