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
=== modified file 'environs/tools.go'
--- environs/tools.go 2012-09-21 14:41:58 +0000
+++ environs/tools.go 2012-09-26 02:10:26 +0000
@@ -437,19 +437,28 @@
437 }437 }
438 defer os.RemoveAll(dir)438 defer os.RemoveAll(dir)
439439
440 cmd := exec.Command("go", "install", "launchpad.net/juju-core/cmd/...")440 cmds := [][]string{
441 cmd.Env = setenv(os.Environ(), "GOBIN="+dir)441 {"go", "install", "launchpad.net/juju-core/cmd/jujud"},
442 out, err := cmd.CombinedOutput()442 {"go", "install", "launchpad.net/juju-core/cmd/jujuc"},
443 if err != nil {443 {"strip", dir + "/jujud"},
444 return version.Binary{}, fmt.Errorf("build failed: %v; %s", err, out)444 {"strip", dir + "/jujuc"},
445 }
446 env := setenv(os.Environ(), "GOBIN="+dir)
447 for _, args := range cmds {
448 cmd := exec.Command(args[0], args[1:]...)
449 cmd.Env = env
450 out, err := cmd.CombinedOutput()
451 if err != nil {
452 return version.Binary{}, fmt.Errorf("build failed: %v; %s", err, out)
453 }
445 }454 }
446 if vers != nil {455 if vers != nil {
447 if err := ioutil.WriteFile(filepath.Join(dir, "FORCE-VERSION"), []byte((*vers).String()), 0666); err != nil {456 if err := ioutil.WriteFile(filepath.Join(dir, "FORCE-VERSION"), []byte((*vers).String()), 0666); err != nil {
448 return version.Binary{}, err457 return version.Binary{}, err
449 }458 }
450 }459 }
451 cmd = exec.Command(filepath.Join(dir, "jujud"), "version")460 cmd := exec.Command(filepath.Join(dir, "jujud"), "version")
452 out, err = cmd.CombinedOutput()461 out, err := cmd.CombinedOutput()
453 if err != nil {462 if err != nil {
454 return version.Binary{}, fmt.Errorf("cannot get version from %q: %v; %s", cmd.Args[0], err, out)463 return version.Binary{}, fmt.Errorf("cannot get version from %q: %v; %s", cmd.Args[0], err, out)
455 }464 }
456465
=== 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:10:26 +0000
@@ -60,10 +60,12 @@
60 cmd []string60 cmd []string
61 output string61 output string
62}{62}{
63 // TODO(niemeyer): Reintroduce this once we start deploying to the public bucket.
64 //{
65 // []string{"juju", "arble"},
66 // "error: unrecognized command: juju arble\n",
67 //},
63 {68 {
64 []string{"juju", "arble"},
65 "error: unrecognized command: juju arble\n",
66 }, {
67 []string{"jujud", "arble"},69 []string{"jujud", "arble"},
68 "error: unrecognized command: jujud arble\n",70 "error: unrecognized command: jujud arble\n",
69 }, {71 }, {

Subscribers

People subscribed via source and target branches