Overall, the goal with this is to make the Makefile smaller. Remove bootstrap and setup scripts from the Makefile so that it is easier to reason about and maintain.

Codecov Report

Merging #1811 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1811   +/-   ##
=======================================
  Coverage   47.33%   47.33%           
=======================================
  Files          91       91           
  Lines        8964     8964           
=======================================
  Hits         4243     4243           
  Misses       4022     4022           
  Partials      699      699

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a921fb6...cc9216c. Read the comment docs.

LGTM

Not against the change, but I think it makes it harder to find dependencies, would vote for adding a getting-started for contributors.

I don't think we should pin dependencies for build tools. It too often leads to out of date build tools and removes the impetus for upgrading build tools and/or processes. The effect here is that developers that use containerd have no choice but to use this tool to install the containerd build dependencies, rather than using what is in their particular environment.

If we do pin these deps, I'd prefer they be pinned through the vendor folder (per the TODO, that is getting removed here: "Install these from the vendor directory").

it makes it harder to find dependencies, would vote for adding a getting-started for contributors.

Sounds reasonable to me. While it probably does make it a bit harder to find I still think it's an improvement. Currently dependencies are scattered across a bunch of files: .travis.yml, Makefile, and README.md (which is unlikely to remain in sync). By moving the dependencies into a single location developers can find everything in a single spot, which can be referenced from the README and whatever developer docs.

There is currently a Developer Quick-Start in the README. I can open an issue about adding more docs for contributors so we can gather some requirements. Sound good?

There is currently a Developer Quick-Start in the README. I can open an issue about adding more docs for contributors so we can gather some requirements. Sound good?

SGTM, cheers.

What's left in this PR is getting the values from the vendor.conf or defaulting to tip as per @stevvooe request then.

The effect [of pinning dependencies for build tools] is that developers that [contribute to] containerd have no choice but to use this tool to install the containerd build dependencies, rather than using what is in their particular environment.

What makes you think this is the case?

Developers are free to use what they want. If they use some arbitrary version of a tool it may fail because the behaviour is different. So they are left trying to guess at what version they need. By pinning dependencies we make it explicit which version works, but it doesn't force anyone to use that version. If they have a version that works great! If they don't, they now have a convenient way to install a version that works. If they're new to the project and don't have any tools yet it's much easier for them to get started.

I think it's important that people can use their own tools. #1819 should be careful to mention that the install-dev-tools script is just one option, and could be used as a reference for what tools are required.

Without pinned versions the build could fail at any time when a tool decides to add new rules, or change behaviour.

If we do pin these deps, I'd prefer they be pinned through the vendor folder

I don't see how that is workable. vendor is for "library dependencies" (packages that are imported by code in the repo). Refrence: https://blog.golang.org/go1.7

Go 1.6 includes support for using local copies of external dependencies to satisfy imports of those dependencies, often referred to as vendoring.

Dependency tools (vndr, glide, dep) will all (correctly) prune out any "unused" (not imported) dependencies. So even adding them to vendor.conf would mean that every person trying to update vendor/ would have to manually restore these packages. Vendoring tools were designed to handle import dependencies, not external binaries/tools. Trying to force them to handle this case causes more problems then it solves.

vendor/ should really be checked as part of CI as well (I'll be opening a PR for that next) to verify that the files in vendor/ match the result of running vndr. Otherwise it is very difficult to ensure that a PR that changes vendor is actually correct. This check is impossible if vendor/ includes non-import dependencies.

@dnephin can we just get the revision to build from out of vendor.conf ?

It's possible, but why would we want to do that?

All I see are disadvantages. It makes it more difficult to prune unused things from vendor.conf. How do you know if it's an unused library or a build tool? It forces a script to be aware of vendor.conf for no reason. It mixes the list of library imports with external tools.

It may actually make running vndr slower, because it would have to clone those extra repos as well, and then prune them later.

In moby/moby we had a separate file for "tool versions", but it seemed to be mostly just a hassle and I couldn't see any advantages to splitting it up.

@dnephin The main problem with this change is that it assumes that the Makefile owns GOPATH. It should not. There should be no mention of GOPATH. It should not checkout anything in GOPATH. GOPATH should be a passthrough to the underlying Go toolchain and that is it. This is a huge problem when developing moby and I don't want to replicate it here.

Setting up the tools is a two second problem when you first develop then you never worry about it again. If the tool changes, it takes two seconds to fix the build or remove the tool. I really don't want to have a bomb buried in a Makefile twiddling the GOPATH without a care in the world. It just doesn't make sense to 10x the complexity to solve a problem that doesn't exist.

The current build is a trade off for allowing many kinds of development environments, including those that actually follow How to Write Go Code. When we add things that make it "easier", it makes it harder to manage deps when you follow that approach. Yes, the build tools aren't pinned, but sacrificing simplicity of development environment is not a good trade off.

The main problem with this change is that it assumes that the Makefile owns GOPATH

What do you mean by "owns" ? Nothing in this PR sets GOPATH (.appveyor.yml is an exception for dealing with windows drive paths. It sets it for a single build step. If that's an issue I can find another fix, but it shouldn't be, because we're already setting GOPATH in this config).

A script does read GOPATH, that is all. I don't see the harm in reading GOPATH.

It should not checkout anything in GOPATH. GOPATH should be a passthrough to the underlying Go toolchain and that is it.

That is exactly what it's doing. go get checks out something in GOPATH, then we cd to that directory. Is there a more correct way to ask go for the filepath to a package, by package name?

I could use go list -f '{{.Dir}}' $repo if you want. That's probably cleaner.

Yes, the build tools aren't pinned, but sacrificing simplicity of development environment is not a good trade off.

There is nothing in this PR that should make anything more complex, or make https://golang.org/doc/code.html harder to follow.

Updated to use go list -f '{{.Dir}}' $repo so there's no need to even read GOPATH anymore.

ping @stevvooe does that work for you now?

If the issue is that the script installs things into GOPATH/GOBIN, another alternative is to have the script use a tmpdir as the GOPATH (which it removes after it builds), and accept a positional arg for the install path. That way it's up to the user to tell it where to install, and it would touch GOPATH it all.

Installing to GOPATH is fine, but checking out a project to a detached state is a problem if there is a local change. Either the script breaks due to the local changes or it overwrites them somehow (either stashing or force).

I'm still against locking down dev tools. This will stagnate the build environment and let issues fester if we don't address them. Upgrading the build tools becomes a hindrance.

I'm fine with always installing them from master. Especially since this is only used on CI, it'll force us to keep up.

Doesn't go get -u present the same problem if there are local changes? What about tools that can't be installed with go get? They would have to be pinned to some release because there is no "master".

Anyway, I've removed the version pinning. This is now just a go get -u. I've added vndr since that is another golang dev tool that is used by the project.

LGTM

Doesn't go get -u present the same problem if there are local changes?

It doesn't cause any destruction. These scripts tend to lack discipline, so we can't say the same for them.

What about tools that can't be installed with go get? They would have to be pinned to some release because there is no "master".

Which ones?

LGTM