search

pwaller / docker-show-context

Show where time is wasted during the context upload of `docker build`
MIT License
335 stars 16 forks source link

Vendor all dependencies with dep #5

Closed ryboe closed 6 years ago

ryboe commented 6 years ago

changes

This is a great project, but the dependencies are in a bad state. This is partly due to trying to use git submodules to manage dependencies, but mostly to the complete mess that is docker's build, release, and versioning process. After some fighting, I was able to get dep to vendor the correct versions of moby/moby and the docker/docker transitive dependency. The dependencies are now in as good a state as I can make them.

I suspect you were using submodules to avoid checking in all the source code in vendor/. I realize large commits like these are ugly, but having an unbuildable project is uglier. Because of the docker/moby mess, your dependency situation is especially tricky. You can avoid a lot of headaches for users and contributors by just checking in your deps.

pwaller commented 6 years ago

Hi! Thanks for your contribution and your efforts. I want to fix this. However, I'm very reluctant to take this PR as is. There are a few issues as I see it.

The first is that it is unnecessarily large. As I understand it, the total list of package dependencies is:

$ go list -f '{{join .Deps "\n"}}' github.com/pwaller/docker-show-context | xargs go list -f '{{if not .Standard}}{{.ImportPath}}{{end}}' | sed 's|.*vendor/||'

github.com/Sirupsen/logrus
github.com/docker/docker/builder/dockerignore
github.com/docker/docker/pkg/archive
github.com/docker/docker/pkg/fileutils
github.com/docker/docker/pkg/idtools
github.com/docker/docker/pkg/ioutils
github.com/docker/docker/pkg/pools
github.com/docker/docker/pkg/promise
github.com/docker/docker/pkg/system
github.com/docker/go-units
golang.org/x/net/context

So we should be able to avoid pulling in most of docker. If we consider these packages alone, it is clear that we should not need to vendor 218k+ lines of code. (I count somewhere more like 15k).

Another issue is that I want to be able to personally audit the code to the extent that I can. I can't accept a 218k+ line diff, unless I can independently generate it exactly. Better would be if I were supplied with instructions which generated such a diff. Better still would be to pull directly from the authoritative sources, so that one doesn't need to trust too much about my repository.

Another issue is that, docker may have renamed to moby, but their codebase is littered with package declarations indicating that it should still be imported as docker/docker. See https://golang.org/cmd/go/#hdr-Import_path_checking. I have just now learned that this checking is disabled in vendor directories, interestingly.

vgo is also on the near-term horizon, so I'm reluctant to start using a tool which may be soon obsolete.

So I'm thinking we might need another strategy. One possibility is to pull in the imports we need directly.

By the way, I have also been thinking about these issues in another context. I'm hopeful that vgo might provide a good route forward.

pwaller commented 6 years ago

Please take a look at #6, I would be interested to know if it works for you.

pwaller commented 6 years ago

FYI, I'm now away and not responding to messages for a little while. I'll come back to this in a couple of weeks. Apologies for the delay!

ryboe commented 6 years ago

vgo is the future, but it's not here yet. If you're okay with making vgo a requirement for installing, that's fine. Personally, I would want to wait until August when Go 1.11 drops with "experimental" vgo support. That way docker-show-context could be installed without needing a parallel build tool. Meanwhile, docker-show-context is broken today.

Another issue is that, docker may have renamed to moby, but their codebase is littered with package declarations indicating that it should still be imported as docker/docker.

I don't think that's a correct interpretation of the import comments. I think they were trying to avoid breaking old package imports, not encouraging the use of docker/docker instead. The go spec says this:

In this way, import comments let package authors make sure the custom import path is used and not a direct path to the underlying code hosting site.

...but that's not how Docker are using it. docker/docker is just a GitHub redirect to moby/moby. I tried changing the URLs back to docker/docker and it was a dep solving failure. AFAICT my PR is the only way to get the deps in a correct state (with dep anyway). Your vgo PR is vendoring Ye olde docker from Feb 2017. You might be able to get the current release by changing the imports to moby/moby.

As for this being too much code, dep decided that it needed all that code. I trust it. I see there are github.com/Microsoft packages in vendor/, so I suspect the difference between go list and what dep is actually vendoring is due to Windows support. The output of go list may only be the dependencies needed to run on your system.

tl;dr It's up to you, but I'd land dep today and transition to vgo sometime after Go 1.11 drops.

pwaller commented 6 years ago

Thanks again for your input. Sorry I chose not to go with your method. The vgo method now seems to work. Let me know if you see any issues with it (other than the fact that at the moment you have to grab the beta vgo tool! :))