| Message ID | 1450804653-15006-2-git-send-email-russell@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Dec 22, 2015 at 12:17:23PM -0500, Russell Bryant wrote: > If flake8 is installed, run it at build time. Similar to most Makefile > targets, run it once and then only run again if the files change. > > flake8 is set to ignore all error and warning types that currently occur. > Future patches will remove items from the ignore list as they are > resolved. > > Signed-off-by: Russell Bryant <russell@ovn.org> Would you mind adding a mention of this tool to INSTALL.md, probably somewhere around where "sparse" is already mentioned? I'd mark the new pep8-check target "phony" in Makefile.am. I'd think that the flake8 target would be flake8-check, not pep8-check. Would there be any value in a FLAKE8_FLAGS variable to supplement (replace?) the default flags? Would you mind adding $(AM_V_GEN) to the beginning of the Makefile command, so that the "make" output is cleaner for those of us who use the abbreviated output? That's a lot of complaints but I actually like this, thanks. Acked-by: Ben Pfaff <blp@ovn.org>
On 01/04/2016 07:23 PM, Ben Pfaff wrote: > On Tue, Dec 22, 2015 at 12:17:23PM -0500, Russell Bryant wrote: >> If flake8 is installed, run it at build time. Similar to most Makefile >> targets, run it once and then only run again if the files change. >> >> flake8 is set to ignore all error and warning types that currently occur. >> Future patches will remove items from the ignore list as they are >> resolved. >> >> Signed-off-by: Russell Bryant <russell@ovn.org> > > Would you mind adding a mention of this tool to INSTALL.md, probably > somewhere around where "sparse" is already mentioned? done. > I'd mark the new pep8-check target "phony" in Makefile.am. I didn't mark it as phony because the target actually generates a "pep8-check" file to make sure flake8 only runs if any Python code has changed since the last time it ran. Let me know if I'm misunderstanding .PHONY or if you have a better suggestion for accomplishing the goal. > I'd think that the flake8 target would be flake8-check, not pep8-check. Makes sense, done. > Would there be any value in a FLAKE8_FLAGS variable to supplement > (replace?) the default flags? It certainly won't hurt. I'll add it. > Would you mind adding $(AM_V_GEN) to the beginning of the Makefile > command, so that the "make" output is cleaner for those of us who use > the abbreviated output? nice, I didn't know about the abbreviated output. done. > That's a lot of complaints but I actually like this, thanks. not too bad. :-) > Acked-by: Ben Pfaff <blp@ovn.org> Thanks for the review!
On Tue, Jan 05, 2016 at 11:23:24AM -0500, Russell Bryant wrote: > On 01/04/2016 07:23 PM, Ben Pfaff wrote: > > On Tue, Dec 22, 2015 at 12:17:23PM -0500, Russell Bryant wrote: > >> If flake8 is installed, run it at build time. Similar to most Makefile > >> targets, run it once and then only run again if the files change. > >> > >> flake8 is set to ignore all error and warning types that currently occur. > >> Future patches will remove items from the ignore list as they are > >> resolved. > >> > >> Signed-off-by: Russell Bryant <russell@ovn.org> > > I'd mark the new pep8-check target "phony" in Makefile.am. > > I didn't mark it as phony because the target actually generates a > "pep8-check" file to make sure flake8 only runs if any Python code has > changed since the last time it ran. > > Let me know if I'm misunderstanding .PHONY or if you have a better > suggestion for accomplishing the goal. You're right, sorry, my mistake. I actually told someone else the same thing about a different target in the same Makefile a while back, you'd think I'd remember my own previous logic.
diff --git a/Makefile.am b/Makefile.am index 966ba27..c08489d 100644 --- a/Makefile.am +++ b/Makefile.am @@ -141,6 +141,7 @@ SUFFIXES = check_DATA = check_SCRIPTS = pkgconfig_DATA = +FLAKE8_PYFILES = scriptsdir = $(pkgdatadir)/scripts completiondir = $(sysconfdir)/bash_completion.d @@ -338,6 +339,12 @@ manpage-check: $(man_MANS) $(dist_man_MANS) $(noinst_man_MANS) CLEANFILES += manpage-check endif +if HAVE_FLAKE8 +ALL_LOCAL += pep8-check +pep8-check: $(FLAKE8_PYFILES) + if flake8 $^ --ignore=E111,E112,E113,E123,E126,E127,E128,E129,E131,E201,E203,E222,E225,E226,E231,E241,E251,E261,E262,E265,E271,E302,E303,E501,E502,E703,E711,E713,E721,F401,F811,F821,F841,W601; then touch $@; else exit 1; fi +endif + include $(srcdir)/manpages.mk $(srcdir)/manpages.mk: $(MAN_ROOTS) build-aux/sodepends.pl @$(PERL) $(srcdir)/build-aux/sodepends.pl -I. -I$(srcdir) $(MAN_ROOTS) >$(@F).tmp diff --git a/configure.ac b/configure.ac index 8418aea..49aa182 100644 --- a/configure.ac +++ b/configure.ac @@ -95,6 +95,7 @@ OVS_CHECK_OPENSSL OVS_CHECK_LIBCAPNG OVS_CHECK_LOGDIR OVS_CHECK_PYTHON +OVS_CHECK_FLAKE8 OVS_CHECK_DOT OVS_CHECK_IF_PACKET OVS_CHECK_IF_DL diff --git a/debian/automake.mk b/debian/automake.mk index c29a560..0e30971 100644 --- a/debian/automake.mk +++ b/debian/automake.mk @@ -58,6 +58,9 @@ EXTRA_DIST += \ debian/ifupdown.sh \ debian/source/format +FLAKE8_PYFILES += \ + debian/ovs-monitor-ipsec + check-debian-changelog-version: @DEB_VERSION=`echo '$(VERSION)' | sed 's/pre/~pre/'`; \ if $(FGREP) '($(DEB_VERSION)' $(srcdir)/debian/changelog >/dev/null; \ diff --git a/m4/openvswitch.m4 b/m4/openvswitch.m4 index 0cfaae6..6d4e5da 100644 --- a/m4/openvswitch.m4 +++ b/m4/openvswitch.m4 @@ -353,6 +353,18 @@ else: AM_CONDITIONAL([HAVE_PYTHON], [test "$HAVE_PYTHON" = yes])]) dnl Checks for dot. +AC_DEFUN([OVS_CHECK_FLAKE8], + [AC_CACHE_CHECK( + [for flake8], + [ovs_cv_flake8], + [if flake8 --version >/dev/null 2>&1; then + ovs_cv_flake8=yes + else + ovs_cv_flake8=no + fi]) + AM_CONDITIONAL([HAVE_FLAKE8], [test "$ovs_cv_flake8" = yes])]) + +dnl Checks for dot. AC_DEFUN([OVS_CHECK_DOT], [AC_CACHE_CHECK( [for dot], diff --git a/ofproto/automake.mk b/ofproto/automake.mk index 0058ff3..50e7507 100644 --- a/ofproto/automake.mk +++ b/ofproto/automake.mk @@ -82,3 +82,5 @@ ofproto/ipfix-entities.def: ofproto/ipfix.xml ofproto/ipfix-gen-entities # IPFIX enterprise entity definition macros. EXTRA_DIST += ofproto/ipfix-enterprise-entities.def + +FLAKE8_PYFILES += ofproto/ipfix-gen-entities diff --git a/python/automake.mk b/python/automake.mk index 42b428a..177d4e3 100644 --- a/python/automake.mk +++ b/python/automake.mk @@ -50,6 +50,13 @@ PYFILES = $(ovs_pyfiles) python/ovs/dirs.py $(ovstest_pyfiles) EXTRA_DIST += $(PYFILES) PYCOV_CLEAN_FILES += $(PYFILES:.py=.py,cover) +FLAKE8_PYFILES += \ + $(PYFILES) \ + python/setup.py \ + python/build/__init__.py \ + python/build/nroff.py \ + python/ovs/dirs.py + if HAVE_PYTHON nobase_pkgdata_DATA = $(ovs_pyfiles) $(ovstest_pyfiles) ovs-install-data-local: diff --git a/tests/automake.mk b/tests/automake.mk index bd06a51..33d887a 100644 --- a/tests/automake.mk +++ b/tests/automake.mk @@ -349,6 +349,8 @@ CHECK_PYFILES = \ EXTRA_DIST += $(CHECK_PYFILES) PYCOV_CLEAN_FILES += $(CHECK_PYFILES:.py=.py,cover) .coverage +FLAKE8_PYFILES += $(CHECK_PYFILES) + if HAVE_OPENSSL TESTPKI_FILES = \ tests/testpki-cacert.pem \ diff --git a/utilities/automake.mk b/utilities/automake.mk index d5d1c33..d08347e 100644 --- a/utilities/automake.mk +++ b/utilities/automake.mk @@ -153,4 +153,6 @@ bin_PROGRAMS += utilities/ovs-benchmark utilities_ovs_benchmark_SOURCES = utilities/ovs-benchmark.c utilities_ovs_benchmark_LDADD = lib/libopenvswitch.la +FLAKE8_PYFILES += utilities/ovs-pcap.in + include utilities/bugtool/automake.mk diff --git a/vtep/automake.mk b/vtep/automake.mk index de028b6..2645f30 100644 --- a/vtep/automake.mk +++ b/vtep/automake.mk @@ -43,6 +43,8 @@ scripts_SCRIPTS += \ docs += vtep/README.ovs-vtep.md EXTRA_DIST += vtep/ovs-vtep +FLAKE8_PYFILES += vtep/ovs-vtep + # VTEP schema and IDL EXTRA_DIST += vtep/vtep.ovsschema pkgdata_DATA += vtep/vtep.ovsschema diff --git a/xenserver/automake.mk b/xenserver/automake.mk index 816b1b5..1f75e3a 100644 --- a/xenserver/automake.mk +++ b/xenserver/automake.mk @@ -26,6 +26,9 @@ EXTRA_DIST += \ xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync \ xenserver/usr_share_openvswitch_scripts_sysconfig.template +FLAKE8_PYFILES += \ + xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync + $(srcdir)/xenserver/openvswitch-xen.spec: xenserver/openvswitch-xen.spec.in $(top_builddir)/config.status $(AM_V_GEN)($(ro_shell) && sed -e 's,[@]VERSION[@],$(VERSION),g') \ < $(srcdir)/xenserver/$(@F).in > $(@F).tmp || exit 1; \
If flake8 is installed, run it at build time. Similar to most Makefile targets, run it once and then only run again if the files change. flake8 is set to ignore all error and warning types that currently occur. Future patches will remove items from the ignore list as they are resolved. Signed-off-by: Russell Bryant <russell@ovn.org> --- Makefile.am | 7 +++++++ configure.ac | 1 + debian/automake.mk | 3 +++ m4/openvswitch.m4 | 12 ++++++++++++ ofproto/automake.mk | 2 ++ python/automake.mk | 7 +++++++ tests/automake.mk | 2 ++ utilities/automake.mk | 2 ++ vtep/automake.mk | 2 ++ xenserver/automake.mk | 3 +++ 10 files changed, 41 insertions(+)