launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/shells-are-daft into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~allenap/maas/shells-are-daft/+merge/103428

This adds explicit checks for several external executables because shells (well, bash/sh at least) consider the absence of an executable the same as any other non-zero return code. The checks take the form of a `make` function that uses `which` to look for the command. If the test fails a message is printed suggesting a package that ought to be installed.

The motivation for doing this was daemontools. It was not installed on the Jenkins builder, yet `make distclean` was completing without error. It was printing out gobs of complaining messages, but was returning zero.

I've also made a similar change for pocketlint. Previously there was a target to `sudo apt-get install` pocketlint's package, but I was never entirely comfortable with that. Now it suggests that the package should be installed and leaves it up to the user to sort that out.

-- 
https://code.launchpad.net/~allenap/maas/shells-are-daft/+merge/103428
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/shells-are-daft into lp:maas.

=== modified file 'Makefile'
--- Makefile	2012-04-24 09:15:00 +0000
+++ Makefile	2012-04-25 09:06:24 +0000
@@ -57,17 +57,16 @@
 	@find $(sources) -name '*.py' ! -path '*/migrations/*' \
 	    -print0 | xargs -r0 bin/flake8
 
+pocketlint = $(call available,pocketlint,python-pocket-lint)
+
 lint-css: sources = src/maasserver/static/css
-lint-css: /usr/bin/pocketlint
+lint-css:
 	@find $(sources) -type f \
-	    -print0 | xargs -r0 pocketlint --max-length=120
+	    -print0 | xargs -r0 $(pocketlint) --max-length=120
 
 lint-js: sources = src/maasserver/static/js
-lint-js: /usr/bin/pocketlint
-	@find $(sources) -type f -print0 | xargs -r0 pocketlint
-
-/usr/bin/pocketlint:
-	sudo apt-get install python-pocket-lint
+lint-js:
+	@find $(sources) -type f -print0 | xargs -r0 $(pocketlint)
 
 check: clean test
 
@@ -156,36 +155,44 @@
   supervise
 endef
 
+# Convenient variables and functions for service control.
+
+setlock = $(call available,setlock,daemontools)
+supervise = $(call available,supervise,daemontools)
+svc = $(call available,svc,daemontools)
+svok = $(call available,svok,daemontools)
+svstat = $(call available,svstat,daemontools)
+
+service_lock = $(setlock) -n /run/lock/maas.dev.$(firstword $(1))
+
 # Pseudo-magic targets for controlling individual services.
 
-service_lock = setlock -n /run/lock/maas.dev.$(firstword $(1))
-
 services/%/@run: services/%/@stop services/%/@deps
 	@$(call service_lock, $*) services/$*/run
 
 services/%/@start: services/%/@supervise
-	@svc -u $(@D)
+	@$(svc) -u $(@D)
 
 services/%/@pause: services/%/@supervise
-	@svc -d $(@D)
+	@$(svc) -d $(@D)
 
 services/%/@status:
-	@svstat $(@D)
+	@$(svstat) $(@D)
 
 services/%/@restart: services/%/@supervise
-	@svc -du $(@D)
+	@$(svc) -du $(@D)
 
 services/%/@stop:
-	@if svok $(@D); then svc -dx $(@D); fi
-	@while svok $(@D); do sleep 0.1; done
+	@if $(svok) $(@D); then $(svc) -dx $(@D); fi
+	@while $(svok) $(@D); do sleep 0.1; done
 
 services/%/@supervise: services/%/@deps
 	@mkdir -p logs/$*
 	@touch $(@D)/down
-	@if ! svok $(@D); then \
+	@if ! $(svok) $(@D); then \
 	    logdir=$(PWD)/logs/$* \
-	        $(call service_lock, $*) supervise $(@D) & fi
-	@while ! svok $(@D); do sleep 0.1; done
+	        $(call service_lock, $*) $(supervise) $(@D) & fi
+	@while ! $(svok) $(@D); do sleep 0.1; done
 
 # Dependencies for individual services.
 
@@ -211,3 +218,14 @@
 phony := $(sort $(strip $(phony)))
 
 .PHONY: $(phony)
+
+#
+# Functions.
+#
+
+# Check if a command is found on PATH. Raise an error if not, citing
+# the package to install. Return the command otherwise.
+# Usage: $(call available,<command>,<package>)
+define available
+  $(if $(shell which $(1)),$(1),$(error $(1) not found; install $(2)))
+endef