Serve runtime environment statically.

Serve runtime environment statically.

Create two new "prod" and "debug" environments, that are served
statically by a HTTP Python server. Keep serving the "devel"
environment dinamically by node.js, it will be made static
later. Make tests run in the "debug" environment. Refactor the
Makefile adding some docs and targets.

https://code.launchpad.net/~teknico/juju-gui/bug-1078910-serve-app-statically/+merge/138212

(do not edit description out of merge proposal)

[teknico, 2012-12-05 15:31:40]

Please take a look.

[hazmat, 2012-12-05 15:46:57]

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

https://codereview.appspot.com/6878053/diff/1/Makefile#newcode53
Makefile:53: @echo "doc: generate Sphinx and YuiDoc documentation"
awesome, thanks.

https://codereview.appspot.com/6878053/diff/1/Makefile#newcode65
Makefile:65: make -C docs html
recursive make considered harmful.

what about just renaming the yuidoc target

[matthew.scott, 2012-12-05 16:37:24]

Ready to land.

Looks good to me, thanks for the work!

[teknico, 2012-12-05 17:54:22]

Please take a look.

[benji, 2012-12-05 20:37:17]

This branch will be a big improvement.  I would like to re-review after you
respond to my comments.

https://codereview.appspot.com/6878053/diff/5001/.bzrignore
File .bzrignore (right):

https://codereview.appspot.com/6878053/diff/5001/.bzrignore#newcode16
.bzrignore:16: build*
This ignore is too broad.  It will ignore all files which have names that begin
with the string "build".

https://codereview.appspot.com/6878053/diff/5001/Makefile
File Makefile (right):

https://codereview.appspot.com/6878053/diff/5001/Makefile#newcode35
Makefile:35: PROD_ASSETS=$(PROD)/$(JUJU_UI)/assets
These sorts of aliases make the code much harder for me to read.  For example, I
know what "app" is, hiding it behind "SRC" just obfuscates things.

https://codereview.appspot.com/6878053/diff/5001/Makefile#newcode156
Makefile:156: link_debug_files:
Wow, that is a lot of links.  I guess this means that every time we add a source
file we will have to add a line here, right?  Is it not possible to just serve
the files directly from app/ and avoid all of this?  Or programmatically
generate the links instead of having to enumerate them all?

https://codereview.appspot.com/6878053/diff/5001/Makefile#newcode158
Makefile:158: ln -sf `pwd`/$(SRC)/favicon.ico `pwd`/$(DEBUG)/
The Makefile finds the current working directory two ways, $(PWD) and `pwd` as
here.  $(PWD) is slightly better because `pwd` runs a shell each time just to
get the current directory.

https://codereview.appspot.com/6878053/diff/5001/Makefile#newcode203
Makefile:203: devel: build
The most bikely of sheds: "dev" is even shorter than "devel" ;)

[gary.poster, 2012-12-06 03:38:12]

Land with changes (from both me and Benji).

Thank you Nicola.  This will be very nice to have.

I talked with Benji about this.  He is OK with you landing this with the
requests I've made, and considerations of your other comments.  He also would
really like to reduce the number of constants, but is willing to be flexible
about it and leave the decision to you.

I saw that you ran tests on debug, rather than devel, which is a good choice to
my mind.  I filed https://bugs.launchpad.net/juju-gui/+bug/1087107 for
production.  This was a pre-existing problem.

I don't like the filesystem division between app ("devel") and debug but I
respect your decision and the logic behind it.  Let's proceed.

I wondered if the help text would be improved by clarifying the meaning of the
different "run levels."  See what you think: http://pastebin.ubuntu.com/1413894/
.

Thanks again,

Gary

https://codereview.appspot.com/6878053/diff/5001/.bzrignore
File .bzrignore (right):

https://codereview.appspot.com/6878053/diff/5001/.bzrignore#newcode16
.bzrignore:16: build*
On 2012/12/05 20:37:17, benji wrote:
> This ignore is too broad.  It will ignore all files which have names that
begin
> with the string "build".
+1

https://codereview.appspot.com/6878053/diff/5001/Makefile
File Makefile (right):

https://codereview.appspot.com/6878053/diff/5001/Makefile#newcode35
Makefile:35: PROD_ASSETS=$(PROD)/$(JUJU_UI)/assets
On 2012/12/05 20:37:17, benji wrote:
> These sorts of aliases make the code much harder for me to read.  For example,
I
> know what "app" is, hiding it behind "SRC" just obfuscates things.
I have more sympathy for constants like this than benji does when in Python
land, because I like NameErrors to find my typos.  Make doesn't give us that
though.  The BUILD, DEBUG, PROD variables don't seem to add much to me in
particular.  I shrug, even though benji doesn't.

https://codereview.appspot.com/6878053/diff/5001/Makefile#newcode156
Makefile:156: link_debug_files:
On 2012/12/05 20:37:17, benji wrote:
> Wow, that is a lot of links.  I guess this means that every time we add a
source
> file we will have to add a line here, right?  Is it not possible to just serve
> the files directly from app/ and avoid all of this?  Or programmatically
> generate the links instead of having to enumerate them all?

I agree that these are a lot of links.  However, it is not nearly as bad as
benji fears, and essentially the same as what we have now: as long as we add
files only in models, store, templates (via handlebars building), views,
widgets, images, javascripts, and svgs, then we will not need to change this
list.

I am unhappy with this list for two other reasons, though.

First, you are moving things around from the app directory.  If the app
directory mirrored the expected deployment structure as I had suggested,
wouldn't this have been simpler, shorter, and possibly more amenable to
generating links as benji describes?  REQUESTED ACTION: either change the app
directory to have the desired structure or make a bug to do so later (and then
remove as much as possible the need to identify individual files here).

Second, you are maintaining both link_debug_files and link_prod_files.  As far
as I can tell on a quick skim, link_debug_files is a rough superset of the
actions of link_prod_files.  REQUESTED ACTION: Could you make a macro for the
shared bits
(http://www.gnu.org/software/make/manual/html_node/Call-Function.html) so we
reduce the chance of drift?

https://codereview.appspot.com/6878053/diff/5001/Makefile#newcode158
Makefile:158: ln -sf `pwd`/$(SRC)/favicon.ico `pwd`/$(DEBUG)/
On 2012/12/05 20:37:17, benji wrote:
> The Makefile finds the current working directory two ways, $(PWD) and `pwd` as
> here.  $(PWD) is slightly better because `pwd` runs a shell each time just to
> get the current directory.
+1

[gary.poster, 2012-12-06 03:42:40]

On 2012/12/06 03:38:12, gary.poster wrote:
> Land with changes (from both me and Benji).
> 
> Thank you Nicola.  This will be very nice to have.
> 
> I talked with Benji about this.  He is OK with you landing this with the
> requests I've made, and considerations of your other comments.

considerations of *his* other comments, I mean :-)

[teknico, 2012-12-06 19:44:16]

Please take a look.

[teknico, 2012-12-06 19:47:35]

gary.poster wrote:
> I talked with Benji about this.  He is OK with you landing this with
> the requests I've made, and considerations of your other comments.
> He also would really like to reduce the number of constants, but is
> willing to be flexible about it and leave the decision to you.

I removed most single-value constants, and reworked targets and instructions as
discussed. Reconciling the last changes from Benji took some efforts. :-)

There have been quite a few more changes, some additional reviewing is probably
warranted.

[benji, 2012-12-06 20:09:35]

This looks great.  I had one suggestion about target names.

https://codereview.appspot.com/6878053/diff/10001/Makefile
File Makefile (right):

https://codereview.appspot.com/6878053/diff/10001/Makefile#newcode243
Makefile:243: clean:
The make "Standard Targets" docs
(https://www.gnu.org/prep/standards/html_node/Standard-Targets.html) and
the principal of least surprise suggest that "make clean" should clean
everything that the make file builds.

Other clean-* targets can then clean less, if you know what you want to ask for.

The same chapter suggests "mostlyclean" to clean the things that people
"normally" want to clean.  That sounds reasonable to me.

[teknico, 2012-12-07 11:50:18]

*** Submitted:

Serve runtime environment statically.

Create two new "prod" and "debug" environments, that are served
statically by a HTTP Python server. Keep serving the "devel"
environment dinamically by node.js, it will be made static
later. Make tests run in the "debug" environment. Refactor the
Makefile adding some docs and targets.

R=hazmat, matthew.scott, benji, gary.poster
CC=
https://codereview.appspot.com/6878053

[teknico, 2012-12-07 14:12:06]

benji wrote:
> The make "Standard Targets" docs
> (https://www.gnu.org/prep/standards/html_node/Standard-Targets.html) and
> the principal of least surprise suggest that "make clean" should clean
> everything that the make file builds.

It does clean everything that the "make" invocation with no target builds.

Now that "make" builds the production and debug environment, I found it useful
that "make clean" cleaned those environments only, while developing.

> The same chapter suggests "mostlyclean" to clean the things that people
> "normally" want to clean.  That sounds reasonable to me.

Not to me, actually, it sounds rather weird. :-) I like "clean-all" better.

I wonder what our colleagues think?