Bug 66069

Summary:	[PATCH] Build failed if tests enabled but asserts disabled
Product:	dbus	Reporter:	Chengwei Yang <chengwei.yang.cn>
Component:	core	Assignee:	Havoc Pennington <hp>
Status:	RESOLVED FIXED	QA Contact:
Severity:	trivial
Priority:	low	CC:	chengwei.yang.cn
Version:	1.5	Keywords:	patch
Hardware:	Other
OS:	All
Whiteboard:	review?
i915 platform:		i915 features:
Attachments:	[PATCH] Fix build failure if tests enabled but asserts disabled [PATCH] Ignore more unused staff if build with tests but without asserts

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.