Makefile does not support static file deployment

Makefile does not support static file deployment

Full description here: https://bugs.launchpad.net/juju-gui/+bug/1078898

This branch does not support CSS minification. It only combines the third-party css files. The minification of the YUI and our custom CSS files is covered by another branch;

The "make debug" command starts the node.js server with the debug version of our application;

The "make server" command starts a python SimpleHTTPServer service with the production version of our application. Note that it is not possible to use REST with this version. It will be covered by another card;

https://code.launchpad.net/~tveronezi/juju-gui/make-build/+merge/134706

(do not edit description out of merge proposal)

[thiago, 2012-11-16 16:36:15]

Please take a look.

[thiago, 2012-11-16 18:26:53]

https://codereview.appspot.com/6844061/diff/1/bin/merge-files
File bin/merge-files (right):

https://codereview.appspot.com/6844061/diff/1/bin/merge-files#newcode63
bin/merge-files:63: filesToLoad.js.push.apply(filesToLoad.js, [
'./app/assets/javascripts/d3.v2.min.js',
Please, ignore this long line. Its is already fixed.

[bac, 2012-11-19 18:18:58]

Thiago I think the work you've done here is good.  I'm a bit unclear how much
still needs to be done to finish off this refactoring of the build structure.

Thanks for tackling it.  The branch looks good to land after you make the
changes suggested and weigh the other ones.

https://codereview.appspot.com/6844061/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/6844061/diff/1/Makefile#newcode18
Makefile:18: PRODUCTION_FILES=build/juju-ui/assets/modules.js \
I'd be tempted to make a symbol for 'build/juju-ui/assets' since it is repeated
so often.

https://codereview.appspot.com/6844061/diff/1/Makefile#newcode89
Makefile:89: @cd build && python -m SimpleHTTPServer 8888
I'm unhappy that the magic port 8888 is found four times in our code and twice
in documentation.  I wish it were possible to define it once, somewhere but am
at a loss for how to do it.

https://codereview.appspot.com/6844061/diff/1/Makefile#newcode94
Makefile:94: @rm -Rf build/
Yay, the clean target is much simplified.

https://codereview.appspot.com/6844061/diff/1/Makefile#newcode100
Makefile:100: 
I'm concerned about separate build and install targets and having template.js in
a separate rule from the others.  Seems complicated and twisty.

https://codereview.appspot.com/6844061/diff/1/app/index.html
File app/index.html (right):

https://codereview.appspot.com/6844061/diff/1/app/index.html#newcode14
app/index.html:14: <link rel="stylesheet"
href="http://yui.yahooapis.com/combo?3.8.0pr1/build/slider-base/assets/skins/sam/slider-base.css">
Is this safe?  The URL with an embedded version number looks fragile.

https://codereview.appspot.com/6844061/diff/1/app/index.html#newcode15
app/index.html:15: <link>
What is the second <link> for?

https://codereview.appspot.com/6844061/diff/1/grunt.js
File grunt.js (right):

https://codereview.appspot.com/6844061/diff/1/grunt.js#newcode8
grunt.js:8: outputImage: 'stylesheets/sprite.png',
I guess putting sprites in 'stylesheets' is a lie we can live with.

[thiago, 2012-11-19 18:50:59]

Thanks for your review Brad!

https://codereview.appspot.com/6844061/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/6844061/diff/1/Makefile#newcode18
Makefile:18: PRODUCTION_FILES=build/juju-ui/assets/modules.js \
On 2012/11/19 18:18:58, bac wrote:
> I'd be tempted to make a symbol for 'build/juju-ui/assets' since it is
repeated
> so often.

Done.

https://codereview.appspot.com/6844061/diff/1/Makefile#newcode89
Makefile:89: @cd build && python -m SimpleHTTPServer 8888
On 2012/11/19 18:18:58, bac wrote:
> I'm unhappy that the magic port 8888 is found four times in our code and twice
> in documentation.  I wish it were possible to define it once, somewhere but am
> at a loss for how to do it.

I agree. I would like to create another card under the "Needs specification"
group for it, so we can discuss this issue later. Is that ok?

https://codereview.appspot.com/6844061/diff/1/Makefile#newcode94
Makefile:94: @rm -Rf build/
On 2012/11/19 18:18:58, bac wrote:
> Yay, the clean target is much simplified.

:O)

https://codereview.appspot.com/6844061/diff/1/Makefile#newcode100
Makefile:100: 
On 2012/11/19 18:18:58, bac wrote:
> I'm concerned about separate build and install targets and having template.js
in
> a separate rule from the others.  Seems complicated and twisty.


True. "install" does not mean "install". IMO "install" should be renamed to
"generate-files", or something like that. wdyt?

template.js should be in a separated rule, so we can determine when we should
execute it, right? The same applies to the sprites.

https://codereview.appspot.com/6844061/diff/1/app/index.html
File app/index.html (right):

https://codereview.appspot.com/6844061/diff/1/app/index.html#newcode14
app/index.html:14: <link rel="stylesheet"
href="http://yui.yahooapis.com/combo?3.8.0pr1/build/slider-base/assets/skins/sam/slider-base.css">
On 2012/11/19 18:18:58, bac wrote:
> Is this safe?  The URL with an embedded version number looks fragile.

True. I've investigated it a little bit further, and it works if I add the
'slider-base' requirement to the environment.js file. Tkx Brad!

https://codereview.appspot.com/6844061/diff/1/app/index.html#newcode15
app/index.html:15: <link>
On 2012/11/19 18:18:58, bac wrote:
> What is the second <link> for?
for nothing :)
Removed tkx.

https://codereview.appspot.com/6844061/diff/1/grunt.js
File grunt.js (right):

https://codereview.appspot.com/6844061/diff/1/grunt.js#newcode8
grunt.js:8: outputImage: 'stylesheets/sprite.png',
On 2012/11/19 18:18:58, bac wrote:
> I guess putting sprites in 'stylesheets' is a lie we can live with.

I agree.

[gary.poster, 2012-11-19 19:00:33]

Thanks Thiago.  This is a nice improvement.  Once we have the CSS changes, and
we no longer have the slow initial combo CSS load from YUI, this will be really
nice to use.  I have some small suggestions for the Makefile, in line with
Brad's comments.

https://codereview.appspot.com/6844061/diff/1/.bzrignore
File .bzrignore (right):

https://codereview.appspot.com/6844061/diff/1/.bzrignore#newcode16
.bzrignore:16: build
Nice simplification and clean-up.

https://codereview.appspot.com/6844061/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/6844061/diff/1/Makefile#newcode100
Makefile:100: 
On 2012/11/19 18:18:58, bac wrote:
> I'm concerned about separate build and install targets and having template.js
in
> a separate rule from the others.  Seems complicated and twisty.

Having "build" depend on "install" also seems odd, conceptually.

Does the "install" target bring any value?  That is, is there any use case for
running it alone?  If not, I suggest combining it and "build" into the "build"
target (and deleting the "install" target).  What do you two think of that?

https://codereview.appspot.com/6844061/diff/1/lib/server.js
File lib/server.js (right):

https://codereview.appspot.com/6844061/diff/1/lib/server.js#newcode57
lib/server.js:57: res.sendfile('build/juju-ui/assets/' + fileName);
Interesting.  I wanted the development server code to be even simpler than this,
but I can see why what you did is compelling.  I would be tempted to experiment
with converting the js files to symlinks in a later branch and simplifying this
code.  This is an improvement anyway, though, and maybe my hunch won't work out.

[thiago, 2012-11-19 20:23:19]

Thanks Gary for your review!

https://codereview.appspot.com/6844061/diff/1/.bzrignore
File .bzrignore (right):

https://codereview.appspot.com/6844061/diff/1/.bzrignore#newcode16
.bzrignore:16: build
On 2012/11/19 19:00:33, gary.poster wrote:
> Nice simplification and clean-up.

Ohhh yeah. :O)

https://codereview.appspot.com/6844061/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/6844061/diff/1/Makefile#newcode100
Makefile:100: 
On 2012/11/19 19:00:33, gary.poster wrote:
> On 2012/11/19 18:18:58, bac wrote:
> > I'm concerned about separate build and install targets and having
template.js
> in
> > a separate rule from the others.  Seems complicated and twisty.
> 
> Having "build" depend on "install" also seems odd, conceptually.
> 
> Does the "install" target bring any value?  That is, is there any use case for
> running it alone?  If not, I suggest combining it and "build" into the "build"
> target (and deleting the "install" target).  What do you two think of that?

Ah... I see! I will delete the "install" target and change the "build" target
from...

build: install

... to...

build: appcache $(NODE_TARGETS) build/juju-ui/templates.js yuidoc spritegen
combinejs

Is that ok?

https://codereview.appspot.com/6844061/diff/1/lib/server.js
File lib/server.js (right):

https://codereview.appspot.com/6844061/diff/1/lib/server.js#newcode57
lib/server.js:57: res.sendfile('build/juju-ui/assets/' + fileName);
On 2012/11/19 19:00:33, gary.poster wrote:
> Interesting.  I wanted the development server code to be even simpler than
this,
> but I can see why what you did is compelling.  I would be tempted to
experiment
> with converting the js files to symlinks in a later branch and simplifying
this
> code.  This is an improvement anyway, though, and maybe my hunch won't work
out.

I needed to change the "test-server.js" file too. The tests were broken due to
the new notification stuff. I've tried to use the symbolic link approach in
order to avoid it, but with no success. 

I will use the "new branch" option to do it. Thanks.

[thiago, 2012-11-19 20:24:44]

Please take a look.

[gary.poster, 2012-11-19 21:03:44]

Good, Thiago, thank you.  The slow CSS loading is the only downside for putting
this branch in staging, and if you can follow it quickly with the CSS branch
then I think it will be great.

+1!

Gary

https://codereview.appspot.com/6844061/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/6844061/diff/1/Makefile#newcode89
Makefile:89: @cd build && python -m SimpleHTTPServer 8888
On 2012/11/19 18:50:59, thiago wrote:
> On 2012/11/19 18:18:58, bac wrote:
> > I'm unhappy that the magic port 8888 is found four times in our code and
twice
> > in documentation.  I wish it were possible to define it once, somewhere but
am
> > at a loss for how to do it.
> 
> I agree. I would like to create another card under the "Needs specification"
> group for it, so we can discuss this issue later. Is that ok?

If you have any ideas on what we can do, +1.  Otherwise, eh, don't bother IMO.

https://codereview.appspot.com/6844061/diff/1/lib/server.js
File lib/server.js (right):

https://codereview.appspot.com/6844061/diff/1/lib/server.js#newcode57
lib/server.js:57: res.sendfile('build/juju-ui/assets/' + fileName);
On 2012/11/19 20:23:19, thiago wrote:
> On 2012/11/19 19:00:33, gary.poster wrote:
> > Interesting.  I wanted the development server code to be even simpler than
> this,
> > but I can see why what you did is compelling.  I would be tempted to
> experiment
> > with converting the js files to symlinks in a later branch and simplifying
> this
> > code.  This is an improvement anyway, though, and maybe my hunch won't work
> out.
> 
> I needed to change the "test-server.js" file too. The tests were broken due to
> the new notification stuff. I've tried to use the symbolic link approach in
> order to avoid it, but with no success. 
> 
> I will use the "new branch" option to do it. Thanks. 

Cool.  I'm not sure it is that worth it to mess with it, given that you have
already experimented with it.  That said, bug 1078910 will lead to a natural
opportunity to investigate.

https://codereview.appspot.com/6844061/diff/6002/Makefile
File Makefile (right):

https://codereview.appspot.com/6844061/diff/6002/Makefile#newcode16
Makefile:16: BUILD_ASSETS_DIR=build/juju-ui/assets
This ends up saving two characters per reference. :-)  I guess it is still an
advantage to prevent typos though.  Well...not even much there, since bash
doesn't have errors for unknown names.  I dunno, I'm certainly fine with this,
but it didn't end up helping much IMO.  Maybe bac has a different suggestion for
a name?

https://codereview.appspot.com/6844061/diff/6002/Makefile#newcode95
Makefile:95: build: appcache $(NODE_TARGETS) build/juju-ui/templates.js yuidoc
spritegen combinejs
I think "build" is the right name logically.  benji might not like the fact that
we are breaking backwards compatibility for the dev experience, but since the
"all" target is equivalent and still works, I think it is fine.

[thiago, 2012-11-19 21:15:51]

*** Submitted:

Makefile does not support static file deployment

Full description here: https://bugs.launchpad.net/juju-gui/+bug/1078898

This branch does not support CSS minification. It only combines the third-party
css files. The minification of the YUI and our custom CSS files is covered by
another branch;

The "make debug" command starts the node.js server with the debug version of our
application;

The "make server" command starts a python SimpleHTTPServer service with the
production version of our application. Note that it is not possible to use REST
with this version. It will be covered by another card;

R=bac, gary.poster
CC=
https://codereview.appspot.com/6844061