Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

Build in a container by default, using go 1.5 and vendored dependencies. #1861

Merged
merged 2 commits into from
Jan 13, 2016

Conversation

tomwilkie
Copy link
Contributor

Fixes #1657, fixes #1656, fixes #1850. I think it also fixes #1563.

This PR:

  • makes the default, and only, way of building weave use a container
  • make container go version 1.5.2
  • uses vendored dependancies
  • removes any traces of go get to prove the vendored dependancies are all there

TODO:

@tomwilkie tomwilkie self-assigned this Jan 7, 2016
@dpw
Copy link
Contributor

dpw commented Jan 7, 2016

Why the use of gvt? What's wrong with just GOVENDOREXPERIMENT plus git submodules (or subtrees if you prefer)?

@tomwilkie
Copy link
Contributor Author

Why the use of gvt? What's wrong with just GOVENDOREXPERIMENT plus git submodules (or subtrees if you prefer)?

This is using GOVENDOREXPERIMENT.

Matthias doesn't want the dependancies checked in (ruling out manually fetching the dependancies or using subtree). I don't want to use submodules, as they are a pain.

The requirements were to pin a specific version of each dependancy, but have them fetched at build time. gvt does exactly this for us.

@dpw
Copy link
Contributor

dpw commented Jan 7, 2016

Matthias doesn't want the dependancies checked in (ruling out manually fetching the dependancies or using subtree). I don't want to use submodules, as they are a pain.

Could you elaborate on how submodules are a pain? It's possible to make their use very inconspicuous in this context.

The advantage of using submodules is that there is no need to learn the conventions of any tools beyond go and git.

(I'd guess subtrees might work as well, but I'm not very familiar with them.)

The requirements were to pin a specific version of each dependancy, but have them fetched at build time. gvt does exactly this for us.

I notice that gvt is only referred to from circle.yml. Presumably it needs to be run to retrieve the dependencies when doing a non-circle build? How does that happen?

@tomwilkie tomwilkie force-pushed the 1657-1656-vendor-and-build-in-container branch from 3e60a7f to 9df8518 Compare January 7, 2016 18:28
@tomwilkie
Copy link
Contributor Author

Could you elaborate on how submodules are a pain? It's possible to make their use very inconspicuous in this context.

I'm assuming your suggest is we subtree in every dependancy? And the do a git submodule sync/update/whatever in the makefile to ensure they are always on the right version?

We've been using gvt in scope & the service and we've found it massively easier than dealing with submodules. The submodule for tools has already causes problems for some people (Bryan, I think).

I notice that gvt is only referred to from circle.yml. Presumably it needs to be run to retrieve the dependencies when doing a non-circle build? How does that happen?

This PR is marked work in progress; we were just discussing how to do that in the net room. The PR should now include a mechanism (via an uptodate file) to ensure gvt is run correctly.

@tomwilkie tomwilkie force-pushed the 1657-1656-vendor-and-build-in-container branch 2 times, most recently from 7b8e383 to 9dd4855 Compare January 8, 2016 09:06
@rade
Copy link
Member

rade commented Jan 8, 2016

You have collapsed the WEAVEUTIL_EXE rule into SIGPROXY_EXE, DOCKEPLUGIN_EXE, etc rule. That has two undesirable consequences:

  1. the comment above that rule is now wrong since it says the programs in question do not import the net package, which weaveutil does import.
  2. the NETGO_CHECK is no longer run on weaveutil.

@rade
Copy link
Member

rade commented Jan 8, 2016

You removed the travis: $(EXES) target. I use that a lot when working on weave, since it's much faster than a full build. Happy to see it renamed, but not gone.

@tomwilkie
Copy link
Contributor Author

re: travis: $(EXES) yes I removed that as I removed all the travis build stuff. Will re-add as make exes.

@rade
Copy link
Member

rade commented Jan 8, 2016

This also fixes #1850, doesn't it?

http://docs.weave.works/weave/latest_release/building.html needs updating.

@tomwilkie
Copy link
Contributor Author

Just the TestHTTPCancel failure now, which @bboreham is working on.

@dpw
Copy link
Contributor

dpw commented Jan 8, 2016

We've been using gvt in scope & the service and we've found it massively easier than dealing with submodules.

Well, we've been using submodules for vendoring in ambergreen, and it is so inconspicuous that it is hard for me to see how anything could be massively easier.

The submodule for tools has already causes problems for some people (Bryan, I think).

Without knowing details I can't respond. If it goes beyond simple unfamiliarity, then maybe more details would be relevant. Like git in general, submodules have a learning curve. But that will be true of any vendoring tool as well. The difference is that some people are already familiar with submodules, and experience gained with them is widely reusable.

Also, from https://golang.org/s/go15vendor :

The environment variable also enables fetching of git submodules during “go get”. This is meant to allow experiments to understand whether git submodules are an appropriate and useful way to vendor code.

@tomwilkie
Copy link
Contributor Author

I'll give the submodules a try.

@tomwilkie tomwilkie force-pushed the 1657-1656-vendor-and-build-in-container branch from e9a2b9c to af043cc Compare January 8, 2016 15:09
$(WEAVEUTIL_EXE):
go build $(BUILD_FLAGS) -o $@ ./$(@D)
$(NETGO_CHECK)

This comment was marked as abuse.

@rade
Copy link
Member

rade commented Jan 8, 2016

Linting should be containerised.

ifeq ($(BUILD_IN_CONTAINER),true)
tests: $(BUILD_UPTODATE) $(VENDOR_UPTODATE) tools/.git
$(SUDO) docker run $(RM) $(RUN_FLAGS) \
-v $(shell pwd):/go/src/github.com/weaveworks/weave \

This comment was marked as abuse.

@rade
Copy link
Member

rade commented Jan 8, 2016

I am not keen on eventually having all our main Makefile targets look like:

ifeq $(BUILD_IN_CONTAINER,true)
target:
        $(SUDO) docker run ... target
else
target:
        the real build commands
fi

How can we avoid that?

@tomwilkie
Copy link
Contributor Author

I am not keen on

I can't think of another way, maybe @dpw can. I've made it so we only have one big if now, and only two docker runs (ie not one for every target).

Linting should be containerised.

Done. Added golint to container.

The WEAVEUTIL_EXE build stanza has moved gratuitously.

Sorry. Done.

why is this not also mounting .pkg? Won't it end up building all libs from scratch as a result?

(for tests) go test --help says:

    -i
        Install packages that are dependencies of the test.
        Do not run the test.

Working on figuring out if this is true or not.

@tomwilkie
Copy link
Contributor Author

those circleci env vars are annoying. Nothing else in the makefile makes any reference to circleci.

yup. what can you do?

tests lint:
$(SUDO) docker run $(RM) $(RUN_FLAGS) \
-v $(shell pwd):/go/src/github.com/weaveworks/weave \
-v $(shell pwd)/.pkg:/go/pkg \

This comment was marked as abuse.

This comment was marked as abuse.

@tomwilkie
Copy link
Contributor Author

re: go test -i

This is interests: https://code.google.com/p/go/issues/detail?id=3896

Basically, sticking a go test -i $GO_TEST_ARGS $TESTDIRS in tools/test before we run the tests make the whole thing faster it seems - just getting numbers now.

@tomwilkie
Copy link
Contributor Author

Maybe not a massive speed up, but a welcome one:

# with the go test -i
vagrant@vagrant-ubuntu-vivid-64:~/src/github.com/weaveworks/weave$ rm -rf .pkg
vagrant@vagrant-ubuntu-vivid-64:~/src/github.com/weaveworks/weave$ time make tests
...
real    4m0.493s
user    0m0.032s
sys 0m0.112s

# without the go test -i
vagrant@vagrant-ubuntu-vivid-64:~/src/github.com/weaveworks/weave$ rm -rf .pkg
vagrant@vagrant-ubuntu-vivid-64:~/src/github.com/weaveworks/weave$ time make tests
...
real    5m18.121s
user    0m0.024s
sys 0m0.188s

@rade
Copy link
Member

rade commented Jan 9, 2016

So this PR contains five changes:

  1. switch to go 1.5 (Update containerised build and Vagrantfiles to Go 1.5 #1657)
  2. vendoring (Vendor dependencies with Go 1.5 and gvt #1656)
  3. reduce travis role to just doc publishing
  4. make all Makefile targets in principle executable in containerised build and remove containerised "clone & build" (enable 'clean', 'test' etc Makefile targets to be run in containerised build #1850)
  5. drive containerised build & test & lint from Makefile (build errors when upgrading go #1563)

AFAICT, the only real dependencies are 1->2, and 4->5. Correct?

Mixing all these in a single commit, in a single PR, makes it rather more difficult to review & iterate on these. I shall try to extract a few things into separate master commits / PRs. I have already done that for a couple of small refactors.

@rade rade changed the title [WIP] Build in a container by default, using go 1.5 and vendored dependancies. [WIP] Build in a container by default, using go 1.5 and vendored dependencies. Jan 9, 2016
@tomwilkie
Copy link
Contributor Author

@dpw the link you provided also has the following paragraph, right after the one you quoted:

Note that when “go get” fetches a new dependency it never places it in the vendor directory. In general, moving code into or out of the vendor directory is the job of vendoring tools, not the go command.

I've just confirmed running go get doesn't setup submodules in the vendor dir for you, as you suggested. How did you setup you vendor dir for ambergreen?

@dpw
Copy link
Contributor

dpw commented Jan 11, 2016

I've just confirmed running go get doesn't setup submodules in the vendor dir for you, as you suggested. How did you setup you vendor dir for ambergreen?

Yeah, I thought I did, but clearly I didn't. (I wondered if I found some contorted way to persuade go get to do this, but that doesn't seem to be possible).

So I suspect I just did a regular go get on the pre-vendor repo, and then a shell one-liner to turn the contents of the GOPATH into a series of submodule add commands.

@tomwilkie tomwilkie force-pushed the 1657-1656-vendor-and-build-in-container branch from 5f3b6c7 to 6bf8878 Compare January 11, 2016 13:44
@tomwilkie
Copy link
Contributor Author

@dpw @rade PTAL, let me know what you think to the sub modules.

And @rade let me know if you want this broken into two prs.

@rade
Copy link
Member

rade commented Jan 11, 2016

Say I've started depending on a new library which in turn depends on a bunch of other libs. What do I run to add the new dependencies?

How do I update a specific dependency to a new version? go-dockerclient springs to mind, which we need to update in order to be able to use recent docker features. And what happens to the dependencies of that when I do the update?

How does one update all dependencies to the most recent version? Note that this should also remove any dependencies that are no longer needed (as well as, obviously, add any new ones). It's the kind of thing I envisage us doing every now and then, i.e. we'd run such an update, do a bunch of tests and pin any breakages to earlier working versions.

@tomwilkie
Copy link
Contributor Author

The answer to all of these, with the PR as it stands, is manually. Using
git submodule.

It's worth noting that gvt isn't a panacea. The gvt rebuild is slow, and
you have to do it too often. Gvt does allow you to add new dependencies
easily, and recursively. It does have a command to fetch the latest version
of a dependency (or all dependencies). It won't recursively update things,
nor will it remove stuff that's no longer needed.

I don't think any of these are frequent operations, so I think we should
optimise for the frequent things (get specified version of all
dependencies, which is quicker with submodules). We can then either develop
tooling to help with the above, or improve gvt to do shallow fetch etc.

On Monday, 11 January 2016, Matthias Radestock notifications@github.com
wrote:

Say I've started depending on a new library which in turn depends on a
bunch of other libs. What do I run to add the new dependencies?

How do I update a specific dependency to a new version? go-dockerclient
springs to mind, which we need to update in order to be able to use recent
docker features. And what happens to the dependencies of that when I do the
update?

How does one update all dependencies to the most recent version? Note that
this should also remove any dependencies that are no longer needed (as
well as, obviously, add any new ones). It's the kind of thing I envisage us
doing every now and then, i.e. we'd run such an update, do a bunch of tests
and pin any breakages to earlier working versions.


Reply to this email directly or view it on GitHub
#1861 (comment).

@rade
Copy link
Member

rade commented Jan 11, 2016

The answer to all of these, with the PR as it stands, is manually. Using git submodule.

That's not a great answer. I don't want us to accumulate cruft, or be stuck with ancient versions. Lack of tooling encourages that.

Surely you didn't produce the .gitmodules and associated "Subproject commit" files manually. So in my, possibly incredibly naive view, it should be easy to remove all those artifacts and re-create them from scratch, using whatever horrific script you cooked up to create them in the first place.

@tomwilkie
Copy link
Contributor Author

Yes, I did:

go list -f '{{join .Deps "\n"}}' ./prog/* | grep -E '^(github.com|k8s.io)' | grep -v 'github.com/weaveworks/weave' | sort -u > deps
cd vendor
cat ../deps | while read repo; do git submodule add -f --depth 1 https://$repo.git $repo; done

It choked on some of the golang.org imports, which I fixed up manually. And it misses some of the test dependancies, which I added manually. It wouldn't be too hard to put together some scripts which do this properly.

@tomwilkie tomwilkie force-pushed the 1657-1656-vendor-and-build-in-container branch from 2477f3c to 50384c9 Compare January 12, 2016 14:13
@tomwilkie tomwilkie changed the title [WIP] Build in a container by default, using go 1.5 and vendored dependencies. Build in a container by default, using go 1.5 and vendored dependencies. Jan 12, 2016
@tomwilkie tomwilkie removed their assignment Jan 12, 2016
FROM golang:1.5.2
ENV GO15VENDOREXPERIMENT 1
RUN apt-get update && apt-get install -y libpcap-dev python-requests time
RUN go get github.com/FiloSottile/gvt; touch /tmp/.donotremove

This comment was marked as abuse.

@awh awh self-assigned this Jan 13, 2016
@tomwilkie tomwilkie force-pushed the 1657-1656-vendor-and-build-in-container branch from 4650090 to 91224be Compare January 13, 2016 12:38
- Use submodules for vendoring.
- Don't go clean, now that everything is in .pkg.
- sudo -E as we need the environment variables.
- Install -race version of std so the tests can run without sudo.
- Cache the build image on circle, use the rebuild-image script from tools.
@tomwilkie tomwilkie force-pushed the 1657-1656-vendor-and-build-in-container branch from 91224be to c098aef Compare January 13, 2016 12:40
@tomwilkie
Copy link
Contributor Author

Rebased, squashed, and remove the incorrectly vendored golint and gvt source.

@bboreham
Copy link
Contributor

I was interested to find that this build is faster for small changes. For example, after having done a full build:

$ touch net/route.go
$ time make prog/weaver/weaver

On my machine the old build takes 6.9s and the new takes 4.1s.

This appears to be because the go get in the old Makefile was linking weaver to install it in $GOPATH/bin, although we didn't need or want that.

@bboreham
Copy link
Contributor

As discussed elsewhere, make update should be re-introduced.

@tomwilkie tomwilkie force-pushed the 1657-1656-vendor-and-build-in-container branch from fdc52a6 to 6684ae8 Compare January 13, 2016 13:59
awh added a commit that referenced this pull request Jan 13, 2016
…n-container

Build in a container by default, using go 1.5 and vendored dependencies.
@awh awh merged commit d9a32e5 into master Jan 13, 2016
@awh awh deleted the 1657-1656-vendor-and-build-in-container branch January 13, 2016 15:55
@awh awh added this to the 1.5.0 milestone Jan 13, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants