Philip Withnall pointed out on Bug #88808 that there is now a standard autoconf-archive macro for coverage. We should use it if possible.
AX_COMPILER_FLAGS, AX_IS_RELEASE, AX_PKG_CHECK_MODULES, AX_VALGRIND_CHECK, AX_CHECK_ENABLE_DEBUG might also be interesting.
Created attachment 125739 [details] [review] [01] Use AX_CODE_COVERAGE for test-coverage statistics Please note that CODE_COVERAGE_LDFLAGS is supposed to be used with LIBS, not LDFLAGS.
Created attachment 125740 [details] [review] [02] Remove lcov-{reset,report,check} make targets and configuration
Comment on attachment 125739 [details] [review] [01] Use AX_CODE_COVERAGE for test-coverage statistics Review of attachment 125739 [details] [review]: ----------------------------------------------------------------- These fixes are sufficiently minor that I might just do them myself and push the result. ::: bus/Makefile.am @@ +32,5 @@ > # if assertions are enabled, improve backtraces > AM_LDFLAGS = @R_DYNAMIC_LDFLAG@ > > +AM_CFLAGS = \ > + $(CODE_COVERAGE_CFLAGS) Please append "\", newline, $(NULL) like the other multi-line lists. This avoids multi-line diffs when adding a single line in future. (Same in a few other places) ::: configure.ac @@ +338,5 @@ > + > +if test x$enable_code_coverage = xyes; then > + ## so that config.h changes when you toggle gcov support > + AC_DEFINE_UNQUOTED(DBUS_GCOV_ENABLED, 1, [Defined if gcov is enabled to force a rebuild due to config.h changing]) > +fi Please prefer AS_IF([test ... = xyes], [ # so that ... AC_DEFINE_UNQUOTED(...) ]) and consistently quote arguments to AC_DEFINE_UNQUOTED, avoiding "underquoting". I know the configure.ac is inconsistent about both of those at the moment - we'll fix it one day - but I would like new code to get it right. @@ +1945,5 @@ > if test x$enable_embedded_tests = xyes -a x$enable_asserts = xno; then > echo "NOTE: building with embedded tests but without assertions means tests may not properly report failures (this configuration is only useful when doing something like profiling the tests)" > fi > +if test x$enable_code_coverage = xyes; then > + echo "NOTE: building with coverage profiling is definitely for developers only." AS_IF ::: m4/ax_code_coverage.m4 @@ +1,2 @@ > +# =========================================================================== > +# http://www.gnu.org/software/autoconf-archive/ax_code_coverage.html I think I'd prefer to document that building from git requires autoconf-archive, like I did in dbus-python, and omit this file.
Comment on attachment 125740 [details] [review] [02] Remove lcov-{reset,report,check} make targets and configuration Review of attachment 125740 [details] [review]: ----------------------------------------------------------------- ::: Makefile.am @@ +36,4 @@ > # Add rules for code-coverage testing, as defined by AX_CODE_COVERAGE > @CODE_COVERAGE_RULES@ > > +# TODO: Remove these obsolete targets after some time. I'd just remove them altogether. Nobody except dbus developers should be using these (and to be honest we don't regularly use them ourselves). ::: configure.ac @@ -343,5 @@ > > -if test x$enable_compiler_coverage = xyes; then > - ## so that config.h changes when you toggle gcov support > - AC_DEFINE_UNQUOTED(DBUS_GCOV_ENABLED, 1, [Defined if gcov is enabled to force a rebuild due to config.h changing]) > -fi Oh, I see, you duplicated the existing logic with the other variable name, then deleted the duplicate afterwards? I would be inclined to do it all in one commit tbh. ::: m4/compiler.m4 @@ -51,5 @@ > [CFLAGS=`echo "$CFLAGS" | sed -e "s/ -O[1-9]*\b/ -O0/g"`] > fi])dnl > ])# COMPILER_OPTIMISATIONS > - > -# COMPILER_COVERAGE This file is pasted in from elsewhere, so I'd prefer to leave the unused macro in place. I wonder whether we use anything else from it? COMPILER_OPTIMISATIONS doesn't seem a worthwhile reason to keep it, if that's the only thing left: anyone who is serious about building distributions will already have a policy for what optimization flags they use, like Debian's "-O2 -g unless there is a good reason not to".
Hi! I have an update to this patch set. Please don't commit yet.
Sure, I'll wait. I would recommend doing it as a single patch - sure, it's somewhat large, but there doesn't seem a lot of point in having a transitional state where we briefly support two sets of code coverage tooling :-)
If you want to replace more macros with their AX_ equivalents, I'd be happy to review those too. You might find the commits leading up to 1.2.2 in https://cgit.freedesktop.org/dbus/dbus-python/log/ to be useful examples.
Created attachment 125753 [details] [review] [01] Use AX_CODE_COVERAGE for test-coverage statistics (v2) Changes since v1 review: - merged patches [01] and [02] - added $(NULL) lines to lists in 'Makefile.am's - replaced plain shell calls with M4sh and Autoconf macros - removed m4/ax_code_coverage.m4; requires Autoconf Archive for development - completely removed old make targets - restored m4/compiler.m4 and: - added @CODE_COVERAGE_RULES@ to test/Makefile.am - removed clean-local hooks; @CODE_COVERAGE_RULES@ provides clean-up rules
(In reply to Simon McVittie from comment #8) > If you want to replace more macros with their AX_ equivalents, I'd be happy > to review those too. You might find the commits leading up to 1.2.2 in > https://cgit.freedesktop.org/dbus/dbus-python/log/ to be useful examples. Sure, I looked at AX_VALGRIND_CHECK. It adds make targets like check-valgrind-memcheck to run tests under Valgrind tools; which seems like a pretty cool feature.
Comment on attachment 125753 [details] [review] [01] Use AX_CODE_COVERAGE for test-coverage statistics (v2) Review of attachment 125753 [details] [review]: ----------------------------------------------------------------- We should probably do the equivalent of https://cgit.freedesktop.org/dbus/dbus-python/commit/?id=00e36366aa437f592a8e7d9dad5a7764890f40b2 to make it clearer that autoconf-archive is required. I think that's sufficiently trivial that I'll apply it unreviewed, along with documenting the new requirement. ::: bus/Makefile.am @@ +35,5 @@ > +AM_CFLAGS = \ > + $(CODE_COVERAGE_CFLAGS) \ > + $(NULL) > + > +AM_CXXFLAGS = \ I don't think we compile any C++ in bus/? We do compile a tiny amount of C++ in dbus/ when targeting Windows, so this should probably move there. ::: m4/compiler.m4 @@ -56,5 @@ > -# ---------------------- > -# Add configure option to enable coverage data. > -AC_DEFUN([COMPILER_COVERAGE], > -[AC_ARG_ENABLE(compiler-coverage, > - AS_HELP_STRING([--enable-compiler-coverage], I know this macro is unused now, but please don't delete it from the "copylib" that we brought in from a third party project - we should keep third-party code unmodified where we can. I'll revert this part when merging.
Added in git for 1.11.4. Thanks!
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.