Merge lp:~benji/charm-tools/prooflib into lp:charm-tools/1.2

[Benji York]

Additional project niceties.

https://codereview.appspot.com/36190043/

[Benji York (benji)]

Reviewers: mp+197439_code.launchpad.net,

Message:
Comments for Marco.

https://codereview.appspot.com/36190043/diff/20001/.lbox
File .lbox (right):

https://codereview.appspot.com/36190043/diff/20001/.lbox#newcode1
.lbox:1: propose -cr -for lp:charmtools
Set the default options for "lbox propose".

https://codereview.appspot.com/36190043/diff/20001/.lbox.check
File .lbox.check (right):

https://codereview.appspot.com/36190043/diff/20001/.lbox.check#newcode1
.lbox.check:1: #!/bin/bash
This file is run pre-propose and pre-submit. If it returns an error the
propose/submit is cancelled.

https://codereview.appspot.com/36190043/diff/20001/Makefile
File Makefile (right):

https://codereview.appspot.com/36190043/diff/20001/Makefile#newcode7
Makefile:7: #SHELL = $(warning [$@ ($^) ($?) ])$(OLD_SHELL)
This snippet is incredibly handy when debugging complex Makefile issues,
but for one this small it may not be worth maintaining here.

https://codereview.appspot.com/36190043/diff/20001/Makefile#newcode24
Makefile:24: virtualenv .
Using a virtualenv gives us (some) isolation from the system, enhancing
build reproducibility and reducing spurious errors.

https://codereview.appspot.com/36190043/diff/20001/Makefile#newcode34
Makefile:34: build: deps bin/python bin/pip develop bin/test
This target is the core of the Makefile.

https://codereview.appspot.com/36190043/diff/20001/Makefile#newcode36
Makefile:36: sysdeps:
This installs the (few) system dependencies.

https://codereview.appspot.com/36190043/diff/20001/Makefile#newcode122
Makefile:122: .DEFAULT_GOAL := build
A bare "make" command should build the software.

https://codereview.appspot.com/36190043/diff/20001/charmtools/__init__.py
File charmtools/__init__.py (left):

https://codereview.appspot.com/36190043/diff/20001/charmtools/__init__.py#oldcode19
charmtools/__init__.py:19: import glob
Lint: unused import.

https://codereview.appspot.com/36190043/diff/20001/charmtools/bundles.py
File charmtools/bundles.py (left):

https://codereview.appspot.com/36190043/diff/20001/charmtools/bundles.py#oldcode2
charmtools/bundles.py:2: import sys
More lint.

https://codereview.appspot.com/36190043/diff/20001/charmtools/charms.py
File charmtools/charms.py (left):

https://codereview.appspot.com/36190043/diff/20001/charmtools/charms.py#oldcode2
charmtools/charms.py:2: import sys
Lint.

https://codereview.appspot.com/36190043/diff/20001/charmtools/cli.py
File charmtools/cli.py (left):

https://codereview.appspot.com/36190043/diff/20001/charmtools/cli.py#oldcode4
charmtools/cli.py:4: import argparse
So much lint.

https://codereview.appspot.com/36190043/diff/20001/charmtools/mr.py
File charmtools/mr.py (left):

https://codereview.appspot.com/36190043/diff/20001/charmtools/mr.py#oldcode19
charmtools/mr.py:19: from bzrlib import errors, trace
Lint.

https://codereview.appspot.com/36190043/diff/20001/charmtools/promulgate.py
File charmtools/promulgate.py (left):

https://codereview.appspot.com/36190043/diff/20001/charmtools/promulgate.py#oldcode27
charmtools/promulgate.py:27: import string
Lint.

https://codereview.app...

[Marco Ceppi (marcoceppi)]

These changes look fine, I'm not sure if .lbox is configured properly or
not. Could you review?

https://codereview.appspot.com/36190043/diff/20001/.lbox
File .lbox (right):

https://codereview.appspot.com/36190043/diff/20001/.lbox#newcode1
.lbox:1: propose -cr -for lp:charmtools
On 2013/12/03 14:40:01, benji wrote:
> Set the default options for "lbox propose".

lp:charm-tools

https://codereview.appspot.com/36190043/diff/20001/charmtools/subscribers.py
File charmtools/subscribers.py (left):

https://codereview.appspot.com/36190043/diff/20001/charmtools/subscribers.py#oldcode126
charmtools/subscribers.py:126: charm =
charmdistro.getSourcePackage(name=charm_name)
On 2013/12/03 14:40:01, benji wrote:
> So... the linter complained that "charm" was assigned a value but
never used,
> which is true. Looking at the code I saw that "charm_name" was
appended to the
> list "charms" so I figured s/charm_name/charm/ was the right thing to
do, so I
> did it and ran the tests. None failed. I ran "make check" and some
failed. I
> changed the code back and "make check" still failed. I ran "make
check" on
> lp:charm-tools/1.2 and it failed in the same way there.

> I looked at the other code and despite the list being named "charms",
charm
> names are appended to it below, so I really don't know what to do
here.

> Therefore, just removed the assignment to make the linter happy and
left it for
> you to figure out. :)

Yeah, I didn't write this :) I'll go over it post merge and review what
it's trying to do.

https://codereview.appspot.com/36190043/

[Benji York (benji)]

Fixed typo.

https://codereview.appspot.com/36190043/diff/20001/.lbox
File .lbox (right):

https://codereview.appspot.com/36190043/diff/20001/.lbox#newcode1
.lbox:1: propose -cr -for lp:charmtools
On 2013/12/03 19:51:14, Marco Ceppi wrote:
> On 2013/12/03 14:40:01, benji wrote:
> > Set the default options for "lbox propose".

> lp:charm-tools

Done.

https://codereview.appspot.com/36190043/

[Benji York (benji)]

Please take a look.

https://codereview.appspot.com/36190043/

[Marco Ceppi (marcoceppi)]

https://codereview.appspot.com/36190043/diff/40001/Makefile
File Makefile (right):

https://codereview.appspot.com/36190043/diff/40001/Makefile#newcode42
Makefile:42: bzr checkout lp:~juju-jitsu/charm-tools/dependencies
Can we not have it owned by ~juju-jitsu and instead place it under
~charmers?

https://codereview.appspot.com/36190043/diff/40001/Makefile#newcode47
Makefile:47: $(PYTHON_PACKAGE_CANARY): requirements.txt | bin/pip
dependencies
Why wouldn't we keep requirements.txt for deps? Decoupling them means
they could change between versions of charm-tools and the dependencies
branch. Having the requirements.txt be the source for deps makes more
sense to me. Unless I'm missing the point of this line.

https://codereview.appspot.com/36190043/

[Benji York (benji)]

Here are my responses to the most recent round of review notes.

https://codereview.appspot.com/36190043/diff/40001/Makefile
File Makefile (right):

https://codereview.appspot.com/36190043/diff/40001/Makefile#newcode42
Makefile:42: bzr checkout lp:~juju-jitsu/charm-tools/dependencies
On 2013/12/05 16:30:50, Marco Ceppi wrote:
> Can we not have it owned by ~juju-jitsu and instead place it under
~charmers?

Sure. Is lp:~charmers/charm-tools/dependencies an acceptable/valid
location?

https://codereview.appspot.com/36190043/diff/40001/Makefile#newcode47
Makefile:47: $(PYTHON_PACKAGE_CANARY): requirements.txt | bin/pip
dependencies
On 2013/12/05 16:30:50, Marco Ceppi wrote:
> Why wouldn't we keep requirements.txt for deps? Decoupling them means
they could
> change between versions of charm-tools and the dependencies branch.
Having the
> requirements.txt be the source for deps makes more sense to me. Unless
I'm
> missing the point of this line.

The requirements.txt file specifies the dependencies. The dependencies
directory exists so all the dependencies are conveniently available in
one place and to make sure the dependencies are available (people have a
bad habit of removing "old" distributions from the web) wile also
removing the need for web access to build the software (building on a
plane or behind an egress-filtering firewall are important use cases).

https://codereview.appspot.com/36190043/

[Marco Ceppi (marcoceppi)]

On 2013/12/05 16:41:16, benji wrote:
> Here are my responses to the most recent round of review notes.

> https://codereview.appspot.com/36190043/diff/40001/Makefile
> File Makefile (right):

> https://codereview.appspot.com/36190043/diff/40001/Makefile#newcode42
> Makefile:42: bzr checkout lp:~juju-jitsu/charm-tools/dependencies
> On 2013/12/05 16:30:50, Marco Ceppi wrote:
> > Can we not have it owned by ~juju-jitsu and instead place it under
~charmers?

> Sure. Is lp:~charmers/charm-tools/dependencies an acceptable/valid
location?

This is fine, thanks!

> https://codereview.appspot.com/36190043/diff/40001/Makefile#newcode47
> Makefile:47: $(PYTHON_PACKAGE_CANARY): requirements.txt | bin/pip
dependencies
> On 2013/12/05 16:30:50, Marco Ceppi wrote:
> > Why wouldn't we keep requirements.txt for deps? Decoupling them
means they
> could
> > change between versions of charm-tools and the dependencies branch.
Having the
> > requirements.txt be the source for deps makes more sense to me.
Unless I'm
> > missing the point of this line.

> The requirements.txt file specifies the dependencies. The
dependencies
> directory exists so all the dependencies are conveniently available in
one place
> and to make sure the dependencies are available (people have a bad
habit of
> removing "old" distributions from the web) wile also removing the need
for web
> access to build the software (building on a plane or behind an
egress-filtering
> firewall are important use cases).

I appreciate the explanation. So as we add deps/move to other versions
we'd just include those in addition to the older versions? Either way I
approve of this.

LGTM!

https://codereview.appspot.com/36190043/

[Marco Ceppi (marcoceppi)]

review: Approve

=== modified file '.bzrignore'
--- .bzrignore	2012-04-23 20:46:55 +0000
+++ .bzrignore	2013-12-04 15:05:03 +0000
@@ -1,1 +1,10 @@
 tests/proof/results/*
+.noseids
+charm_tools.egg-info
+dependencies
+tags
+bin
+include
+lib
+local
+man

=== added file '.lbox'
--- .lbox	1970-01-01 00:00:00 +0000
+++ .lbox	2013-12-04 15:05:03 +0000
@@ -0,0 +1,1 @@
+propose -cr -for lp:charm-tools

=== added file '.lbox.check'
--- .lbox.check	1970-01-01 00:00:00 +0000
+++ .lbox.check	2013-12-04 15:05:03 +0000
@@ -0,0 +1,4 @@
+#!/bin/bash
+set -e
+make lint
+make test

=== added file 'HACKING.txt'
--- HACKING.txt	1970-01-01 00:00:00 +0000
+++ HACKING.txt	2013-12-04 15:05:03 +0000
@@ -0,0 +1,61 @@
+=======
+HACKING
+=======
+
+Herein lay instructions on how to contribute to this project.
+
+
+Developer install
+=================
+
+This codebase requires a few system-wide dependencies be installed.  The
+"sysdeps" make target will install them::
+
+    make sysdeps
+
+Next the build needs to be run::
+
+    make
+
+Tests can be run my make or as a script (which allows for command-line options
+to be passed).
+
+::
+
+  make test
+  bin/test
+
+
+Submitting a branch
+===================
+
+In order to submit code for review we use a tool called lbox. If you are using
+Ubuntu < 13.04 you can install lbox by::
+
+  sudo apt-get install lbox
+
+If you are using a newer version you will have to build it from source. First
+step is to install Go lang, you can find the instructions on installing it
+here http://golang.org/doc/install#bsd_linux , making sure to set your GOROOT
+environment variable appropriately. After that, you should set up an install
+directory that you can access, such as $HOME/bin/go which you can set as your
+GOPATH directory (see `go help gopath` for more information). For ease of
+use, you can append $GOROOT/bin and $GOPATH/bin to your PATH environment
+variable. Then follow the instructions::
+
+  sudo go get launchpad.net/lbox
+  sudo go install launchpad.net/lbox
+
+To submit for review::
+
+  lbox propose
+
+After review, to merge into trunk::
+
+  lbox submit
+
+
+Filing Bugs
+===========
+
+Please file bugs here: https://bugs.launchpad.net/charm-tools/+filebug

=== modified file 'Makefile'
--- Makefile	2013-08-27 18:18:35 +0000
+++ Makefile	2013-12-04 15:05:03 +0000
@@ -1,3 +1,12 @@
+# Makefile debugging hack: uncomment the two lines below and make will tell you
+# more about what is happening.  The output generated is of the form
+# "FILE:LINE [TARGET (DEPENDENCIES) (NEWER)]" where DEPENDENCIES are all the
+# things TARGET depends on and NEWER are all the files that are newer than
+# TARGET.  DEPENDENCIES will be colored green and NEWER will be blue.
+#OLD_SHELL := $(SHELL)
+#SHELL = $(warning [$@ ($^) ($?) ])$(OLD_SHELL)
+
+WD := $(shell pwd)
 DESTDIR = 
 prefix = /usr
 DDIR = $(DESTDIR)$(prefix)
@@ -8,7 +17,61 @@
 confdir = $(DESTDIR)/etc
 INSTALL = install
 
-all:
+bin/pip:
+	virtualenv .
+
+bin/python:
+	virtualenv .
+
+# We use a "canary" file to tell us if the package has been installed in
+# "develop" mode.
+DEVELOP_CANARY := lib/__develop_canary
+develop: $(DEVELOP_CANARY)
+$(DEVELOP_CANARY): | bin/python
+	bin/python setup.py develop
+	touch $(DEVELOP_CANARY)
+
+build: deps bin/python bin/pip develop bin/test
+
+sysdeps:
+	sudo apt-get update
+	sudo apt-get install -y build-essential bzr python-dev \
+	    python-virtualenv 
+
+dependencies:
+	bzr checkout lp:~juju-jitsu/charm-tools/dependencies
+
+# We use a "canary" file to tell us if the Python packages have been installed.
+PYTHON_PACKAGE_CANARY := lib/python2.7/site-packages/___canary
+python-deps: $(PYTHON_PACKAGE_CANARY)
+$(PYTHON_PACKAGE_CANARY): requirements.txt | bin/pip dependencies
+	bin/pip install --no-index --no-dependencies --find-links \
+	    file:///$(WD)/dependencies/python -r requirements.txt
+	touch $(PYTHON_PACKAGE_CANARY)
+
+deps: python-deps | dependencies
+
+bin/nosetests: python-deps
+
+bin/test: | bin/nosetests
+	ln scripts/test bin/test
+
+test: build bin/test
+	bin/test
+
+lint: sources = setup.py charmtools
+lint: build
+	@find $(sources) -name '*.py' -print0 | xargs -r0 bin/flake8
+
+tags:
+	ctags --tag-relative --python-kinds=-iv -Rf tags --sort=yes \
+	    --exclude=.bzr --languages=python
+
+clean:
+	find . -name '*.py[co]' -delete
+	find . -type f -name '*~' -delete
+	find . -name '*.bak' -delete
+	rm -rf bin include lib local man
 
 install:
 	$(INSTALL) -d $(mandir)
@@ -27,17 +90,30 @@
 check:
 	tests/helpers/helpers.sh || sh -x tests/helpers/helpers.sh timeout
 	@echo Test shell helpers with dash
-	bash tests/helpers/helpers.sh || bash -x tests/helpers/helpers.sh timeout
+	bash tests/helpers/helpers.sh \
+	    || bash -x tests/helpers/helpers.sh timeout
 	tests/helpers/helpers.bash || sh -x tests/helpers/helpers.bash timeout
 	@echo Test shell helpers with bash
-	bash tests/helpers/helpers.bash || bash -x tests/helpers/helpers.bash timeout
+	bash tests/helpers/helpers.bash \
+	    || bash -x tests/helpers/helpers.bash timeout
 	@echo Test charm proof
 	tests/proof/test.sh
 	tests/create/test.sh
-	PYTHONPATH=helpers/python python helpers/python/charmhelpers/tests/test_charmhelpers.py
-#	@echo PEP8 Lint of Python files
-#	@echo `grep -rl '^#!/.*python' .` | xargs -r -n1 pep8
-
-clean:
-	find . -name '*.pyc' -delete
-	find . -name '*.bak' -delete
+
+define phony
+  build
+  check
+  clean
+  deps
+  develop
+  install
+  lint
+  python-deps
+  sysdeps
+  tags
+  test
+endef
+
+.PHONY: $(phony)
+
+.DEFAULT_GOAL := build

=== modified file 'charmtools/__init__.py'
--- charmtools/__init__.py	2013-10-31 04:29:33 +0000
+++ charmtools/__init__.py	2013-12-04 15:05:03 +0000
@@ -16,7 +16,6 @@
 
 import os
 import sys
-import glob
 
 import subprocess
 

=== modified file 'charmtools/bundles.py'
--- charmtools/bundles.py	2013-11-04 20:59:11 +0000
+++ charmtools/bundles.py	2013-12-04 15:05:03 +0000
@@ -1,5 +1,4 @@
 import os
-import sys
 import yaml
 import glob
 import json

=== modified file 'charmtools/charms.py'
--- charmtools/charms.py	2013-11-04 20:41:01 +0000
+++ charmtools/charms.py	2013-12-04 15:05:03 +0000
@@ -1,5 +1,4 @@
 import os
-import sys
 
 import re
 import yaml

=== modified file 'charmtools/cli.py'
--- charmtools/cli.py	2013-11-01 01:53:58 +0000
+++ charmtools/cli.py	2013-12-04 15:05:03 +0000
@@ -1,7 +1,6 @@
 import os
 import sys
 import glob
-import argparse
 
 
 def parser_defaults(parser):

=== modified file 'charmtools/mr.py'
--- charmtools/mr.py	2013-10-31 04:13:18 +0000
+++ charmtools/mr.py	2013-12-04 15:05:03 +0000
@@ -16,7 +16,7 @@
 import os
 import ConfigParser
 
-from bzrlib import errors, trace
+from bzrlib import trace
 from bzrlib.bzrdir import BzrDir
 from bzrlib.branch import Branch
 from bzrlib.plugin import load_plugins

=== modified file 'charmtools/promulgate.py'
--- charmtools/promulgate.py	2013-10-20 18:23:16 +0000
+++ charmtools/promulgate.py	2013-12-04 15:05:03 +0000
@@ -24,12 +24,10 @@
 
 import os
 import sys
-import string
 
 from optparse import OptionParser
 
 from bzrlib import bzrdir
-from bzrlib.plugins.launchpad import lp_api
 
 import yaml
 

=== modified file 'charmtools/search.py'
--- charmtools/search.py	2013-10-31 04:13:18 +0000
+++ charmtools/search.py	2013-12-04 15:05:03 +0000
@@ -14,9 +14,6 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-import os
-import sys
-import re
 import argparse
 
 from . import charms

=== modified file 'charmtools/subscribers.py'
--- charmtools/subscribers.py	2013-10-31 04:13:18 +0000
+++ charmtools/subscribers.py	2013-12-04 15:05:03 +0000
@@ -123,7 +123,8 @@
     charms = []
     if len(args.charms):
         for charm_name in args.charms:
-            charm = charmdistro.getSourcePackage(name=charm_name)
+            # XXX Is this line really neccesary?
+            charmdistro.getSourcePackage(name=charm_name)
             charms.append(charm_name)
     else:
         branches = charmdistro.getBranchTips()

=== added directory 'charmtools/tests'
=== renamed file 'tests/proof/test_proof.py' => 'charmtools/tests/test_proof.py'
--- tests/proof/test_proof.py	2013-10-15 15:07:14 +0000
+++ charmtools/tests/test_proof.py	2013-12-04 15:05:03 +0000
@@ -25,7 +25,7 @@
 proof_path = dirname(dirname(dirname(abspath(__file__))))
 proof_path = join(proof_path, 'charmtools')
 sys.path.append(proof_path)
-from proof import Linter
+from charmtools.charms import CharmLinter
 
 
 class TestProof(TestCase):
@@ -33,7 +33,7 @@
     def setUp(self):
         self.charm_dir = mkdtemp()
         self.config_path = join(self.charm_dir, 'config.yaml')
-        self.linter = Linter()
+        self.linter = CharmLinter()
 
     def tearDown(self):
         rmtree(self.charm_dir)

=== modified file 'charmtools/update.py'
--- charmtools/update.py	2013-10-31 04:13:18 +0000
+++ charmtools/update.py	2013-12-04 15:05:03 +0000
@@ -52,5 +52,5 @@
         mr.save()
     except Exception as e:
         # Got this from http://stackoverflow.com/q/5574702/196832
-        print >> sys.stderr,  ".mrconfig not saved: ", e
+        print >> sys.stderr, ".mrconfig not saved: ", e
         sys.exit(1)

=== added file 'requirements.txt'
--- requirements.txt	1970-01-01 00:00:00 +0000
+++ requirements.txt	2013-12-04 15:05:03 +0000
@@ -0,0 +1,12 @@
+PyYAML==3.10
+launchpadlib==1.10.2
+nose==1.2.1
+requests==1.1.0
+lazr.authentication==0.1.2
+lazr.restfulclient==0.13.1
+lazr.uri==1.0.3
+simplejson==2.2.1
+wadllib==1.3.1
+httplib2==0.7.7
+oauth==1.0.1
+flake8==1.6.2

=== added directory 'scripts'
=== added file 'scripts/test'
--- scripts/test	1970-01-01 00:00:00 +0000
+++ scripts/test	2013-12-04 15:05:03 +0000
@@ -0,0 +1,7 @@
+#!/bin/bash
+# If the --pdb switch is passed, inject --pdb-failures too.
+if [[ $* =~ --pdb( .*|$) ]]
+then
+    extra_args="--pdb-failures"
+fi
+bin/nosetests --exe --with-id $extra_args $@

=== modified file 'setup.py'
--- setup.py	2013-11-01 03:10:36 +0000
+++ setup.py	2013-12-04 15:05:03 +0000
@@ -3,14 +3,13 @@
 # Copyright 2012 Canonical Ltd.  This software is licensed under the
 # GNU General Public License version 3 (see the file LICENSE).
 
-import sys
 import ez_setup
 
 
 ez_setup.use_setuptools()
 
 from charmtools.version import __VERSION__
-from setuptools import setup, find_packages
+from setuptools import setup
 
 
 setup(