Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(219)

Issue 6579043: environs: halve tools tarball size

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 6 months ago by niemeyer
Modified:
11 years, 6 months ago
Reviewers:
mp+126364, dfc, rog
Visibility:
Public.

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)

Patch Set 1 #

Total comments: 3

Patch Set 2 : environs: halve tools tarball size #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+23 lines, -10 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M environs/tools.go View 1 chunk +16 lines, -7 lines 4 comments Download
M environs/tools_test.go View 1 chunk +5 lines, -3 lines 0 comments Download

Messages

Total messages: 5
niemeyer
Please take a look.
11 years, 6 months ago (2012-09-26 02:09:35 UTC) #1
dfc
LGTM. Not pushing cmd/juju alone is worth the saving. https://codereview.appspot.com/6579043/diff/1/environs/tools.go File environs/tools.go (right): https://codereview.appspot.com/6579043/diff/1/environs/tools.go#newcode441 environs/tools.go:441: ...
11 years, 6 months ago (2012-09-26 02:15:00 UTC) #2
niemeyer
*** Submitted: environs: halve tools tarball size Strip jujud and jujuc and do not add ...
11 years, 6 months ago (2012-09-26 02:33:24 UTC) #3
rog
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 ...
11 years, 6 months ago (2012-09-26 07:48:36 UTC) #4
niemeyer
11 years, 6 months ago (2012-09-26 15:48:48 UTC) #5
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b