Bug 88922 - switch to AX_CODE_COVERAGE
Summary: switch to AX_CODE_COVERAGE
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Thomas Zimmermann
QA Contact: D-Bus Maintainers
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 97357 98191 98192 98195
  Show dependency treegraph
 
Reported: 2015-02-02 17:43 UTC by Simon McVittie
Modified: 2016-10-10 14:54 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
[01] Use AX_CODE_COVERAGE for test-coverage statistics (18.10 KB, patch)
2016-08-12 15:07 UTC, Thomas Zimmermann
Details | Splinter Review
[02] Remove lcov-{reset,report,check} make targets and configuration (5.14 KB, patch)
2016-08-12 15:07 UTC, Thomas Zimmermann
Details | Splinter Review
[01] Use AX_CODE_COVERAGE for test-coverage statistics (v2) (9.59 KB, patch)
2016-08-12 19:08 UTC, Thomas Zimmermann
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2015-02-02 17:43:19 UTC
Philip Withnall pointed out on Bug #88808 that there is now a standard autoconf-archive macro for coverage. We should use it if possible.
Comment 1 Simon McVittie 2015-02-02 18:35:05 UTC
AX_COMPILER_FLAGS, AX_IS_RELEASE, AX_PKG_CHECK_MODULES, AX_VALGRIND_CHECK, AX_CHECK_ENABLE_DEBUG might also be interesting.
Comment 2 Thomas Zimmermann 2016-08-12 15:07:00 UTC
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.
Comment 3 Thomas Zimmermann 2016-08-12 15:07:38 UTC
Created attachment 125740 [details] [review]
[02] Remove lcov-{reset,report,check} make targets and configuration
Comment 4 Simon McVittie 2016-08-12 18:07:15 UTC
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 5 Simon McVittie 2016-08-12 18:11:16 UTC
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".
Comment 6 Thomas Zimmermann 2016-08-12 18:14:39 UTC
Hi!

I have an update to this patch set. Please don't commit yet.
Comment 7 Simon McVittie 2016-08-12 18:17:30 UTC
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 :-)
Comment 8 Simon McVittie 2016-08-12 18:20:33 UTC
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.
Comment 9 Thomas Zimmermann 2016-08-12 19:08:15 UTC
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
Comment 10 Thomas Zimmermann 2016-08-12 19:11:44 UTC
(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 11 Simon McVittie 2016-08-15 13:18:00 UTC
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.
Comment 12 Simon McVittie 2016-08-15 13:52:35 UTC
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.