Use pre-built jujud if found.

Use pre-built jujud if found.

Instead of rebuilding jujud everytime we want to upload some tools, try to
find jujud next to the juju being executed.

When we are uploading tools, we are running the juju command.  The
juju executable is normally (when we build it and in the package
version) next to the jujud executable.  So, what we do is start with
os.Args[0] and try to find the full path for it.  There are three
situations here:
 1. arg[0] is an absolute path, if so, return it
 2. arg[0] is a relative path, if so, join it to the current working
   directory, clean that and return it
 3. arg[0] is just the command, in which case, walk the path looking
   for an executable file that matches.

Amazingly, there is no os.CopyFile, so you have to mangle it yourself.

https://code.launchpad.net/~thumper/juju-core/find-jujud/+merge/174918

Requires: https://code.launchpad.net/~thumper/juju-core/local-provider-bootstrap/+merge/174913

(do not edit description out of merge proposal)

[thumper, 2013-07-16 04:25:46]

Please take a look.

[gz, 2013-07-16 09:43:00]

LGTM

https://codereview.appspot.com/11326043/diff/1/environs/localstorage/storage.go
File environs/localstorage/storage.go (right):

https://codereview.appspot.com/11326043/diff/1/environs/localstorage/storage....
environs/localstorage/storage.go:95: // struct so the Close method is not
exposed.
Unrelated drive-by? Kind of thing I'd like a failing test for.

https://codereview.appspot.com/11326043/diff/1/environs/tools/build.go
File environs/tools/build.go (right):

https://codereview.appspot.com/11326043/diff/1/environs/tools/build.go#newcod...
environs/tools/build.go:112: func findExecutable(execFile string) (string,
error) {
Would like some tests for this function.

https://codereview.appspot.com/11326043/diff/1/environs/tools/build.go#newcod...
environs/tools/build.go:131: if info.Mode()&0111 != 0 {
I like spaces even around bitops.

https://codereview.appspot.com/11326043/diff/1/environs/tools/build.go#newcod...
environs/tools/build.go:161: // copy the file into the dir.
I'd factor this out to a seperate function. Just 'cos go doesn't include it
doesn't mean we can't reuse it.

https://codereview.appspot.com/11326043/diff/1/environs/tools/storage.go
File environs/tools/storage.go (right):

https://codereview.appspot.com/11326043/diff/1/environs/tools/storage.go#newc...
environs/tools/storage.go:49: logger.Debugf("reading v%d.* tools", majorVersion)
Having logger declared in build.go and used without import in storage.go is
slightly confusing. I will get my head around the go package model at some
point.

[fwereade, 2013-07-16 11:09:50]

LGTM with mgz's comments taken into account

https://codereview.appspot.com/11326043/diff/1/environs/localstorage/storage.go
File environs/localstorage/storage.go (right):

https://codereview.appspot.com/11326043/diff/1/environs/localstorage/storage....
environs/localstorage/storage.go:95: // struct so the Close method is not
exposed.
On 2013/07/16 09:43:00, gz wrote:
> Unrelated drive-by? Kind of thing I'd like a failing test for.

+1

https://codereview.appspot.com/11326043/diff/1/environs/tools/build.go
File environs/tools/build.go (right):

https://codereview.appspot.com/11326043/diff/1/environs/tools/build.go#newcod...
environs/tools/build.go:131: if info.Mode()&0111 != 0 {
On 2013/07/16 09:43:00, gz wrote:
> I like spaces even around bitops.

I think go fmt decides that for you.

[fwereade, 2013-07-16 11:21:32]

https://codereview.appspot.com/11326043/diff/1/environs/tools/build.go
File environs/tools/build.go (right):

https://codereview.appspot.com/11326043/diff/1/environs/tools/build.go#newcod...
environs/tools/build.go:212: if err := copyExistingJujud(dir); err != nil {
Actually, this has percolated a bit and I'm not so happy. This is quite a big
and magical behaviour change... I have on a few occasions needed/wanted to
upload tools that *don't* match the juju binary I'm using, and this kinda breaks
that. Can still be worked around; and should probably only affect developers;
but I'm starting to think that it's wrong to piggyback on the upload-tools
mechanism for this use case.

I should be around tonight to talk about it a little... my big concern is that
we're changing behavior that *will* affect all the users who are using
upload-tools. Maybe I can be convinced that it's only a little, and maybe even a
beneficial, change; but let's discuss tonight.

[thumper, 2013-07-17 01:02:31]

Working on writing extra tests now.

https://codereview.appspot.com/11326043/diff/1/environs/localstorage/storage.go
File environs/localstorage/storage.go (right):

https://codereview.appspot.com/11326043/diff/1/environs/localstorage/storage....
environs/localstorage/storage.go:95: // struct so the Close method is not
exposed.
On 2013/07/16 11:09:50, fwereade wrote:
> On 2013/07/16 09:43:00, gz wrote:
> > Unrelated drive-by? Kind of thing I'd like a failing test for.
> 
> +1

Was needed as part of this branch as Putting from a file into storage, which has
a Close method, caused the Seek(0,0) as the localstorage used http.NewRequest,
which had the side effect of closing the file.

I've added a test now to confirm that Close isn't called.

https://codereview.appspot.com/11326043/diff/1/environs/tools/build.go
File environs/tools/build.go (right):

https://codereview.appspot.com/11326043/diff/1/environs/tools/build.go#newcod...
environs/tools/build.go:131: if info.Mode()&0111 != 0 {
On 2013/07/16 11:09:50, fwereade wrote:
> On 2013/07/16 09:43:00, gz wrote:
> > I like spaces even around bitops.
> 
> I think go fmt decides that for you.

I like spaces too, but go fmt doesn't.

https://codereview.appspot.com/11326043/diff/1/environs/tools/build.go#newcod...
environs/tools/build.go:212: if err := copyExistingJujud(dir); err != nil {
On 2013/07/16 11:21:33, fwereade wrote:
> Actually, this has percolated a bit and I'm not so happy. This is quite a big
> and magical behaviour change... I have on a few occasions needed/wanted to
> upload tools that *don't* match the juju binary I'm using, and this kinda
breaks
> that. Can still be worked around; and should probably only affect developers;
> but I'm starting to think that it's wrong to piggyback on the upload-tools
> mechanism for this use case.
> 
> I should be around tonight to talk about it a little... my big concern is that
> we're changing behavior that *will* affect all the users who are using
> upload-tools. Maybe I can be convinced that it's only a little, and maybe even
a
> beneficial, change; but let's discuss tonight.

As discussed, we'll go with this for now.

https://codereview.appspot.com/11326043/diff/1/environs/tools/storage.go
File environs/tools/storage.go (right):

https://codereview.appspot.com/11326043/diff/1/environs/tools/storage.go#newc...
environs/tools/storage.go:49: logger.Debugf("reading v%d.* tools", majorVersion)
On 2013/07/16 09:43:00, gz wrote:
> Having logger declared in build.go and used without import in storage.go is
> slightly confusing. I will get my head around the go package model at some
> point.

Yeah.  I'd really prefer the ability to have file local variables, but go
doesn't do that.

[thumper, 2013-07-17 01:41:52]

Please take a look.

https://codereview.appspot.com/11326043/diff/1/environs/tools/build.go
File environs/tools/build.go (right):

https://codereview.appspot.com/11326043/diff/1/environs/tools/build.go#newcod...
environs/tools/build.go:112: func findExecutable(execFile string) (string,
error) {
On 2013/07/16 09:43:00, gz wrote:
> Would like some tests for this function.

Done.

https://codereview.appspot.com/11326043/diff/1/environs/tools/build.go#newcod...
environs/tools/build.go:161: // copy the file into the dir.
On 2013/07/16 09:43:00, gz wrote:
> I'd factor this out to a seperate function. Just 'cos go doesn't include it
> doesn't mean we can't reuse it.

I'll add a TODO.