Make target for appcache manifest

Make target for appcache manifest

Install target now depends on an appcache target, which updates a timestamp in app/assets/manifest.appcache, thus forcing an update in the application cache.

https://code.launchpad.net/~makyo/juju-gui/make-appcache/+merge/131586

(do not edit description out of merge proposal)

[matthew.scott, 2012-10-26 10:55:53]

Please take a look.

[matthew.scott, 2012-10-26 11:06:53]

Please take a look.

[matthew.scott, 2012-10-26 11:08:30]

https://codereview.appspot.com/6776052/diff/5/Makefile
File Makefile (right):

https://codereview.appspot.com/6776052/diff/5/Makefile#newcode19
Makefile:19: install: appcache $(NODE_TARGETS) app/templates.js
Typo fixed.

[gary.poster, 2012-10-28 12:25:14]

Hey Matt.  This looks good.  I have one question in-line about whether you
considered touch instead of the sed replacement, but if you've thought through
the pros and cons I'm fine with whatever you decide.  Approved.

Thank you!

Gary

https://codereview.appspot.com/6776052/diff/5/Makefile
File Makefile (right):

https://codereview.appspot.com/6776052/diff/5/Makefile#newcode50
Makefile:50: @sed -re 's/^\# TIMESTAMP .+$$/\# TIMESTAMP $(DATE)/' -i
$(APPCACHE)
As I said on IRC, I'm not sure why this is preferable to simply touching the
file.  Touching seems simpler, so attractive to me.  Maybe it is insufficient or
maybe you prefer the written timestamp.  If you've considered touch, and you
prefer to go with what you have here, then that's cool.

[matthew.scott, 2012-10-28 13:00:45]

https://codereview.appspot.com/6776052/diff/5/Makefile
File Makefile (right):

https://codereview.appspot.com/6776052/diff/5/Makefile#newcode50
Makefile:50: @sed -re 's/^\# TIMESTAMP .+$$/\# TIMESTAMP $(DATE)/' -i
$(APPCACHE)
Did a bit of research and found http://appcachefacts.info/ which suggests that
touch may not be enough.  I suspect that this may be server dependent, but as
we're not sure of which server the gui will be deployed on, I think this might
be safer.
On 2012/10/28 12:25:14, gary.poster wrote:
> As I said on IRC, I'm not sure why this is preferable to simply touching the
> file.  Touching seems simpler, so attractive to me.  Maybe it is insufficient
or
> maybe you prefer the written timestamp.  If you've considered touch, and you
> prefer to go with what you have here, then that's cool.

[matthew.scott, 2012-10-28 13:03:03]

*** Submitted:

Make target for appcache manifest

Install target now depends on an appcache target, which updates a timestamp in
app/assets/manifest.appcache, thus forcing an update in the application cache.

R=gary.poster
CC=
https://codereview.appspot.com/6776052

[matthew.scott, 2012-10-28 14:31:59]

Please take a look.

[gary.poster, 2012-10-28 15:56:38]

Looks good, Matt.  Two small suggestions.  Thanks!

Gary

https://codereview.appspot.com/6776052/diff/9005/Makefile
File Makefile (right):

https://codereview.appspot.com/6776052/diff/9005/Makefile#newcode58
Makefile:58: appcache-force: appcache-touch appcache
I'm guessing you added this because you discovered you needed it, so that's cool
if so.  That said, I don't see a reason to factor out appcache-touch though.  Oh
wait...I see.  It's an ordering issue.  OK, new, different suggestion: add a
comment explaining what is going on so people don't have to figure it out. :-)

https://codereview.appspot.com/6776052/diff/9005/Makefile#newcode60
Makefile:60: .PHONY: test lint beautify server install clean prep jshint gjslint
appcache appcache-touch appcache-force
If you already know this, forgive me, but if you wanted to you could make those
break at 80 chars or similar by having a trailing "\" at the end of lines that
break.  Something like

.PHONY: test lint beautify server install clean prep jshint gjslint \
        appcache appcache-touch appcache-force

[matthew.scott, 2012-10-28 16:00:36]

*** Submitted:

Make target for appcache manifest

Install target now depends on an appcache target, which updates a timestamp in
app/assets/manifest.appcache, thus forcing an update in the application cache.

R=gary.poster
CC=
https://codereview.appspot.com/6776052