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@xxxxxxxxxx> > > > > > > > --- > > > > > > > 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel