Bug 66069 - [PATCH] Build failed if tests enabled but asserts disabled
Summary: [PATCH] Build failed if tests enabled but asserts disabled
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: Other All
: low trivial
Assignee: Havoc Pennington
QA Contact:
URL:
Whiteboard: review?
Keywords: patch
Depends on:
Blocks:
 
Reported: 2013-06-23 07:40 UTC by Chengwei Yang
Modified: 2013-09-13 13:12 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
[PATCH] Fix build failure if tests enabled but asserts disabled (1.89 KB, patch)
2013-06-23 07:41 UTC, Chengwei Yang
Details | Splinter Review
[PATCH] Ignore more unused staff if build with tests but without asserts (1.04 KB, patch)
2013-06-27 05:02 UTC, Chengwei Yang
Details | Splinter Review

Description Chengwei Yang 2013-06-23 07:40:50 UTC
Build will fail like below

$ ./autogen.sh --enable-tests --disable-asserts

...
  CC     libdbus_1_la-dbus-object-tree.lo
dbus-object-tree.c: In function 'object_tree_test_iteration':
dbus-object-tree.c:1697:9: error: variable 'nb' set but not used [-Werror=unused-but-set-variable]
dbus-object-tree.c:1525:15: error: unused variable 'exact_match' [-Werror=unused-variable]
cc1: all warnings being treated as errors
Comment 1 Chengwei Yang 2013-06-23 07:41:41 UTC
Created attachment 81256 [details] [review]
[PATCH] Fix build failure if tests enabled but asserts disabled
Comment 2 Simon McVittie 2013-06-24 11:47:41 UTC
(In reply to comment #0)
> $ ./autogen.sh --enable-tests --disable-asserts

That configuration doesn't make sense. The "embedded" tests work entirely in terms of

    _dbus_assert (test has not failed);

so turning off assertions makes these tests useless.
Comment 3 Chengwei Yang 2013-06-24 13:01:05 UTC
(In reply to comment #2)
> (In reply to comment #0)
> > $ ./autogen.sh --enable-tests --disable-asserts
> 
> That configuration doesn't make sense. The "embedded" tests work entirely in
> terms of
> 
>     _dbus_assert (test has not failed);
> 
> so turning off assertions makes these tests useless.

Yes, I think one do that should be warned by the NOTE from configure.

"
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)
"

This trivial patch just make dbus build more robust I think. From my POV, since we provide these valid configure options, it's not bad we just make sure they can build.
Comment 4 Simon McVittie 2013-06-26 11:47:34 UTC
(In reply to comment #3)
> This trivial patch just make dbus build more robust I think. From my POV,
> since we provide these valid configure options, it's not bad we just make
> sure they can build.

If you want this configuration to work, please turn off some of the -Wunused-* family if it's detected (we already turn off -Wunused-label sometimes). I'm not interested in working around "unused stuff" warnings in the tests.
Comment 5 Chengwei Yang 2013-06-26 12:21:20 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > This trivial patch just make dbus build more robust I think. From my POV,
> > since we provide these valid configure options, it's not bad we just make
> > sure they can build.
> 
> If you want this configuration to work, please turn off some of the
> -Wunused-* family if it's detected (we already turn off -Wunused-label
> sometimes). I'm not interested in working around "unused stuff" warnings in
> the tests.

How about another solution,sounds like that: --disable-asserts will disable tests?
Comment 6 Chengwei Yang 2013-06-27 05:02:38 UTC
Created attachment 81522 [details] [review]
[PATCH] Ignore more unused staff if build with tests but without  asserts

As you suggested, this is a better solution.
Comment 7 Simon McVittie 2013-09-13 13:11:09 UTC
Comment on attachment 81522 [details] [review]
[PATCH] Ignore more unused staff if build with tests but without  asserts

Review of attachment 81522 [details] [review]:
-----------------------------------------------------------------

"stuff" not "staff", but yes.
Comment 8 Simon McVittie 2013-09-13 13:12:09 UTC
Fixed in git for 1.7.6


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.