Re: [PATCH] Add parameter to allow enabling/disabling selftests via configure

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Friday, October 05 2018, Simon Marchi wrote:

> On 2018-09-17 4:22 p.m., Sergio Durigan Junior wrote:
>> This is a follow-up of:
>> 
>>   https://sourceware.org/ml/gdb-patches/2018-08/msg00347.html
>> 
>> Instead of going throttle and always enabling our selftests (even in
>> non-development builds), this patch is a bit more conservative and
>> introduces a configure option ("--enable-unit-tests") that allows the
>> user to choose whether she wants unit tests in the build or not.  Note
>> that the current behaviour is retained: if no option is provided, GDB
>> will have selftests included in a development build, and will *not*
>> have selftests included in a non-development build.
>> 
>> The rationale for having this option is still the same: due to the
>> many racy testcases and random failures we see when running the GDB
>> testsuite, it is unfortunately not possible to perform a full test
>> when one is building a downstream package.  As the Fedora GDB
>> maintainer and one of the Debian GDB uploaders, I feel like this
>> situation could be improved by, at least, executing our selftests
>> after the package has been built.
>> 
>> This patch introduces no regressions to our build.
>
> Hi Sergio,

Hi Simon,

Thanks for the review.

> I think having an configure switch with that default value makes sense.
>
> I have two suggestions:
>
> - Mention the switch explicitly in the error message (--{enable,disable}-unit-tests)
>   so the user knows exactly what you are talking about.
> - Share the code between gdb's and gdbserver's configure script
>
>> +`--enable-unit-tests[=yes|no]'
>> +     Enable (i.e., include) support for unit tests when compiling GDB
>> +     and GDBServer.  Note that if this option is not passed, GDB will
>> +     have selftests if it is a development build, and will *not* have
>> +     selftests if it a non-development build.
>
> Missing a word: "if it is a".
>
> Here's a patch that goes on top of yours that does what I suggest (I had to try it
> before suggesting it to make sure I don't ask for the impossible, so I might as well
> share it).  With this (especially the code-sharing part), the patch LGTM (the
> ChangeLog entry would need to be updated to account for the new changes).

Fair enough.  I applied your patch, updated the ChangeLog, and pushed
it.

8ecfd7bd4acd69213c06fac6de9af38299123547

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/