> On 20 Feb 2018, at 10:00, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote: > > On Mon, 2018-02-19 at 21:19 +0200, Uri Lublin wrote: >> On 02/19/2018 06:47 PM, Lukáš Hrázký wrote: >>> On Mon, 2018-02-19 at 18:29 +0200, Uri Lublin wrote: >>>> On 02/14/2018 06:37 PM, Christophe Fergeau wrote: >>>>> On Wed, Feb 14, 2018 at 10:40:58AM -0500, Frediano Ziglio wrote: >>>>>>> >>>>>>>> On 14 Feb 2018, at 13:34, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote: >>>>>>>> >>>>>>>> Introduce a unit test framework (Catch) to the codebase and a simple >>>>>>>> unit test for parsing the options of the mjpeg plugin. >>>>>>>> >>>>>>>> Signed-off-by: Lukáš Hrázký <lhrazky@xxxxxxxxxx> >>>>>>>> --- >>>>>>>> configure.ac | 3 ++ >>>>>>>> src/mjpeg-fallback.cpp | 5 +++ >>>>>>>> src/mjpeg-fallback.hpp | 1 + >>>>>>>> src/unittests/.gitignore | 5 +-- >>>>>>>> src/unittests/Makefile.am | 15 +++++++++ >>>>>>>> src/unittests/test-mjpeg-fallback.cpp | 58 >>>>>>>> +++++++++++++++++++++++++++++++++++ >>>>>>>> 6 files changed, 85 insertions(+), 2 deletions(-) >>>>>>>> create mode 100644 src/unittests/test-mjpeg-fallback.cpp >>>>>>>> >>>>>>>> diff --git a/configure.ac b/configure.ac >>>>>>>> index 8795dae..5aab662 100644 >>>>>>>> --- a/configure.ac >>>>>>>> +++ b/configure.ac >>>>>>>> @@ -17,6 +17,7 @@ if test x"$ac_cv_prog_cc_c99" = xno; then >>>>>>>> AC_MSG_ERROR([C99 compiler is required.]) >>>>>>>> fi >>>>>>>> AC_PROG_CXX >>>>>>>> +AC_LANG(C++) >>>>>>>> AX_CXX_COMPILE_STDCXX_11 >>>>>>>> AC_PROG_INSTALL >>>>>>>> AC_CANONICAL_HOST >>>>>>>> @@ -49,6 +50,8 @@ AC_CHECK_LIB(jpeg, jpeg_destroy_decompress, >>>>>>>> AC_MSG_ERROR([libjpeg not found])) >>>>>>>> AC_SUBST(JPEG_LIBS) >>>>>>>> >>>>>>>> +AC_CHECK_HEADER([catch/catch.hpp],,[AC_MSG_ERROR([Could not find Catch >>>>>>>> dependency header (catch/catch.hpp)])]) >>>>>>> >>>>>>> Instead of an error, shouldn’t we instead fallback to not compiling the unit >>>>>>> tests? Possibly a warning? >>>>>>> >>>>>> >>>>>> Good point but I would suggest a follow up and an explicit --I-dont-really-want-unittests >>>>>> option, I agree people should run tests. >>>>>> Another follow up is a patch for the spec file. >>>>> >>>>> Alternatively, this could go with a --with-catch flag (or >>>>> --enable-unittest or any names which fits you), and >>>>> 1) if there is nothing specified, then we silently enable/disable tests >>>>> depending on the availability of the catch >>>>> 2) if --with-catch is specified, then we error out if it's not found >>>>> 3) if --without-catch is used, then we never use it. >>>>> >>>>> Then we add --with-catch to autogen.sh, and all developers will have to >>>>> go through extra hoops not to use it. >>>>> >>>>> Christophe >>>>> >>>> >>>> I agree that's a good alternative. >>> >>> I prefer Frediano's variant. Nobody is forced to use autogen.sh and >>> also I don't think anybody would expect autogen.sh to modify the >>> default configure behaviour. I don't think it's a good idea. >> >> If users prefer to not run autogen.sh that's ok. >> It provides defaults options for developers. >> For example, I do not expect users to run configure with >> --enable-maintainer-mode too. >> Most users will use configure directly from the tarball. >> >> Users can choose what options they want to enable/disable >> >>> >>> For example, on Gentoo, which doesn't care for autogen.sh and runs >>> autotools build automatically, the default behaviour (unless the person >>> writing the ebuild would notice) would be dependent on Catch being >>> present in the system. This can create subtle inconsistencies, which >>> aren't critical in this case, but still unnecessary. >> >> So if on Gentoo (or another distribution) there exists no catch >> package, they are forced to either add this package or search >> for how to disable it. > > Yes. The packager is forced to resolve the issue by either providing > the dependency or explicitly disabling the tests. In contrast to the > tests being "quietly" skipped, if Catch is not present, unless he would > notice it. I’d be more comfortable with a warning at the end of configure stating that we did not find catch, so unit tests can’t be run, like we do for example for a few other optional missing components such as codecs. A warning is also more friendly than a hard failure. To summarize, like Christophe, I would like to see a —with-catch option, but I would like to see it default to yes, unless we cannot find catch, in which case: - If the option —with-catch was specified, configure fails - If the option was set to default, we get a warning, we can build, we can’t test. (Maybe this is really what Christophe had in mind too, not sure). > If he isn't dilligent and doesn't notice, the behaviour of > the package will depend on the presence of Catch on the target system ( > on Gentoo packages are compiled on users' systems) and since the > packager didn't test one of the variants, it could potentially fail on > the user. I would like to understand what you mean with “the behaviour of the package”. Only the ability to run tests, right? > > Packagers at least on Gentoo are used to enabling/disabling things in > configure. I don't think it's a hurdle for them to disable the tests if > indeed they don't have the package (and I think most distros have it - > Gentoo does). > >> This package is only required if the user wants >> to run the tests. >> Those tests do not run by default either. >> One have to "make check" for the tests to run. > > It is desirable to run the tests during packaging. I saw Frediano post > a patch the other day for I think the Fedora package to run `make > check` during the packaging? IIRC `make check` is run by default by > Debian packaging. I am not sure, but would think on Gentoo `make check` > is also run by default. > > I would just like to turn the tests on for users by default. (assuming > they are working well) We could make something like "You can use > './configure --disable-unittests' to skip the unit tests." part of the > error message if Catch is not found to make it easier for them? > > Lukas _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel