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

[Lukáš Hrázký]

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.

> > 
> > > With this patch, the situation becomes odd I think,
> > > configure.ac requires an unusual (imo) dependency ('catch') which is
> > > then not going to be used in our default build (a straight 'make').
> > 
> > How is it more odd than having an unusual dependency which is only used
> > to build the tests which are then not run? :) I find it more odd to
> > build the tests for no reason (other than to make sure they build, but
> > the 'make sure' part is only our responsibility and makes no sense for
> > users that just want to use it).
> 
> Hmm, the last part of your sentence could apply to unit tests too ;)

I thought you might notice that :D The thing is, depending on what the
user wants, he can:

1) Run `make` to build just the binaries

2) Run `make check` to build and run the tests

He may or may not be interested in running the tests. One scenario,
where he should always run the tests, is IMHO when he is building
binaries that are going to be installed in a system.

> > 
> > By the way, I only sent this change, because it makes sense to me in
> > itself, but I am somewhat expecting someone (who has more configure.ac-
> > fu than me) to post the part that makes Catch an optional dependency...
> 
> Ah, I did not realize you needed a hand with that, which is why I did
> not propose any patch.

I would manage, but I have a dislike for autoconf and somehow didn't
feel it's up to me, since the discussion was post-merge. I can still do
it as an excersize :) But if you can whip it up quickly, I wouldn't
mind :)

Thanks,
Lukas

> Christophe
> 
> > 
> > Cheers,
> > Lukas
> > 
> > > Christophe