[Merge] lp:~allanlesage/pkgbinarymangler/coverage-mangler into lp:ubuntu/pkgbinarymangler
Martin Pitt
martin.pitt at ubuntu.com
Tue Aug 26 16:39:50 UTC 2014
Review: Needs Fixing
I did an initial review, as inline comments.
Diff comments:
> === modified file 'debian/changelog'
> --- debian/changelog 2014-03-18 07:50:06 +0000
> +++ debian/changelog 2014-08-25 23:25:55 +0000
> @@ -1,3 +1,9 @@
> +pkgbinarymangler (121~alesage24) utopic; urgency=medium
> +
> + * Test of producing coverage.xml under dh_ build.
> +
> + -- Allan LeSage <allan.lesage at canonical.com> Tue, 12 Aug 2014 14:32:24 -0500
> +
> pkgbinarymangler (120) trusty; urgency=medium
>
> * pkgstripfiles: Fix escaping of % characters to also work with bash 4.3.
>
> === modified file 'debian/control'
> --- debian/control 2014-01-15 09:28:07 +0000
> +++ debian/control 2014-08-25 23:25:55 +0000
> @@ -18,7 +18,10 @@
> Depends: ${misc:Depends},
> lockfile-progs,
> advancecomp,
> - optipng
> + optipng,
> + gcovr,
These are some fairly heavy new dependencies. They might be too heavy for the Launchpad buildd chroots, they pull in python2 in particular. That should be discussed with Adam Conrad.
If it's too heavy, then gcovr needs to be dropped, and would need to become an extra build dependency of the packages that want to produce coverage.
> + lcov,
> + cmake-extras,
> Description: strips translations and alters maintainers during build
> pkgbinarymangler consists of a dpkg-deb wrapper that calls the following
> helper applications while building a debian binary package:
>
> === modified file 'debian/pkgbinarymangler.install'
> --- debian/pkgbinarymangler.install 2011-11-29 08:59:57 +0000
> +++ debian/pkgbinarymangler.install 2014-08-25 23:25:55 +0000
> @@ -1,5 +1,9 @@
> dpkg-deb usr/bin
> dh_builddeb usr/bin
> +src/dh_auto_configure usr/bin
> +src/dh_auto_build usr/bin
> +src/dh_auto_test usr/bin
> +dpkg-gensymbols usr/bin
> pkgstriptranslations usr/bin
> pkgmaintainermangler usr/bin
> pkgsanitychecks usr/bin
>
> === modified file 'debian/pkgbinarymangler.postrm'
> --- debian/pkgbinarymangler.postrm 2011-11-29 08:59:57 +0000
> +++ debian/pkgbinarymangler.postrm 2014-08-25 23:25:55 +0000
> @@ -7,6 +7,10 @@
> # Remove our diversions
> dpkg-divert --remove --rename --package pkgbinarymangler --divert /usr/bin/dpkg-deb.pkgbinarymangler /usr/bin/dpkg-deb > /dev/null
> dpkg-divert --remove --rename --package pkgbinarymangler --divert /usr/bin/dh_builddeb.pkgbinarymangler /usr/bin/dh_builddeb > /dev/null
> + dpkg-divert --remove --rename --package pkgbinarymangler --divert /usr/bin/pkgbinarymangler/dh_auto_configure /usr/bin/dh_auto_configure > /dev/null
> + dpkg-divert --remove --rename --package pkgbinarymangler --divert /usr/bin/pkgbinarymangler/dh_auto_build /usr/bin/dh_auto_build > /dev/null
> + dpkg-divert --remove --rename --package pkgbinarymangler --divert /usr/bin/pkgbinarymangler/dh_auto_test /usr/bin/dh_auto_test > /dev/null
> + dpkg-divert --remove --rename --package pkgbinarymangler --divert /usr/bin/dpkg-gensymbols.pkgbinarymangler /usr/bin/dpkg-gensymbols > /dev/null
> ;;
> esac
>
>
> === modified file 'debian/pkgbinarymangler.preinst'
> --- debian/pkgbinarymangler.preinst 2011-11-29 09:19:59 +0000
> +++ debian/pkgbinarymangler.preinst 2014-08-25 23:25:55 +0000
> @@ -14,6 +14,11 @@
> fi
>
> dpkg-divert --add --rename --package pkgbinarymangler --divert /usr/bin/dh_builddeb.pkgbinarymangler /usr/bin/dh_builddeb > /dev/null
> + mkdir -p /usr/bin/pkgbinarymangler
> + dpkg-divert --add --rename --package pkgbinarymangler --divert /usr/bin/pkgbinarymangler/dh_auto_configure /usr/bin/dh_auto_configure > /dev/null
> + dpkg-divert --add --rename --package pkgbinarymangler --divert /usr/bin/pkgbinarymangler/dh_auto_build /usr/bin/dh_auto_build > /dev/null
> + dpkg-divert --add --rename --package pkgbinarymangler --divert /usr/bin/pkgbinarymangler/dh_auto_test /usr/bin/dh_auto_test > /dev/null
> + dpkg-divert --add --rename --package pkgbinarymangler --divert /usr/bin/dpkg-gensymbols.pkgbinarymangler /usr/bin/dpkg-gensymbols > /dev/null
> ;;
> esac
>
>
> === added file 'dpkg-gensymbols'
> --- dpkg-gensymbols 1970-01-01 00:00:00 +0000
> +++ dpkg-gensymbols 2014-08-25 23:25:55 +0000
> @@ -0,0 +1,10 @@
> +#!/bin/bash
> +
> +set -e
> +
> +# gcov introduces mismatches; set to lowest check level
> +export DPKG_GENSYMBOLS_CHECK_LEVEL=0
Note that this is not acceptable for trunk/Ubuntu. It's ok as a temporary hack for playing around with this, but it's really not ok to expose private symbols in public libraries as public ABI; it's a disaster waiting to happen. So ideally this would be fixed in gcov itself, by putting coverage information of private (or just all) symbols into a separate ELF section, if they are necessary at all; debug symbols are handled in a similar fashion.
If that's not possible or too hard to do, then doing a separate build just for coverage would be a better hack (although very resource intensive, as it doubles build time). But maybe that's cleaner anyway as it avoids having to carry the coverage information in production binaries?
> +
> +# TODO filter options, eliminating -c
> +DPKG_GENSYMBOLS_OPTS=$@
> +exec dpkg-gensymbols.pkgbinarymangler $DPKG_GENSYMBOLS_OPTS
>
> === added directory 'src'
> === added file 'src/dh_auto_build'
> --- src/dh_auto_build 1970-01-01 00:00:00 +0000
> +++ src/dh_auto_build 2014-08-25 23:25:55 +0000
> @@ -0,0 +1,9 @@
> +#!/bin/bash
/bin/sh should be quite enough for most of the new scripts. Please only use bash if you use bash specific features (most of the existing scripts use "exec -a" which is a bashism).
> +set -e
> +
> +/usr/bin/pkgbinarymangler/dh_auto_build $@
> +
> +echo "Verify that we produced .gcno artifacts."
> +
> +find -name "*.o"
> +find -name "*.gcno"
This does not actually do anything -- is that just for testing that everything went as expected? This can produce a lot of noise which is hard to evaluate as a human; I suggest to rather iterate over all *.o files and only print those which do *not* have a corresponding *.gcno. This is a lot more useful then.
>
> === added file 'src/dh_auto_configure'
> --- src/dh_auto_configure 1970-01-01 00:00:00 +0000
> +++ src/dh_auto_configure 2014-08-25 23:25:55 +0000
> @@ -0,0 +1,41 @@
> +#!/bin/bash
> +
> +set -e
> +
> +echo "Diverted dh_auto_configure enabling coverage build."
> +
Not necessary, this will always be the case then. The more specific echo's further down are more useful.
> +DH_AUTO_CONFIGURE_OPTS=$@
> +
> +enable_coverage_report() {
> + if $(echo $DH_AUTO_CONFIGURE_OPTS | grep '.*-- .*' > /dev/null); then
Use grep -q instead of redirection; also, the .* are redundant. Or even more efficiently (avoids an external program): if [ "${DH_AUTO_CONFIGURE_OPTS%-- *}" != "$DH_AUTO_CONFIGURE_OPTS" ]; then
> + # slip our option in after the --
> + DH_AUTO_CONFIGURE_OPTS=$(echo "$DH_AUTO_CONFIGURE_OPTS" | sed 's/\(^.*-- \)\(.*\)/\1'"$1"' \2/')
> + else
> + DH_AUTO_CONFIGURE_OPTS="$DH_AUTO_CONFIGURE_OPTS -- $1"
> + fi
> +}
> +
> +if [ -f Makefile.am.coverage ]; then
> + # 2011-era alesage effort added Makefile.am.coverage macro for autotools projects
> + # http://qualityhour.wordpress.com/2012/01/29/test-coverage-tutorial-for-cc-autotools-projects/
> + echo "Enabling coverage reporting per Makefile.am.coverage."
> + enable_coverage_report "--enable-gcov"
> +elif [ -f coverage.pri ]; then
> + # An unknown genius (vrruiz?) introduced coverage.pri for qmake projects
> + echo "Enabling coverage reporting per coverage.pri."
> + enable_coverage_report "CONFIG+=coverage"
> +elif grep -ri "if.*cmake_build_type.*c.*o.*v.*e.*r.*a.*g.*e" > /dev/null; then
grep -q again, but that seems a bit dangerous -- e. g. this could also be in some README file, renamed file etc. Better do
if find -name '*.whatever' -print0 | xargs -0 grep -qi "if..."; then
> + # CMake projects virally added cmake/EnableCoverageReport.cmake (filenames vary)
> + # Above grep could be more clever but not as clever as
> + # IF(CMAKE_BUILD_TYPE MATCHES [cC][oO][vV][eE][rR][aA][gG][eE])
> + # Recursive grep to find options hidden in src/CMakeLists.txt e.g.
> + echo "Enabling coverage reporting per CMakeLists.txt CMAKE_BUILD_TYPE instruction."
> + if $(echo "$DH_AUTO_CONFIGURE_OPTS" | grep "\-DCMAKE_BUILD_TYPE" > /dev/null); then
> + # correct CMAKE_BUILD_TYPE to coverage
> + DH_AUTO_CONFIGURE_OPTS=$(echo "$DH_AUTO_CONFIGURE_OPTS" | sed 's/^\(.*-DCMAKE_BUILD_TYPE.*=\)[^ $]*/\1coverage/')
There doesn't need to be any separator before the new "coverage"? Like a space or comma?
> + else
> + enable_coverage_report "-DCMAKE_BUILD_TYPE=coverage"
> + fi
> +fi
> +
> +exec /usr/bin/pkgbinarymangler/dh_auto_configure $DH_AUTO_CONFIGURE_OPTS
This won't work with running the mangler out of the source tree if pkgbinarymangler is not installed. Perhaps something like $(dirname $0)/pkgbinarymangler/dh_auto_configure ? That should work for both.
>
> === added file 'src/dh_auto_test'
> --- src/dh_auto_test 1970-01-01 00:00:00 +0000
> +++ src/dh_auto_test 2014-08-25 23:25:55 +0000
> @@ -0,0 +1,17 @@
> +#!/bin/bash
> +set -e
> +
> +/usr/bin/pkgbinarymangler/dh_auto_test $@
> +
> +echo "dh_auto_test runs gcovr to produce coverage.xml."
> +
> +# TODO if no gcdas then exit in disgust
> +find -name "*.gcda"
Please don't let that output to stdout -- if you just need the exit code, use >/dev/null.
But anyway, there are a lot of packages where these won't exist, so you have to deal with packages that don't produce coverage. Please add a test case which uses dh_auto_test on a simple arch:all package, i. e. doesn't compile anything. (Same would be true for a Go/Python/Perl/Erlang/etc. package).
> +
> +# NOTE that pkgbinarymangler depends on gcovr (in debian/control)
> +gcovr --xml -r $(pwd) -o coverage.xml --exclude='.*test.*' --exclude='.*gmock.*' --exclude='/usr/.*'
> +
> +# on wgrant's recommendation we dump coverage.xml to the log
> +echo "===== BEGIN coverage.xml ====="
> +cat coverage.xml
> +echo "===== END coverage.xml ====="
>
Instead, this whole code should be wrapped into an if condition that checks whether gcovr is available (see dep/build dep issue from above), and whether there are any *.gcda files.
> === modified file 'test/run'
> --- test/run 2014-01-15 09:21:27 +0000
> +++ test/run 2014-08-25 23:25:55 +0000
> @@ -38,6 +38,13 @@
> os.symlink('/usr/bin/dh_builddeb.pkgbinarymangler', os.path.join(self.srcdir, 'dh_builddeb.pkgbinarymangler'))
> else:
> os.symlink('/usr/bin/dh_builddeb', os.path.join(self.srcdir, 'dh_builddeb.pkgbinarymangler'))
> + # debhelper objects to '.pkgbinarymangler' suffix for dh_auto_configure; instead install to special dir
> + os.mkdir(os.path.join(self.srcdir, 'pkgbinarymangler'))
> + os.environ['PATH'] = os.path.join(self.srcdir, 'pkgbinarymangler') + os.pathsep + os.environ['PATH']
> + if os.path.exists('/usr/bin/pkgbinarymangler/dh_auto_configure'):
> + os.symlink('/usr/bin/pkgbinarymangler/dh_auto_configure', os.path.join(self.srcdir, 'pkgbinarymangler', 'dh_auto_configure'))
> + else:
> + os.symlink('/usr/bin/dh_auto_configure', os.path.join(self.srcdir, 'pkgbinarymangler', 'dh_auto_configure'))
>
> # copy our default configuration files, and enable them
> for conf in glob(os.path.join(self.srcdir, '*.conf')):
> @@ -59,6 +66,9 @@
> shutil.rmtree(self.workdir)
> os.unlink(os.path.join(self.srcdir, 'dpkg-deb.pkgbinarymangler'))
> os.unlink(os.path.join(self.srcdir, 'dh_builddeb.pkgbinarymangler'))
> + os.unlink(os.path.join(self.srcdir, 'pkgbinarymangler', 'dh_auto_configure'))
> + os.rmdir(os.path.join(self.srcdir, 'pkgbinarymangler'))
> + os.environ['PATH'] = os.environ['PATH'].replace(os.path.join(self.srcdir, 'pkgbinarymangler') + os.pathsep, '')
>
> def build(self, use_local_mangler=True, extra_env=None, srcname='icecream'):
> env = os.environ.copy()
>
--
https://code.launchpad.net/~allanlesage/pkgbinarymangler/coverage-mangler/+merge/232127
Your team Ubuntu branches is subscribed to branch lp:ubuntu/pkgbinarymangler.
More information about the Ubuntu-reviews
mailing list