[Spice-devel] [PATCH spice-streaming-agent] Only build unit tests when running make check

[Christophe Fergeau]

On Wed, Feb 28, 2018 at 01:33:35PM +0100, Lukáš Hrázký wrote:
> On Wed, 2018-02-28 at 12:19 +0100, Christophe Fergeau wrote:
> > On Wed, Feb 28, 2018 at 11:56:07AM +0100, Lukáš Hrázký wrote:
> > > On Tue, 2018-02-27 at 18:02 +0100, Christophe Fergeau wrote:
> > > > On Tue, Feb 27, 2018 at 01:04:31PM +0100, Lukáš Hrázký wrote:
> > > > > On Tue, 2018-02-27 at 06:33 -0500, Frediano Ziglio wrote:
> > > > > > > 
> > > > > > > Removing noinst_PROGRAMS from src/unittests/Makefile.am will cause the
> > > > > > > unit tests not be built on a regular build (`make`), they will only be
> > > > > > > built when running the tests (`make check`).
> > > > > > > 
> > > > > > > C++ unit test frameworks are notorious for long build times too.
> > > > > > > 
> > > > > > > Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> > > > > > > ---
> > > > > > > I am not sure if there was another reason for noinst_PROGRAMS, but I
> > > > > > > don't think so.
> > > > > > > 
> > > > > > 
> > > > > > Yes, this is mainly what it does. In spice-server we have same pattern,
> > > > > > the reason to have it is the opposite of this patch, we want to make
> > > > > > sure tests compile even if they are not executed.
> > > > > > 
> > > > > > If the final aim of this patch is to avoid the requirement of catch
> > > > > > library this patch is just partial.
> > > > > > 
> > > > > > If the aim is to just save some time avoiding to compile tests
> > > > > > the patch is fine but looks like presenting different behaviour
> > > > > > comparing to other SPICE projects.
> > > > > 
> > > > > Not to avoid the Catch dependency, to save time and just not do what
> > > > > the user doesn't want to do. If he wants to build the project, he
> > > > > doesn't need the tests. You only really need to build the tests if you
> > > > > want to run them.
> > > > > 
> > > > > For SPICE projects in general, I think the logic is backwards. We do
> > > > > not want to build the tests on a regular build to make sure they do
> > > > > build. We want to (build and) run the unit tests all the time to make
> > > > > sure not only they build, but also that they pass :)
> > > > 
> > > > This logic is following what the autotools give us, a separate make
> > > > check target.
> > > 
> > > I am not following :) I think you could make the default (build) target
> > > depend on the check target (practically making them one target, as
> > > opposed to a separate check target, which you mentioned), if that is
> > > what you mean. But that makes little sense, as often you want to only
> > > build and not run tests.
> > 
> > All I meant is that when you use the autotools, what you get by default
> > is a make (all) target, and a make check target, which are separate. So
> > I was saying that your suggestion (always running make check as part of
> > make (all)) is not what the autotools offer by default, which is why
> > it's not done this way at the moment. I was not saying it's not possible
> > to do it ;)
> 
> Oh, but I was not suggesting to run make check as part of make (all),
> only that we should run it explicitely. And besides, `make check`
> builds the project if it's not built, so it is one command, just a
> different one... To be clear, I'm suggesting we should `make check` as
> part of our workflow to post patches (and eventually have a CI for it
> too), not add things to `make all` to make them inevitable to run.

Ah ok, misunderstood that part, yes, I think I agree on that, we can at
least try it, only build tests as part of make check, and complain
loudly when someone breaks it.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180301/d86f0a2b/attachment.sig>