Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build in a container, use go1.5, vendor Dependancies, build for Darwin and Arm on Circle. #584

Merged
merged 23 commits into from
Oct 26, 2015

Conversation

tomwilkie
Copy link
Contributor

Fixes #535, Fixes #550, Fixes #530

With this change, make should build the scope container on a correctly configured Mac, using Docker Machine.

See #583 for more context.

@tomwilkie tomwilkie self-assigned this Oct 22, 2015
@tomwilkie tomwilkie force-pushed the 535-vendor branch 2 times, most recently from 8008334 to 05db73d Compare October 23, 2015 13:03
@tomwilkie
Copy link
Contributor Author

Latest: instead of vendoring whole repos in vendor/, gvt supports vendoring just the sub modules we actually use (it does this by default). This reduced the number of files from ~18k to ~2.5k, which is a massive improvement.

The commands I ran to produce this were:

go list -f '{{join .Deps "\n"}}' ./app ./probe | grep -E '^(github.com|k8s.io)' | grep -v 'github.com/weaveworks/scope' | sort -u > deps
cat deps | xargs -n1 gvt fetch

To update the vendored dependancies, you can do a gvt update github.com/foo/bar, or a gvt update -all

@2opremio
Copy link
Contributor

Some numbers in my Vagrant box (using NFS):

scope $ git branch
  535-vendor
* master

scope $ time git checkout 535-vendor                                                                                                                                                       
Checking out files: 100% (2439/2439), done.
Switched to branch '535-vendor'
Your branch is up-to-date with 'origin/535-vendor'.

real    0m13.652s
user    0m0.192s
sys     0m0.904s

scope$ time git status
On branch 535-vendor
Your branch is up-to-date with 'origin/535-vendor'.
nothing to commit, working directory clean

real    0m0.898s
user    0m0.000s
sys     0m0.128s

scope $ time git diff

real    0m0.076s
user    0m0.004s
sys     0m0.044s

scope $ time make clean                                                                                                                                                                    
go clean ./...
rm -rf scope.tar  app/scope-app probe/scope-probe client/build/app.js docker/weave

real    0m4.609s
user    0m0.452s
sys     0m0.876s

scope $ du -hs .
176M    .

scope $ docker pull golang:1.5.1
[...]
scope $ time make #fresh
[...]
real    3m7.191s
user    0m0.972s
sys     0m3.204s

scope $ touch report/id.go # second time
scope $ time make 
[...]
real    1m44.136s
user    0m0.336s
sys     0m2.560s

scope $ time make # third time without modifying anything (to measure overhead)
[...]
real    0m15.273s
user    0m0.328s
sys     0m2.648s

@tomwilkie
Copy link
Contributor Author

Thanks for the timing info. Are those times acceptable? Note the slow down in build is due to go1.5, not the vendoring - I can't see a reason for the vendoring to make anything slower.

Your git status time seems fine, and the checkout time won't be a problem once the change goes in. What do you think?

@tomwilkie tomwilkie removed their assignment Oct 24, 2015
@tomwilkie tomwilkie changed the title [WIP] Vendor Dependancies Vendor Dependancies Oct 24, 2015
@2opremio
Copy link
Contributor

@tomwilkie Yep, I think it's fine (I just shared the numbers to show how it performed in a dev VM with shared FS).

@peterbourgon
Copy link
Contributor

Works On My Machine™

LGTM

@paulbellamy
Copy link
Contributor

Just trying out gocertifi, quick...

Edit: Gocertifi change looks good.

@paulbellamy
Copy link
Contributor

Also, it runs the scope-probe build every time I do make...

@tomwilkie tomwilkie changed the title Vendor Dependancies Build in a container, use go1.5, vendor Dependancies, build for Darwin and Arm on Circle. Oct 26, 2015
@paulbellamy
Copy link
Contributor

How do you run the tests with this?

go test ./..., rightfully says there are no dependencies... and make test is not a thing.

@paulbellamy
Copy link
Contributor

Also, we should be running go test on circle...

@tomwilkie
Copy link
Contributor Author

tools/test is run on circle; and I've got another PR open to add a make tests.

Peter has been running go test in the subdir he wants, but you're right, this will break that. Tools will need updating for the vendoring.

@paulbellamy
Copy link
Contributor

After f2f discussion, my main concern is that tools/test is not using the vendored deps, or same version of go.

@peterbourgon
Copy link
Contributor

Ah, good catch. The vendoring seems to be incomplete. Gist. Not sure how it's working via make?

Nevermind. I understand now; ./... is catching the vendor directory itself, which is wrong.

@tomwilkie
Copy link
Contributor Author

Its not; circle it using tools/test, which does its own go gets.

I'm just fixing the always-rebuilding-the-exes problem, then I'll look into
this.

On Mon, Oct 26, 2015 at 1:45 PM, Peter Bourgon notifications@github.com
wrote:

Ah, good catch. The vendoring seems to be incomplete. Gist
https://gist.githubusercontent.com/peterbourgon/0666c67d846c3bbc5f3a/raw/0a29e13d561a8f6fcd7e77c93954aefcb6543aa5/gistfile1.txt.
Not sure how it's working via make?


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

@peterbourgon
Copy link
Contributor

Yep. This works fine via

$ env GO15VENDOREXPERIMENT=1 go test ./...

from any of the individual directories. Which is fine for me.

@tomwilkie
Copy link
Contributor Author

@paulbellamy PTAL

I suggest we get this one in then fix the test issues, as its blocking paul fixing the SSL issues.

@paulbellamy
Copy link
Contributor

LGTM, we should probably also apply similar to the frontend build at some point.

Edit: And we should fix circle, so it's not lies, but this is ok to merge.

@paulbellamy
Copy link
Contributor

Actually, I think this is missing a few deps.

Particularly:

# github.com/weaveworks/scope/xfer
xfer/http_publisher_test.go:14:2: cannot find package "github.com/gorilla/handlers" in any of:
    /Users/paulbellamy/gopath/src/github.com/weaveworks/scope/vendor/github.com/gorilla/handlers (vendor tree)
    /usr/local/go/src/github.com/gorilla/handlers (from $GOROOT)
    /Users/paulbellamy/gopath/src/github.com/gorilla/handlers (from $GOPATH)

@peterbourgon
Copy link
Contributor

Apart from a longstanding panic bug when testing probe/endpoint on Darwin, this works fine for me:

ƒ for d in app common probe render report xfer
    pushd (pwd)/$d
    pwd
    env GO15VENDOREXPERIMENT=1 go test -v ./...
    popd
end

@tomwilkie
Copy link
Contributor Author

@paulbellamy I think this one should be good. Mind taking one last look?

@tomwilkie
Copy link
Contributor Author

Last two tools updates were to --no-go-get and no-coverage-for-vendored-packages fixes. Please don't ask me to squash them, I don't seem to be able to do it for subtrees.

@tomwilkie
Copy link
Contributor Author

I count 2 LGTMs; quite a lot has gone in and @paulbellamy promises to give that a look tomorrow, after I has merged.

tomwilkie added a commit that referenced this pull request Oct 26, 2015
Build in a container, use go1.5, vendor Dependancies, build for Darwin and Arm on Circle.
@tomwilkie tomwilkie merged commit 15371a2 into master Oct 26, 2015
@tomwilkie tomwilkie deleted the 535-vendor branch October 26, 2015 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants