Merge lp:~jtv/pyjuju/makefile-fixups into lp:pyjuju

Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Kapil Thangavelu
Approved revision: 618
Merged at revision: 620
Proposed branch: lp:~jtv/pyjuju/makefile-fixups
Merge into: lp:pyjuju
Diff against target: 55 lines (+27/-17)
1 file modified
Makefile (+27/-17)
To merge this branch: bzr merge lp:~jtv/pyjuju/makefile-fixups
Reviewer Review Type Date Requested Status
Kapil Thangavelu (community) Approve
Ian Booth Approve
Review via email: mp+149907@code.launchpad.net

Commit message

Get "make" targets check, review etc. to work, and warn about missing pep8/pyflakes.

Description of the change

Makefile improvements and fixes.

Whenever I tried to run "make check" or "make review" in a juju branch, I would get output like:

bzr: ERROR: Not a branch: "/".
    bzr: ERROR: Not a branch: "/".
    bzr: ERROR: Not a branch: "/".
    bzr: ERROR: Not a branch: "/".
    make: *** [check] Error 1

The errors seem to happen (and I'll admit I don't know how or why) because
there are several lines in the Makefile each defining $(modified) as a shell
command involving bzr. I get the same result on a cloud machine as on my development system.
Giving each of those variables a separate name fixed the errors.

There were also other problems: the linters weren't actually being run, there
was no helpful indication of pep8 and/or pyflakes being missing, newly added
files weren't being checked, greps for ".py" suffixes sometimes neglectedto
escape the dot (so it stood for "any character followed by py"). I fixed all of those.

Finally there were some cosmetic nuisances. Spacing of the command lines was
irregular, and some use of Perl-style regexes in grep became unnecessary.

One of the checks in the "review" target was commented out. I don't know why,
so I left that as-is.

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Oh, and I documented some targets, since I was figuring out what they were meant to do anyway.

Revision history for this message
Ian Booth (wallyworld) wrote :

Looks like nice cleanup.

review: Approve
Revision history for this message
Kapil Thangavelu (hazmat) wrote :

looks good, thanks.

review: Approve
Revision history for this message
Kapil Thangavelu (hazmat) wrote :

*** Submitted:

Makefile improvements and fixes.

Whenever I tried to run "make check" or "make review" in a juju branch, I would get output like:

bzr: ERROR: Not a branch: "/".
    bzr: ERROR: Not a branch: "/".
    bzr: ERROR: Not a branch: "/".
    bzr: ERROR: Not a branch: "/".
    make: *** [check] Error 1

The errors seem to happen (and I'll admit I don't know how or why) because
there are several lines in the Makefile each defining $(modified) as a shell
command involving bzr. I get the same result on a cloud machine as on my development system.
Giving each of those variables a separate name fixed the errors.

There were also other problems: the linters weren't actually being run, there
was no helpful indication of pep8 and/or pyflakes being missing, newly added
files weren't being checked, greps for ".py" suffixes sometimes neglectedto
escape the dot (so it stood for "any character followed by py"). I fixed all of those.

Finally there were some cosmetic nuisances. Spacing of the command lines was
irregular, and some use of Perl-style regexes in grep became unnecessary.

One of the checks in the "review" target was commented out. I don't know why,
so I left that as-is.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'Makefile'
2--- Makefile 2012-09-27 01:43:32 +0000
3+++ Makefile 2013-02-21 18:42:36 +0000
4@@ -21,24 +21,34 @@
5 etags:
6 @ctags -e --python-kinds=-iv -R juju
7
8-modified=$(shell bzr status -S |grep -P '^\s*M' | awk '{print $$2;}'| grep -P ".py$$")
9-check:
10- @test -n "$(modified)" && echo $(modified) | xargs $(PEP8) --repeat
11- @test -n "$(modified)" && echo $(modified) | xargs pyflakes
12-
13-
14-modified=$(shell bzr status -S -r ancestor:$(JUJU_TRUNK) |grep -P '^\s*M' | awk '{print $$2;}'| grep -P ".py$$")
15-review:
16- #@test -n "$(modified)" && echo $(modified) | xargs $(PEP8) --repeat
17- @test -n "$(modified)" && echo $(modified) | xargs pyflakes
18-
19-
20-modified=$(shell bzr status -S -r branch::prev |grep -P '^\s*\+?[MN]' | awk '{print $$2;}'| grep -P "test_.*\.py$$")
21+
22+present_pep8=$(shell which $(PEP8))
23+present_pyflakes=$(shell which pyflakes)
24+warn_missing_linters:
25+ @test -n "$(present_pep8)" || echo "WARNING: $(PEP8) not installed."
26+ @test -n "$(present_pyflakes)" || echo "WARNING: pyflakes not installed."
27+
28+
29+# "check": Check uncommitted changes for lint.
30+check_changes=$(shell bzr status -S | grep '^[ +]*[MN]' | awk '{print $$2;}' | grep "\\.py$$")
31+check: warn_missing_linters
32+ @test -z $(present_pep8) || (echo $(check_changes) | xargs -r $(PEP8) --repeat)
33+ @test -z $(present_pyflakes) || (echo $(check_changes) | xargs -r pyflakes)
34+
35+
36+# "review": Check all changes compared to trunk for lint.
37+review_changes=$(shell bzr status -S -r ancestor:$(JUJU_TRUNK) | grep '^[ +]*[MN]' | awk '{print $$2;}' | grep "\\.py$$")
38+review: warn_missing_linters
39+ #@test -z $(present_pep8) || (echo $(review_changes) | xargs -r $(PEP8) --repeat)
40+ @test -z $(present_pyflakes) || (echo $(review_changes) | xargs -r pyflakes)
41+
42+
43+ptests_changes=$(shell bzr status -S -r branch::prev | grep -P '^[ +]*[MN]' | awk '{print $$2;}'| grep "test_.*\\.py$$")
44 ptests:
45- @test -n "$(modified)" && echo $(modified) | xargs ./test
46+ @echo $(ptests_changes) | xargs -r ./test
47
48-modified=$(shell bzr status -S -r ancestor:$(JUJU_TRUNK)/|grep -P 'test.*\.py' |awk '{print $$2;}')
49+btests_changes=$(shell bzr status -S -r ancestor:$(JUJU_TRUNK)/ | grep "test.*\\.py$$" | awk '{print $$2;}')
50 btests:
51- @./test $(modified)
52+ @./test $(btests_changes)
53
54-.PHONY: tags check review
55+.PHONY: tags check review warn_missing_linters

Subscribers

People subscribed via source and target branches

to status/vote changes: