juju-core

Merge lp:~niemeyer/juju-core/halve-tools-tarball-size into lp:~juju/juju-core/trunk

  1. halve-tools-tarball-size
  2. Merge into trunk
Proposed by Gustavo Niemeyer
Status: Merged
Merged at revision: 578
Proposed branch: lp:~niemeyer/juju-core/halve-tools-tarball-size
Merge into: lp:~juju/juju-core/trunk
Diff against target: 59 lines (+21/-10)
2 files modified
environs/tools.go (+16/-7)
environs/tools_test.go (+5/-3)
To merge this branch: bzr merge lp:~niemeyer/juju-core/halve-tools-tarball-size
Reviewer Review Type Date Requested Status
The Go Language Gophers Pending
Review via email: mp+126364@code.launchpad.net

Description of the change

environs: halve tools tarball size

Strip jujud and jujuc and do not add the "juju" command
to the tarball. This drops the tarball size by 50%+.

https://codereview.appspot.com/6579043/

To post a comment you must log in.
Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

Reviewers: mp+126364_code.launchpad.net,

Message:
Please take a look.

Description:
environs: halve tools tarball size

Strip jujud and jujuc and do not add the "juju" command
to the tarball. This drops the tarball size by 50%+.

https://code.launchpad.net/~niemeyer/juju-core/halve-tools-tarball-size/+merge/126364

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M environs/tools.go
   M environs/tools_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: environs/tools.go
=== modified file 'environs/tools.go'
--- environs/tools.go 2012-09-21 14:41:58 +0000
+++ environs/tools.go 2012-09-26 02:08:23 +0000
@@ -437,19 +437,28 @@
   }
   defer os.RemoveAll(dir)

- cmd := exec.Command("go", "install", "launchpad.net/juju-core/cmd/...")
- cmd.Env = setenv(os.Environ(), "GOBIN="+dir)
- out, err := cmd.CombinedOutput()
- if err != nil {
- return version.Binary{}, fmt.Errorf("build failed: %v; %s", err, out)
+ cmds := [][]string{
+ {"go", "install", "launchpad.net/juju-core/cmd/jujud"},
+ {"go", "install", "launchpad.net/juju-core/cmd/jujuc"},
+ {"strip", dir + "/jujud"},
+ {"strip", dir + "/jujuc"},
+ }
+ env := setenv(os.Environ(), "GOBIN="+dir)
+ for _, args := range cmds {
+ cmd := exec.Command(args[0], args[1:]...)
+ cmd.Env = env
+ out, err := cmd.CombinedOutput()
+ if err != nil {
+ return version.Binary{}, fmt.Errorf("build failed: %v; %s", err, out)
+ }
   }
   if vers != nil {
    if err := ioutil.WriteFile(filepath.Join(dir, "FORCE-VERSION"),
[]byte((*vers).String()), 0666); err != nil {
     return version.Binary{}, err
    }
   }
- cmd = exec.Command(filepath.Join(dir, "jujud"), "version")
- out, err = cmd.CombinedOutput()
+ cmd := exec.Command(filepath.Join(dir, "jujud"), "version")
+ out, err := cmd.CombinedOutput()
   if err != nil {
    return version.Binary{}, fmt.Errorf("cannot get version
from %q: %v; %s", cmd.Args[0], err, out)
   }

Index: environs/tools_test.go
=== modified file 'environs/tools_test.go'
--- environs/tools_test.go 2012-09-21 11:45:08 +0000
+++ environs/tools_test.go 2012-09-26 02:08:23 +0000
@@ -60,10 +60,12 @@
   cmd []string
   output string
  }{
+ // TODO(niemeyer): Reintroduce this once we start deploying to the public
bucket.
+ //{
+ // []string{"juju", "arble"},
+ // "error: unrecognized command: juju arble\n",
+ //},
   {
- []string{"juju", "arble"},
- "error: unrecognized command: juju arble\n",
- }, {
    []string{"jujud", "arble"},
    "error: unrecognized command: jujud arble\n",
   }, {

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

*** Submitted:

environs: halve tools tarball size

Strip jujud and jujuc and do not add the "juju" command
to the tarball. This drops the tarball size by 50%+.

R=dfc
CC=
https://codereview.appspot.com/6579043

https://codereview.appspot.com/6579043/diff/1/environs/tools_test.go
File environs/tools_test.go (right):

https://codereview.appspot.com/6579043/diff/1/environs/tools_test.go#newcode63
environs/tools_test.go:63: // TODO(niemeyer): Reintroduce this once we
start deploying to the public bucket.
On 2012/09/26 02:15:00, dfc wrote:
> Why would we ever way to push the client tool into the environment ?

It seems like a good idea to have available the client tool that can
talk to such an environment.

https://codereview.appspot.com/6579043/

Revision history for this message
Roger Peppe (rogpeppe) wrote :

i see you've submitted, but a couple of comments for the record.

https://codereview.appspot.com/6579043/diff/4002/environs/tools.go
File environs/tools.go (right):

https://codereview.appspot.com/6579043/diff/4002/environs/tools.go#newcode442
environs/tools.go:442: {"go", "install",
"launchpad.net/juju-core/cmd/jujuc"},
better to have both commands installed in the same go command - that way
they share work.

similarly, you can pass two args to strip, although it probably doesn't
have the same benefits.

https://codereview.appspot.com/6579043/diff/4002/environs/tools.go#newcode452
environs/tools.go:452: return version.Binary{}, fmt.Errorf("build
failed: %v; %s", err, out)
it would be nice to know which command failed.

https://codereview.appspot.com/6579043/

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

Done: https://codereview.appspot.com/6574049

https://codereview.appspot.com/6579043/diff/4002/environs/tools.go
File environs/tools.go (right):

https://codereview.appspot.com/6579043/diff/4002/environs/tools.go#newcode442
environs/tools.go:442: {"go", "install",
"launchpad.net/juju-core/cmd/jujuc"},
On 2012/09/26 07:48:36, rog wrote:
> better to have both commands installed in the same go command - that
 way they
> share work.

> similarly, you can pass two args to strip, although it probably
doesn't have the
> same benefits.

Done.

https://codereview.appspot.com/6579043/diff/4002/environs/tools.go#newcode452
environs/tools.go:452: return version.Binary{}, fmt.Errorf("build
failed: %v; %s", err, out)
On 2012/09/26 07:48:36, rog wrote:
> it would be nice to know which command failed.

Done.

https://codereview.appspot.com/6579043/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'environs/tools.go'
2--- environs/tools.go 2012-09-21 14:41:58 +0000
3+++ environs/tools.go 2012-09-26 02:10:26 +0000
4@@ -437,19 +437,28 @@
5 }
6 defer os.RemoveAll(dir)
7
8- cmd := exec.Command("go", "install", "launchpad.net/juju-core/cmd/...")
9- cmd.Env = setenv(os.Environ(), "GOBIN="+dir)
10- out, err := cmd.CombinedOutput()
11- if err != nil {
12- return version.Binary{}, fmt.Errorf("build failed: %v; %s", err, out)
13+ cmds := [][]string{
14+ {"go", "install", "launchpad.net/juju-core/cmd/jujud"},
15+ {"go", "install", "launchpad.net/juju-core/cmd/jujuc"},
16+ {"strip", dir + "/jujud"},
17+ {"strip", dir + "/jujuc"},
18+ }
19+ env := setenv(os.Environ(), "GOBIN="+dir)
20+ for _, args := range cmds {
21+ cmd := exec.Command(args[0], args[1:]...)
22+ cmd.Env = env
23+ out, err := cmd.CombinedOutput()
24+ if err != nil {
25+ return version.Binary{}, fmt.Errorf("build failed: %v; %s", err, out)
26+ }
27 }
28 if vers != nil {
29 if err := ioutil.WriteFile(filepath.Join(dir, "FORCE-VERSION"), []byte((*vers).String()), 0666); err != nil {
30 return version.Binary{}, err
31 }
32 }
33- cmd = exec.Command(filepath.Join(dir, "jujud"), "version")
34- out, err = cmd.CombinedOutput()
35+ cmd := exec.Command(filepath.Join(dir, "jujud"), "version")
36+ out, err := cmd.CombinedOutput()
37 if err != nil {
38 return version.Binary{}, fmt.Errorf("cannot get version from %q: %v; %s", cmd.Args[0], err, out)
39 }
40
41=== modified file 'environs/tools_test.go'
42--- environs/tools_test.go 2012-09-21 11:45:08 +0000
43+++ environs/tools_test.go 2012-09-26 02:10:26 +0000
44@@ -60,10 +60,12 @@
45 cmd []string
46 output string
47 }{
48+ // TODO(niemeyer): Reintroduce this once we start deploying to the public bucket.
49+ //{
50+ // []string{"juju", "arble"},
51+ // "error: unrecognized command: juju arble\n",
52+ //},
53 {
54- []string{"juju", "arble"},
55- "error: unrecognized command: juju arble\n",
56- }, {
57 []string{"jujud", "arble"},
58 "error: unrecognized command: jujud arble\n",
59 }, {

Subscribers

People subscribed via source and target branches