Bug 88922

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!