Merge lp:~gary/juju-gui/favicon into lp:juju-gui/experimental

[Gary Poster]

Add favicon, tweak Makefile

Added the juju icon as a favicon, and then got lost in Makefile tweaks. Makefile now does not do unnecessary work, and no longer hides commands with @ so that we can see when we reintroduce unnecessary work. Also switch from a Makefile comment about the node_modules to a message generated by the Makefile.

https://codereview.appspot.com/6842072/

[Brad Crittenden (bac)]

Hi Gary,

Thanks for the clean up to the Makefile and the addition of the favicon. I hadn't missed it before but think it'll be nice to have.

I took the liberty of making a card for this task assuming you just forgot. Also I couldn't find a Rietveld proposal for this work so I'm reviewing old school.

Unfortunately, running 'make server' locally I do not see the favicon show up in Chromium (and FF is hopelessly broken ATM). Looking at the downloaded resources I see it was served up but it does not show in the address bar. Are you actually seeing it?

I'm a bit confused by the NODE_TARGETS / EXPECTED_NODE_TARGETS / FOUND_NODE_TARGETS dance. I assume we can't just compute FOUND_NODE_TARGETS and use it directly. If that is really the case could you document exactly what is going on lest someone like the Future Me try to undo your work?

The 'test -d' before 'mkdir -p' is nice but unnecessary ask 'mkdir -p' will happily do a no-op and exit with 0 if the directory already exists. I see it used in a few places.

Otherwise it looks good and is easier to follow. Thanks.

review: Approve

[Gary Poster (gary)]

Reviewers: mp+135045_code.launchpad.net,

Message:
Please take a look.

Description:
Add favicon, tweak Makefile

Added the juju icon as a favicon, and then got lost in Makefile tweaks.
Makefile now does not do unnecessary work, and no longer hides commands
with @ so that we can see when we reintroduce unnecessary work. Also
switch from a Makefile comment about the node_modules to a message
generated by the Makefile; maybe this should cause the make to fail, so
that we actually force this to be maintained?

https://code.launchpad.net/~gary/juju-gui/favicon/+merge/135045

(do not edit description out of merge proposal)

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

Affected files:
   M Makefile
   A [revision details]
   A app/favicon.ico

[lp:~gary/juju-gui/favicon]

253. By Gary Poster on 2012-11-20

get debug and FF working

[Thiago Veronezi (tveronezi)]

Thanks Gary!
Seems good to me.

+1

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

https://codereview.appspot.com/6842072/diff/1/Makefile#newcode27
Makefile:27: test -d "$(BUILD_ASSETS_DIR)/stylesheets" || mkdir -p
"$(BUILD_ASSETS_DIR)/stylesheets"
Whats wrong with the use of "@"? Don't we like it any more? :O)

https://codereview.appspot.com/6842072/diff/1/Makefile#newcode42
Makefile:42: @# Check to see if we made what we expected to make, and
warn if we did not.
Do we need this @ before the comment?

https://codereview.appspot.com/6842072/

[Gary Poster (gary)]

Thank you, Thiago. Replies below. I'm happy to make a change if you
prefer, but until you tell me otherwise I'm going to proceed with the
understanding that you are fine with it as is.

Gary

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

https://codereview.appspot.com/6842072/diff/1/Makefile#newcode27
Makefile:27: test -d "$(BUILD_ASSETS_DIR)/stylesheets" || mkdir -p
"$(BUILD_ASSETS_DIR)/stylesheets"
On 2012/11/20 14:33:56, thiago wrote:
> Whats wrong with the use of "@"? Don't we like it any more? :O)
:-) "@" hides context when there are errors, and it hides the fact that
code is being re-run when it should not be in the Makefile. IMO, the
only advantage used to be that it made restarting the server (or running
tests or whatever) quieter, but now with the changes I've made it is
pretty quiet unless it is actually doing work. It didn't seem to get in
my way.

https://codereview.appspot.com/6842072/diff/1/Makefile#newcode42
Makefile:42: @# Check to see if we made what we expected to make, and
warn if we did not.
On 2012/11/20 14:33:56, thiago wrote:
> Do we need this @ before the comment?

If you don't, it is sent to stdout, and in this particular case I didn't
think it brought value to have it in stdout. Maybe that's
idiosyncratic; if someone removes I'd be fine with it.

https://codereview.appspot.com/6842072/

[lp:~gary/juju-gui/favicon]

254. By Gary Poster on 2012-11-20

respond to bac review

[Gary Poster (gary)]

> Hi Gary,

Hi Brad. Thank you for the review. Replies inline.

>
> Thanks for the clean up to the Makefile and the addition of the favicon. I
> hadn't missed it before but think it'll be nice to have.
>
> I took the liberty of making a card for this task assuming you just forgot.
> Also I couldn't find a Rietveld proposal for this work so I'm reviewing old
> school.

Yes, thank you. Sorry, it was an evening hack that got bigger than I intended.

>
> Unfortunately, running 'make server' locally I do not see the favicon show up
> in Chromium (and FF is hopelessly broken ATM). Looking at the downloaded
> resources I see it was served up but it does not show in the address bar. Are
> you actually seeing it?

Yes. As I mentioned on IRC, it was not working in debug mode, and I fixed that. Firefox seemed happier when I was explicit about the ico file also, though that should not be necessary. I tested it on another computer as well and it seemed ok to me.

>
> I'm a bit confused by the NODE_TARGETS / EXPECTED_NODE_TARGETS /
> FOUND_NODE_TARGETS dance. I assume we can't just compute FOUND_NODE_TARGETS
> and use it directly. If that is really the case could you document exactly
> what is going on lest someone like the Future Me try to undo your work?

Good call. I added a comment and got your approval on IRC.

>
> The 'test -d' before 'mkdir -p' is nice but unnecessary ask 'mkdir -p' will
> happily do a no-op and exit with 0 if the directory already exists. I see it
> used in a few places.

Nice improvement, yes. Changed.

>
> Otherwise it looks good and is easier to follow. Thanks.

Thank you!

Gary

[Gary Poster (gary)]

*** Submitted:

Add favicon, tweak Makefile

Added the juju icon as a favicon, and then got lost in Makefile tweaks.
Makefile now does not do unnecessary work, and no longer hides commands
with @ so that we can see when we reintroduce unnecessary work. Also
switch from a Makefile comment about the node_modules to a message
generated by the Makefile.

R=thiago
CC=
https://codereview.appspot.com/6842072

https://codereview.appspot.com/6842072/

=== modified file 'Makefile'
--- Makefile	2012-11-19 20:20:50 +0000
+++ Makefile	2012-11-20 15:12:20 +0000
@@ -1,16 +1,14 @@
 JSFILES=$(shell bzr ls -RV -k file | grep -E -e '.+\.js(on)?$$|generateTemplates$$' | grep -Ev -e '^manifest\.json$$' -e '^test/assets/' -e '^app/assets/javascripts/reconnecting-websocket.js$$' -e '^server.js$$')
 
-# After a successful "make" run, the NODE_TARGETS list can be regenerated with
-# this command (and then manually pasted in here):
-# find node_modules -maxdepth 1 -mindepth 1 -type d -printf 'node_modules/%f '
-NODE_TARGETS=node_modules/minimatch node_modules/cryptojs \
-	node_modules/yuidocjs node_modules/chai node_modules/less \
-	node_modules/.bin node_modules/node-markdown node_modules/rimraf \
-	node_modules/mocha node_modules/d3 node_modules/graceful-fs \
-	node_modules/should node_modules/jshint node_modules/expect.js \
-	node_modules/express node_modules/yui node_modules/yuidoc \
-	node_modules/grunt node_modules/node-spritesheet \
-	node_modules/node-minify
+NODE_TARGETS=node_modules/chai node_modules/cryptojs node_modules/d3 \
+	node_modules/expect.js node_modules/express node_modules/graceful-fs \
+	node_modules/grunt node_modules/jshint node_modules/less \
+	node_modules/minimatch node_modules/mocha node_modules/node-markdown \
+	node_modules/node-minify node_modules/node-spritesheet \
+	node_modules/rimraf node_modules/should node_modules/yui \
+	node_modules/yuidocjs
+EXPECTED_NODE_TARGETS=$(shell echo "$(NODE_TARGETS)" | tr ' ' '\n' | sort | tr '\n' ' ')
+
 TEMPLATE_TARGETS=$(shell bzr ls -k file app/templates)
 SPRITE_SOURCE_FILES=$(shell bzr ls -R -k file app/assets/images)
 BUILD_ASSETS_DIR=build/juju-ui/assets
@@ -26,92 +24,151 @@
 all: build
 
 build/juju-ui/templates.js: $(TEMPLATE_TARGETS) bin/generateTemplates
-	@test -d "$(BUILD_ASSETS_DIR)/stylesheets" || mkdir -p "$(BUILD_ASSETS_DIR)/stylesheets"
-	@./bin/generateTemplates
+	mkdir -p "$(BUILD_ASSETS_DIR)/stylesheets"
+	./bin/generateTemplates
 
 yuidoc/index.html: node_modules/yuidocjs $(JSFILES)
-	@node_modules/.bin/yuidoc -o yuidoc -x assets app
+	node_modules/.bin/yuidoc -o yuidoc -x assets app
 
 yuidoc: yuidoc/index.html
 
 $(SPRITE_GENERATED_FILES): node_modules/grunt node_modules/node-spritesheet $(SPRITE_SOURCE_FILES)
-	@node_modules/grunt/bin/grunt spritegen
+	node_modules/grunt/bin/grunt spritegen
 
 $(NODE_TARGETS): package.json
-	@npm install
-	@#link depends
-	@ln -sf `pwd`/node_modules/yui ./app/assets/javascripts/
-	@ln -sf `pwd`/node_modules/d3/d3.v2* ./app/assets/javascripts/
+	npm install
+	# Keep all targets up to date, not just new/changed ones.
+	for dirname in $(NODE_TARGETS); do touch $$dirname ; done
+	@# Check to see if we made what we expected to make, and warn if we did not.
+	@# Note that we calculate FOUND_TARGETS here, in this way and not in the
+	@# standard Makefile way, because we need to see what node_modules were
+	@# created by this target.  Makefile variables and substitutions, even when
+	@# using $(eval...) within a target, happen initially, before the target
+	@# is run.  Therefore, if this were a simple Makefile variable, it
+	@# would be empty after a first run, and you would always see the warning
+	@# message in that case.  We have to connect it to the "if" command with
+	@# "; \" because Makefile targets are evaluated per line, with bash
+	@# variables discarded between them.  We compare the result with
+	@# EXPECTED_NODE_TARGETS and not simply the NODE_TARGETS because this
+	@# gives us normalization, particularly of the trailing whitespace, that
+	@# we do not otherwise have.
+	@FOUND_TARGETS=$$(find node_modules -maxdepth 1 -mindepth 1 -type d \
+	-printf 'node_modules/%f ' | tr ' ' '\n' | grep -Ev '\.bin$$' \
+	| sort | tr '\n' ' '); \
+	if [ "$$FOUND_TARGETS" != "$(EXPECTED_NODE_TARGETS)" ]; then \
+	echo; \
+	echo "******************************************************************"; \
+	echo "******************************************************************"; \
+	echo "******************************************************************"; \
+	echo "******************************************************************"; \
+	echo "IMPORTANT: THE NODE_TARGETS VARIABLE IN THE MAKEFILE SHOULD CHANGE"; \
+	echo "******************************************************************"; \
+	echo "******************************************************************"; \
+	echo "******************************************************************"; \
+	echo "******************************************************************"; \
+	echo; \
+	echo "Change it to the following."; \
+	echo; \
+	echo $$FOUND_TARGETS; \
+	fi
+
+app/assets/javascripts/yui: node_modules/yui
+	ln -sf `pwd`/node_modules/yui ./app/assets/javascripts/
+
+node_modules/d3/d3.v2.js node_modules/d3/d3.v2.min.js: node_modules/d3
+
+app/assets/javascripts/d3.v2.js: node_modules/d3/d3.v2.js
+	ln -sf `pwd`/node_modules/d3/d3.v2.js ./app/assets/javascripts/d3.v2.js
+
+app/assets/javascripts/d3.v2.min.js: node_modules/d3/d3.v2.min.js
+	ln -sf `pwd`/node_modules/d3/d3.v2.min.js ./app/assets/javascripts/d3.v2.min.js
+
+javascript_libraries: app/assets/javascripts/yui \
+	app/assets/javascripts/d3.v2.js app/assets/javascripts/d3.v2.min.js
 
 gjslint: virtualenv/bin/gjslint
-	@virtualenv/bin/gjslint --strict --nojsdoc --jslint_error=all \
+	virtualenv/bin/gjslint --strict --nojsdoc --jslint_error=all \
 	    --custom_jsdoc_tags module,main,class,method,event,property,attribute,submodule,namespace,extends,config,constructor,static,final,readOnly,writeOnce,optional,required,param,return,for,type,private,protected,requires,default,uses,example,chainable,deprecated,since,async,beta,bubbles,extension,extensionfor,extension_for \
 	    $(JSFILES)
 
 jshint: node_modules/jshint
-	@node_modules/jshint/bin/hint $(JSFILES)
+	node_modules/jshint/bin/hint $(JSFILES)
 
 yuidoc-lint: $(JSFILES)
-	@bin/lint-yuidoc
+	bin/lint-yuidoc
 
 lint: gjslint jshint yuidoc-lint
 
 virtualenv/bin/gjslint virtualenv/bin/fixjsstyle:
-	@virtualenv virtualenv
-	@virtualenv/bin/easy_install archives/closure_linter-latest.tar.gz
+	virtualenv virtualenv
+	virtualenv/bin/easy_install archives/closure_linter-latest.tar.gz
 
 beautify: virtualenv/bin/fixjsstyle
-	@virtualenv/bin/fixjsstyle --strict --nojsdoc --jslint_error=all $(JSFILES)
+	virtualenv/bin/fixjsstyle --strict --nojsdoc --jslint_error=all $(JSFILES)
 
 spritegen: $(SPRITE_GENERATED_FILES)
 
 $(PRODUCTION_FILES): node_modules/yui node_modules/d3/d3.v2.min.js $(JSFILES) ./bin/merge-files
-	@rm -f $(PRODUCTION_FILES)
-	@test -d "$(BUILD_ASSETS_DIR)/stylesheets" || mkdir -p "$(BUILD_ASSETS_DIR)/stylesheets"
-	@./bin/merge-files
-	@cp app/modules.js $(BUILD_ASSETS_DIR)/modules.js
-	@cp app/config.js $(BUILD_ASSETS_DIR)/config.js
+	rm -f $(PRODUCTION_FILES)
+	mkdir -p "$(BUILD_ASSETS_DIR)/stylesheets"
+	./bin/merge-files
+	cp app/modules.js $(BUILD_ASSETS_DIR)/modules.js
+	cp app/config.js $(BUILD_ASSETS_DIR)/config.js
 
 combinejs: $(PRODUCTION_FILES)
 
 prep: beautify lint
 
 test: build
-	@./test-server.sh
+	./test-server.sh
 
 debug: build
 	@echo "Customize config.js to modify server settings"
-	@node server.js
+	node server.js
 
 server: build
-	@echo "Runnning the application from a SimpleHTTPServer"
-	@cd build && python -m SimpleHTTPServer 8888
+	@echo "Running the application from a SimpleHTTPServer"
+	cd build && python -m SimpleHTTPServer 8888
 
 clean:
-	@rm -rf node_modules virtualenv
-	@make -C docs clean
-	@rm -Rf build/
-
-build: appcache $(NODE_TARGETS) build/juju-ui/templates.js yuidoc spritegen combinejs
-	@cp -f app/index.html build/
-	@cp -rf app/assets/images $(BUILD_ASSETS_DIR)/images
-	@cp -rf app/assets/svgs $(BUILD_ASSETS_DIR)/svgs
+	rm -rf node_modules virtualenv
+	make -C docs clean
+	rm -Rf build/
+
+build/index.html: app/index.html
+	cp -f app/index.html build/
+
+build/favicon.ico: app/favicon.ico
+	cp -f app/favicon.ico build/
+
+$(BUILD_ASSETS_DIR)/images: $(SPRITE_SOURCE_FILES)
+	cp -rf app/assets/images $(BUILD_ASSETS_DIR)/images
+
+$(BUILD_ASSETS_DIR)/svgs: $(shell bzr ls -R -k file app/assets/svgs)
+	cp -rf app/assets/svgs $(BUILD_ASSETS_DIR)/svgs
+
+build_images: build/favicon.ico $(BUILD_ASSETS_DIR)/images \
+	$(BUILD_ASSETS_DIR)/svgs
+
+build: appcache $(NODE_TARGETS) javascript_libraries \
+	build/juju-ui/templates.js yuidoc spritegen \
+	combinejs build/index.html build_images
 
 $(APPCACHE): manifest.appcache.in
-	@test -d "build/juju-ui/assets" || mkdir -p "build/juju-ui/assets"
-	@cp manifest.appcache.in $(APPCACHE)
-	@sed -re 's/^\# TIMESTAMP .+$$/\# TIMESTAMP $(DATE)/' -i $(APPCACHE)
+	mkdir -p "build/juju-ui/assets"
+	cp manifest.appcache.in $(APPCACHE)
+	sed -re 's/^\# TIMESTAMP .+$$/\# TIMESTAMP $(DATE)/' -i $(APPCACHE)
 
 appcache: $(APPCACHE)
 
 # A target used only for forcibly updating the appcache.
 appcache-touch:
-	@touch manifest.appcache.in
+	touch manifest.appcache.in
 
 # This is the real target.  appcache-touch needs to be executed before
 # appcache, and this provides the correct order.
 appcache-force: appcache-touch appcache
 
-.PHONY: test lint beautify server build clean prep jshint gjslint \
+.PHONY: test lint beautify server clean build_images prep jshint gjslint \
 	appcache appcache-touch appcache-force yuidoc spritegen yuidoc-lint \
-	combinejs
+	combinejs javascript_libraries

=== added file 'app/favicon.ico'
Binary files app/favicon.ico	1970-01-01 00:00:00 +0000 and app/favicon.ico	2012-11-20 15:12:20 +0000 differ
=== modified file 'app/index.html'
--- app/index.html	2012-11-19 18:49:43 +0000
+++ app/index.html	2012-11-20 15:12:20 +0000
@@ -7,6 +7,7 @@
     <meta name="description" content="">
     <meta name="author" content="Juju team">
     <link href='http://fonts.googleapis.com/css?family=Ubuntu:400,400italic&subset=latin,latin-ext' rel='stylesheet' type='text/css'>
+    <link rel="shortcut icon" href="/favicon.ico">
     <link rel="stylesheet" href="/juju-ui/assets/stylesheets/all-static.css">
     <link rel="stylesheet" href="/juju-ui/assets/stylesheets/juju-gui.css">
     <link rel="stylesheet" href="/juju-ui/assets/stylesheets/sprite.css">

=== modified file 'lib/server.js'
--- lib/server.js	2012-11-16 16:20:13 +0000
+++ lib/server.js	2012-11-20 15:12:20 +0000
@@ -68,6 +68,10 @@
   res.sendfile('build/juju-ui/assets/stylesheets/' + fileName);
 });
 
+server.get('/favicon.ico', function(req, res) {
+  res.sendfile('app/favicon.ico');
+});
+
 server.get('*', function(req, res) {
   res.sendfile('app/index.html');
 });