| Summary: | config-parser.c is too big or complex (?) for gcov to cope with | ||
|---|---|---|---|
| Product: | dbus | Reporter: | Bernard Leak <bernard> |
| Component: | core | Assignee: | Simon McVittie <smcv> |
| Status: | RESOLVED FIXED | QA Contact: | John (J5) Palmieri <johnp> |
| Severity: | minor | ||
| Priority: | low | CC: | bernard, hp, smcv |
| Version: | unspecified | Keywords: | patch |
| Hardware: | All | ||
| OS: | All | ||
| URL: | http://git.collabora.co.uk/?p=user/smcv/dbus-smcv.git;a=shortlog;h=refs/heads/config-parser-gcov-10887 | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
Don't ignore all files in m4, just the libtool ones
Import compiler.m4 and lcov.am from telepathy-glib, and use them to replace gcov Break up the monster conditional in config-parser so gcov can cope |
||
|
Description
Bernard Leak
2007-05-08 15:32:55 UTC
I don't think the gcov stuff has worked in a long time, the format gcc produces changed once and I updated for it, then it changed at least once more and the gcov support was never updated to match. The way the --enable options work is that the default is "auto" (enable if possible) and yes means fail if it can't be enabled, no means force disable. The purpose of "yes" is to force the build to fail in e.g. a spec file if something goes wrong with the configure check. (In reply to comment #1) > The purpose of "yes" is to force the build to fail in e.g. a spec file if > something goes wrong with the configure check. I think my wording above was correct, but perhaps too terse to be helpful. I distinguish between configuration steps and build steps. I expected - that is, felt entitled to insist - that the configuration step fail, given that I didn't believe my gcov would work. The configuration step completed with no apparent errors: the failure came during the subsequent build. Just what it shouldn't do... Indeed the gcov check needs fixing. I'm not sure, to be honest, which gcc versions the gcov stuff currently in-tree works with, if any, though... It may be better to just rip the gcov stuff out since I don't know that anyone will ever have time to fix it and since gcc doesn't seem to hold the format stable it's a lot of ongoing work to keep it working even if someone did fix it. FWIW, in dbus-glib I ripped out the gcov support (which hadn't worked since dbus-glib was separated from libdbus, as far as I could see) and replaced it with the lcov integration we use in Telepathy. It might be worth giving libdbus the same treatment. From the department of re-using other people's solutions, here's a branch to: * throw away our unmaintained wrapper for gcov * use lcov (someone else's maintained wrapper for gcov) to make HTML reports I've found this to be very useful for feature development, to check which branches are covered by our tests. Created attachment 43429 [details] [review] Don't ignore all files in m4, just the libtool ones This patch seems safe for 1.4 too. Created attachment 43430 [details] [review] Import compiler.m4 and lcov.am from telepathy-glib, and use them to replace gcov We might want to avoid this one for 1.4 and apply it on master for 1.5, or other maintainers might consider it OK for 1.4; I'd be happy either way. It's most useful for feature development, which should be done on master rather than 1.4. Forgot to mention, there's a nasty workaround in the second patch:
> +# gcov takes ~forever to process config-parser.c for some reason?
> +lcov-report:
> + true > bus/bus_test-config-parser.gcno
> + true > bus/dbus_daemon-config-parser.gcno
...
However, it's still better than our gcov "support", which can't give us coverage stats for *anything* at the moment, afaics.
config-parser.c is nearly 100K, so it might just be too big for gcov to cope with. I'd be inclined to blame append_rule_from_element, which has a composite "if" conditional nearly 100 lines long...
fwiw I had some decent results with trucov for a code doodle I was working on lately: https://github.com/havocp/hwf (trucov is in deps/ and Makefile-test.am has a rule using it) lcov is surely an improvement and I'd support putting it in, just thought I'd mention trucov too Review of attachment 43429 [details] [review]: Looks OK. Review of attachment 43430 [details] [review]: This is a big patch, but since it's just a developer tool (not critical path) and it comes from more used/tested code I'd say just put it in =) Comment on attachment 43430 [details] [review] Import compiler.m4 and lcov.am from telepathy-glib, and use them to replace gcov Patches applied for 1.4.4/1.5.0, but I'm leaving this bug open because we skip config-parser.c. Created attachment 44435 [details] [review] Break up the monster conditional in config-parser so gcov can cope Review of attachment 44435 [details] [review]: Looks good to me! Fixed in git for 1.4.8, and I'll merge to master shortly. |
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.