Merge lp:~thumper/juju-core/find-jujud into lp:~go-bot/juju-core/trunk

[Tim Penhey]

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.

https://codereview.appspot.com/11326043/

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://codereview.appspot.com/11326043/

[Tim Penhey (thumper)]

Reviewers: mp+174918_code.launchpad.net,

Message:
Please take a look.

Description:
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)

Please review this at https://codereview.appspot.com/11326043/

Affected files:
   A [revision details]
   M environs/localstorage/storage.go
   M environs/tools/build.go
   M environs/tools/storage.go

[Martin Packman (gz)]

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.go#newcode95
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#newcode112
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#newcode131
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#newcode161
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#newcode49
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.

https://codereview.appspot.com/11326043/

[William Reade (fwereade)]

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.go#newcode95
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#newcode131
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.

https://codereview.appspot.com/11326043/

[William Reade (fwereade)]

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#newcode212
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.

https://codereview.appspot.com/11326043/

[Tim Penhey (thumper)]

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.go#newcode95
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#newcode131
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#newcode212
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#newcode49
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.

https://codereview.appspot.com/11326043/

[Tim Penhey (thumper)]

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#newcode112
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#newcode161
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.

https://codereview.appspot.com/11326043/

=== modified file 'environs/localstorage/storage.go'
--- environs/localstorage/storage.go	2013-07-10 05:07:08 +0000
+++ environs/localstorage/storage.go	2013-07-17 01:41:27 +0000
@@ -88,7 +88,13 @@
 	if err != nil {
 		return err
 	}
-	req, err := http.NewRequest("PUT", url, r)
+	// Here we wrap up the reader.  For some freaky unexplainable reason, the
+	// http library will call Close on the reader if it has a Close method
+	// available.  Since we sometimes reuse the reader, especially when
+	// putting tools, we don't want Close called.  So we wrap the reader in a
+	// struct so the Close method is not exposed.
+	justReader := struct{ io.Reader }{r}
+	req, err := http.NewRequest("PUT", url, justReader)
 	if err != nil {
 		return err
 	}

=== modified file 'environs/localstorage/storage_test.go'
--- environs/localstorage/storage_test.go	2013-07-10 05:07:08 +0000
+++ environs/localstorage/storage_test.go	2013-07-17 01:41:27 +0000
@@ -6,6 +6,7 @@
 import (
 	"bytes"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"net/http"
 
@@ -14,6 +15,7 @@
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/localstorage"
 	"launchpad.net/juju-core/errors"
+	jc "launchpad.net/juju-core/testing/checkers"
 )
 
 type storageSuite struct{}
@@ -77,10 +79,25 @@
 	c.Assert(lnames, DeepEquals, names)
 }
 
+type readerWithClose struct {
+	*bytes.Buffer
+	closeCalled bool
+}
+
+var _ io.Reader = (*readerWithClose)(nil)
+var _ io.Closer = (*readerWithClose)(nil)
+
+func (r *readerWithClose) Close() error {
+	r.closeCalled = true
+	return nil
+}
+
 func checkPutFile(c *C, storage environs.StorageWriter, name string, contents []byte) {
 	c.Logf("check putting file %s ...", name)
-	err := storage.Put(name, bytes.NewBuffer(contents), int64(len(contents)))
+	reader := &readerWithClose{bytes.NewBuffer(contents), false}
+	err := storage.Put(name, reader, int64(len(contents)))
 	c.Assert(err, IsNil)
+	c.Assert(reader.closeCalled, jc.IsFalse)
 }
 
 func checkFileDoesNotExist(c *C, storage environs.StorageReader, name string) {

=== modified file 'environs/tools/build.go'
--- environs/tools/build.go	2013-05-02 15:55:42 +0000
+++ environs/tools/build.go	2013-07-17 01:41:27 +0000
@@ -9,13 +9,18 @@
 	"fmt"
 	"io"
 	"io/ioutil"
-	"launchpad.net/juju-core/version"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"strings"
+
+	"launchpad.net/loggo"
+
+	"launchpad.net/juju-core/version"
 )
 
+var logger = loggo.GetLogger("juju.environs.tools")
+
 // archive writes the executable files found in the given directory in
 // gzipped tar format to w.  An error is returned if an entry inside dir
 // is not a regular executable file.
@@ -33,6 +38,7 @@
 
 	for _, ent := range entries {
 		h := tarHeader(ent)
+		logger.Debugf("adding entry: %#v", h)
 		// ignore local umask
 		if isExecutable(ent) {
 			h.Mode = 0755
@@ -103,17 +109,80 @@
 	return append(env, val)
 }
 
-// bundleTools bundles all the current juju tools in gzipped tar
-// format to the given writer.
-// If forceVersion is not nil, a FORCE-VERSION file is included in
-// the tools bundle so it will lie about its current version number.
-func bundleTools(w io.Writer, forceVersion *version.Number) (version.Binary, error) {
-	dir, err := ioutil.TempDir("", "juju-tools")
-	if err != nil {
-		return version.Binary{}, err
-	}
-	defer os.RemoveAll(dir)
-
+func findExecutable(execFile string) (string, error) {
+	logger.Debugf("looking for: %s", execFile)
+	if filepath.IsAbs(execFile) {
+		return execFile, nil
+	}
+
+	dir, file := filepath.Split(execFile)
+
+	// Now we have two possibilities:
+	//   file == path indicating that the PATH was searched
+	//   dir != "" indicating that it is a relative path
+
+	if dir == "" {
+		path := os.Getenv("PATH")
+		for _, name := range filepath.SplitList(path) {
+			result := filepath.Join(name, file)
+			info, err := os.Stat(result)
+			if err == nil {
+				// Sanity check to see if executable.
+				if info.Mode()&0111 != 0 {
+					return result, nil
+				}
+			}
+		}
+
+		return "", fmt.Errorf("could not find %q in the path", file)
+	}
+	cwd, err := os.Getwd()
+	if err != nil {
+		return "", err
+	}
+	return filepath.Clean(filepath.Join(cwd, execFile)), nil
+}
+
+func copyExistingJujud(dir string) error {
+	// Assume that the user is running juju.
+	jujuLocation, err := findExecutable(os.Args[0])
+	if err != nil {
+		logger.Infof("%v", err)
+		return err
+	}
+	jujudLocation := filepath.Join(filepath.Dir(jujuLocation), "jujud")
+	logger.Debugf("checking: %s", jujudLocation)
+	info, err := os.Stat(jujudLocation)
+	if err != nil {
+		logger.Infof("couldn't find existing jujud")
+		return err
+	}
+	logger.Infof("found existing jujud")
+	// TODO(thumper): break this out into a util function.
+	// copy the file into the dir.
+	source, err := os.Open(jujudLocation)
+	if err != nil {
+		logger.Infof("open source failed: %v", err)
+		return err
+	}
+	defer source.Close()
+	target := filepath.Join(dir, "jujud")
+	logger.Infof("target: %v", target)
+	destination, err := os.OpenFile(target, os.O_RDWR|os.O_TRUNC|os.O_CREATE, info.Mode())
+	if err != nil {
+		logger.Infof("open destination failed: %v", err)
+		return err
+	}
+	defer destination.Close()
+	_, err = io.Copy(destination, source)
+	if err != nil {
+		return err
+	}
+	return nil
+}
+
+func buildJujud(dir string) error {
+	logger.Infof("building jujud")
 	cmds := [][]string{
 		{"go", "install", "launchpad.net/juju-core/cmd/jujud"},
 		{"strip", dir + "/jujud"},
@@ -124,10 +193,32 @@
 		cmd.Env = env
 		out, err := cmd.CombinedOutput()
 		if err != nil {
-			return version.Binary{}, fmt.Errorf("build command %q failed: %v; %s", args[0], err, out)
-		}
-	}
+			return fmt.Errorf("build command %q failed: %v; %s", args[0], err, out)
+		}
+	}
+	return nil
+}
+
+// bundleTools bundles all the current juju tools in gzipped tar
+// format to the given writer.
+// If forceVersion is not nil, a FORCE-VERSION file is included in
+// the tools bundle so it will lie about its current version number.
+func bundleTools(w io.Writer, forceVersion *version.Number) (version.Binary, error) {
+	dir, err := ioutil.TempDir("", "juju-tools")
+	if err != nil {
+		return version.Binary{}, err
+	}
+	defer os.RemoveAll(dir)
+
+	if err := copyExistingJujud(dir); err != nil {
+		logger.Debugf("copy existing failed: %v", err)
+		if err := buildJujud(dir); err != nil {
+			return version.Binary{}, err
+		}
+	}
+
 	if forceVersion != nil {
+		logger.Debugf("forcing version to %s", forceVersion)
 		if err := ioutil.WriteFile(filepath.Join(dir, "FORCE-VERSION"), []byte(forceVersion.String()), 0666); err != nil {
 			return version.Binary{}, err
 		}

=== added file 'environs/tools/build_test.go'
--- environs/tools/build_test.go	1970-01-01 00:00:00 +0000
+++ environs/tools/build_test.go	2013-07-17 01:41:27 +0000
@@ -0,0 +1,93 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package tools_test
+
+import (
+	"fmt"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/environs/tools"
+	"launchpad.net/juju-core/testing"
+)
+
+type buildSuite struct {
+	testing.LoggingSuite
+	restore  func()
+	cwd      string
+	filePath string
+}
+
+var _ = gc.Suite(&buildSuite{})
+
+func (b *buildSuite) SetUpTest(c *gc.C) {
+	b.LoggingSuite.SetUpTest(c)
+
+	dir1 := c.MkDir()
+	dir2 := c.MkDir()
+
+	c.Log(dir1)
+	c.Log(dir2)
+
+	path := os.Getenv("PATH")
+	os.Setenv("PATH", fmt.Sprintf("%s:%s:%s", dir1, dir2, path))
+
+	// Make an executable file called "juju-test" in dir2.
+	b.filePath = filepath.Join(dir2, "juju-test")
+	err := ioutil.WriteFile(
+		b.filePath,
+		[]byte("doesn't matter, we don't execute it"),
+		0755)
+	c.Assert(err, gc.IsNil)
+
+	cwd, err := os.Getwd()
+	c.Assert(err, gc.IsNil)
+
+	b.cwd = c.MkDir()
+	err = os.Chdir(b.cwd)
+	c.Assert(err, gc.IsNil)
+
+	b.restore = func() {
+		os.Setenv("PATH", path)
+		os.Chdir(cwd)
+	}
+}
+
+func (b *buildSuite) TearDownTest(c *gc.C) {
+	b.restore()
+	b.LoggingSuite.TearDownTest(c)
+}
+
+func (b *buildSuite) TestFindExecutable(c *gc.C) {
+
+	for _, test := range []struct {
+		execFile   string
+		expected   string
+		errorMatch string
+	}{{
+		execFile: "/some/absolute/path",
+		expected: "/some/absolute/path",
+	}, {
+		execFile: "./foo",
+		expected: filepath.Join(b.cwd, "foo"),
+	}, {
+		execFile: "juju-test",
+		expected: b.filePath,
+	}, {
+		execFile:   "non-existent-exec-file",
+		errorMatch: `could not find "non-existent-exec-file" in the path`,
+	}} {
+		result, err := tools.FindExecutable(test.execFile)
+		if test.errorMatch == "" {
+			c.Assert(err, gc.IsNil)
+			c.Assert(result, gc.Equals, test.expected)
+		} else {
+			c.Assert(err, gc.ErrorMatches, test.errorMatch)
+			c.Assert(result, gc.Equals, "")
+		}
+	}
+}

=== modified file 'environs/tools/export_test.go'
--- environs/tools/export_test.go	2013-05-02 15:55:42 +0000
+++ environs/tools/export_test.go	2013-07-17 01:41:27 +0000
@@ -4,3 +4,5 @@
 package tools
 
 var Setenv = setenv
+
+var FindExecutable = findExecutable

=== modified file 'environs/tools/storage.go'
--- environs/tools/storage.go	2013-05-02 15:55:42 +0000
+++ environs/tools/storage.go	2013-07-17 01:41:27 +0000
@@ -8,11 +8,11 @@
 	"fmt"
 	"io"
 	"io/ioutil"
-	"launchpad.net/juju-core/log"
+	"os"
+	"strings"
+
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/version"
-	"os"
-	"strings"
 )
 
 var ErrNoTools = errors.New("no tools available")
@@ -46,7 +46,7 @@
 // ReadList returns a List of the tools in store with the given major version.
 // If store contains no such tools, it returns ErrNoMatches.
 func ReadList(storage URLLister, majorVersion int) (List, error) {
-	log.Debugf("environs/tools: reading v%d.* tools", majorVersion)
+	logger.Debugf("reading v%d.* tools", majorVersion)
 	names, err := storage.List(toolPrefix)
 	if err != nil {
 		return nil, err
@@ -66,7 +66,7 @@
 		if t.Major != majorVersion {
 			continue
 		}
-		log.Debugf("environs/tools: found %s", vers)
+		logger.Debugf("found %s", vers)
 		if t.URL, err = storage.URL(name); err != nil {
 			return nil, err
 		}
@@ -99,6 +99,7 @@
 	// TODO(rog) find binaries from $PATH when not using a development
 	// version of juju within a $GOPATH.
 
+	logger.Debugf("Uploading tools for %v", fakeSeries)
 	// We create the entire archive before asking the environment to
 	// start uploading so that we can be sure we have archived
 	// correctly.
@@ -117,13 +118,13 @@
 		return nil, fmt.Errorf("cannot stat newly made tools archive: %v", err)
 	}
 	size := fileInfo.Size()
-	log.Infof("environs/tools: built %v (%dkB)", toolsVersion, (size+512)/1024)
+	logger.Infof("built %v (%dkB)", toolsVersion, (size+512)/1024)
 	putTools := func(vers version.Binary) (string, error) {
 		if _, err := f.Seek(0, 0); err != nil {
 			return "", fmt.Errorf("cannot seek to start of tools archive: %v", err)
 		}
 		name := StorageName(vers)
-		log.Infof("environs/tools: uploading %s", vers)
+		logger.Infof("uploading %s", vers)
 		if err := storage.Put(name, f, size); err != nil {
 			return "", err
 		}