Fwd: Various small cleanups (issue 7005044)

Gary Poster gary.poster at canonical.com
Fri Dec 21 02:52:04 UTC 2012

Hey Nicola, Francesco and Ben.  This branch has some bits that might be 
particularly interesting to you.  Note that, although the diff is 
gigantic at 5000 lines, the "real" diff is more like a bit over 400 
lines: I checked in some gallery files, as I discuss below (mentioning 
other options).

Ben, the only part you might care about in particular is that I *think* 
I addressed the "make test-prod" issue.  I *think* I did it with a 
console.debug addition to our dummy console.  But anyway, prod tests 
seem to pass on my branch, even after clearing the cache.  You don't 
have to read any more now. Bye. ;-)

Francesco and Nicola, I'd like for you guys to either review it, maybe 
with small changes for me, and I'll land it tomorrow when I have a spare 
moment; *or* take this branch as inspiration and do everything better. 
:-)  I have no great attachment to how I made these changes, so if you 
want to do them differently that's cool by me.

There is a card for bug 1092199 in Review right now that represents this 
branch.  Feel free to do what you will with it--delete it, own it, 
subdivide it, or...just approve it. :-)



-------- Original Message --------
Subject: Various small cleanups (issue 7005044)
Date: Fri, 21 Dec 2012 02:37:16 +0000
From: gary.poster at canonical.com
Reply-To: gary.poster at canonical.com, mp+141022 at code.launchpad.net, 
   reply at codereview-hr.appspotmail.com
To: mp+141022 at code.launchpad.net
CC: reply at codereview-hr.appspotmail.com

Reviewers: mp+141022_code.launchpad.net,

Please take a look.

Various small cleanups

This branch has a little for everyone.  I did it during spare moments
today to see if I could address various issues I saw and had heard
about.  I think that landing this branch would be fine, but I also would
be happy enough if people wanted to extract bits from it on a piecemeal
basis instead.  I'm sorry that everything is mixed together, but they
are small changes.  I can extract, or ask other people to, if necessary.

I'm going to fully explain the changes.  This will help me remember.  It
will also probably be painful to read.  Sorry.

For Nicola, and for those with a masochistic enjoyment of Makefile
churn, I addressed bug 1092199: "Makefile build target is misnamed."
See the bug for rationale and for the plan, which I followed except that
I used "build-shared" instead of "build-common" as the new name for the
former "build" directory.  This "build-shared" choice, like a couple of
other choices in this branch, was fairly arbitrary.  I'd rather not
change it, but that's because of time and inertia.  If I'd reread the
bug before starting I probably would have used build-common.  Fixes for
this bug account for the vast majority of the Makefile changes.  The two
exceptions are the change to the JSFILES variable definition, and the
removal of the skin asset Make dependencies for build-prod and
build-devel.  I'll explain those in a moment.  This bug also accounts
for many of the changes in bin/merge-files, and various other places
where you see s/build/build-shared/.

Thanks to the diagnosis and discussion from Nicola, Francesco and Kapil,
I addressed one of the remaining parts of bug 1083920, "Charm should
serve the GUI assets over HTTPS".  Francesco identified two insecure
sources we were having trouble with when we served over HTTPS.  One
insecure connection was the charm store.  Kapil is working on getting a
cert for the charm store, so we can simply use HTTPS for our charm store
URLs.  The other insecure connection is that we were still relying on a
number of files from the Yahoo CDN.  In prod, we relied on the HTTP-only
CDN for three Gallery files, and in debug (and devel) we relied on the
CDN for many files in addition.  I fixed the way we serve our files in
devel, debug and prod, so that all YUI files are served locally.  The
main step for this was to get the gallery files stored locally.  I could
have done this many ways.  One choice could have been to use the npm
yui-gallery package, but it is quite old so I rejected it.  I also could
have gotten all the gallery files from git as part of the build process.
  That would have been fine, as would have other approaches, but I went
for an explicit and simple approach: I downloaded the files we needed
into app/assets/javascripts and told the debug modules file where to
find them and told the minifier to include them.  This also required
some tweaks to the Makefile's JSFILES calculation, to exclude those

With help from Benji, I updated the reviewer documentation and the
release documentation to reflect the current state of things.  Hopefully
those changes are self-explanatory, and if they are not, we should
probably fix them so they are!

I removed some unnecessarily duplicated and ignored topology "requires"
in the modules-debug.js file, as requested by a comment at the top of
that file and as blessed by Ben.

More valuably, I seem to have fixed the errors we saw in ``make
test-prod`` in Ben's recently landed branch.  I believe it was because
of the fake console missing a "debug" call.  In any case, I was having
trouble with prod, and now it is working for me, and the prod tests seem
fine too.

I tweaked Benji's Makefile change that had switched back from linking to
copying YUI files into the build directories.  The old symlinks had put
the skin file assets in the wrong place.  When Benji fixed the assets,
he didn't notice that the Makefile said that it depended on the wrong
location of those files.  This meant that every time you ran make, the
Makefile would rerun the linking code, in the vain attempt to create
those incorrectly-placed files.  Rather than include all skin asset
locations, I simply removed those two dependencies ("night" and "sam").
Now the Makefile is as quiet as desired on repeated runs of "make,"
"make prod," "make debug," and "make devel."

OK, that's it.  As I said, landing this seems OK, but feel free to steal
from it instead.  Please poke around both at the Makefile and the app to
make sure nothing seems to have regressed.  Thank you!


(do not edit description out of merge proposal)

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

Affected files:
   M .bzrignore
   M Makefile
   A [revision details]
   A app/assets/javascripts/gallery-ellipsis-debug.js
   A app/assets/javascripts/gallery-ellipsis.js
   A app/assets/javascripts/gallery-markdown-debug.js
   A app/assets/javascripts/gallery-markdown.js
   A app/assets/javascripts/gallery-timer-debug.js
   A app/assets/javascripts/gallery-timer.js
   M app/modules-debug.js
   M app/views/utils.js
   M bin/merge-files
   M docs/process.rst
   M grunt.js
   M lib/server.js
   M lib/templates.js

More information about the Juju-GUI mailing list