[Spice-devel] [spice-streaming-agent PATCH v4] catch: check for any of catch and catch2

[Uri Lublin]

On 2/7/19 4:55 PM, Frediano Ziglio wrote:
>>
>> Just fixing the subject (spice-streaming-agent patch v4)
>> Uri
>>
>> On 2/7/19 2:02 PM, Uri Lublin wrote:
>>> Catch2 is now in upstream (github) and Fedora (since Fedora 27)
>>>
>>> Signed-off-by: Uri Lublin <uril at redhat.com>
>>> ---
>>>
>>> Since v3 (local only):
>>>     - Use AC_CHECK_HEADERS instead of AC_CHECK_HEADER
>>> Since v2:
>>>     - Do not assume the .h files are under /usr/include
>>> Since v1:
>>>     - Check both Catch2 and Catch
>>>
>>> ---
>>>    configure.ac                          |  6 ++++--
>>>    src/unittests/Makefile.am             |  2 ++
>>>    src/unittests/spice-catch.hpp         | 18 ++++++++++++++++++
>>>    src/unittests/test-mjpeg-fallback.cpp |  2 +-
>>>    src/unittests/test-stream-port.cpp    |  2 +-
>>>    5 files changed, 26 insertions(+), 4 deletions(-)
>>>    create mode 100644 src/unittests/spice-catch.hpp
>>>
>>> diff --git a/configure.ac b/configure.ac
>>> index c259f7e..1c7788b 100644
>>> --- a/configure.ac
>>> +++ b/configure.ac
>>> @@ -119,9 +119,11 @@ case "$enable_tests" in
>>>      *) AC_MSG_ERROR([bad value ${enable_tests} for enable-tests option]) ;;
>>>    esac
>>>    AS_IF([test "x$enable_tests" != "xno"],
>>> -      [AC_CHECK_HEADER([catch/catch.hpp],have_check="yes",)])
>>> +      [AC_CHECK_HEADERS([catch/catch.hpp],  have_check="yes")
>>> +       AC_CHECK_HEADERS([catch2/catch.hpp], have_check="yes")])
> 
> I would check "catch2" before "catch" as newer versions use "catch2".

I think it does not matter, in any case it checks both.
Here the indentation is correct, so it's two independent calls done
one after the other. I expect only one (or zero) to be installed.

see more below (at <HERE>).

> 
>>> +
>>>    AS_IF([test "x$enable_tests" = "xyes" && test "x$have_check" != "xyes"],
>>> -      [AC_MSG_ERROR([Could not find Catch dependency header
>>> (catch/catch.hpp)])])
>>> +      [AC_MSG_ERROR([Could not find Catch dependency header
>>> (catch.hpp)])])
>>>    AM_CONDITIONAL([ENABLE_TESTS], [test "x$have_check" = "xyes"])
>>>    
>>>    AC_DEFINE_DIR([BINDIR], [bindir], [Where binaries are installed.])
>>> diff --git a/src/unittests/Makefile.am b/src/unittests/Makefile.am
>>> index 8ce1f7a..556b885 100644
>>> --- a/src/unittests/Makefile.am
>>> +++ b/src/unittests/Makefile.am
>>> @@ -49,6 +49,7 @@ test_mjpeg_fallback_SOURCES = \
>>>    	../mjpeg-fallback.cpp \
>>>    	../utils.cpp \
>>>    	../x11-display-info.cpp \
>>> +	spice-catch.hpp \
>>>    	$(NULL)
>>>    
>>>    test_mjpeg_fallback_LDADD = \
>>> @@ -61,6 +62,7 @@ test_mjpeg_fallback_LDADD = \
>>>    test_stream_port_SOURCES = \
>>>    	test-stream-port.cpp \
>>>    	../stream-port.cpp \
>>> +	spice-catch.hpp \
>>>    	$(NULL)
>>>    
>>>    EXTRA_DIST = \
>>> diff --git a/src/unittests/spice-catch.hpp b/src/unittests/spice-catch.hpp
>>> new file mode 100644
>>> index 0000000..69aac90
>>> --- /dev/null
>>> +++ b/src/unittests/spice-catch.hpp
>>> @@ -0,0 +1,18 @@
>>> +/*
>>> + * Include catch/catch.hpp or catch2/catch.hpp
>>> + * according to what configure found
>>> + *
>>> + * \copyright
>>> + * Copyright 2019 Red Hat Inc. All rights reserved.
>>> + */
>>> +
>>> +#ifndef SPICE_CATCH_HPP
>>> +#include <config.h>
>>> +
> 
> Could be seen weird to include config.h in an header but
> as is limited to test director I don't find it an issue.

I can move it to the two source files.

> 
> Could be an option to have
> 
> #if !defined(HAVE_CATCH2_CATCH_HPP) && !defined(HAVE_CATCH_CATCH_HPP)
> #include <config.h>
> #endif
> 
> so you can avoid to include from module and header.

I expected config.h to have an #ifndef guard, but it does not.

I wrote a quick test (a.c file) that has
two #include <config.h> lines and gcc did not
warn/error about it.

> 
>>> +#if   defined(HAVE_CATCH2_CATCH_HPP)

<HERE> CATCH2 is checked first, so even if both are defined
catch2 will be used (catch is in #elif block).

>>> +#include <catch2/catch.hpp>
>>> +#elif defined(HAVE_CATCH_CATCH_HPP)
>>> +#include <catch/catch.hpp>
>>> +#endif
>>> +
>>> +#endif // SPICE_CATCH_HPP


Thanks,
     Uri.

>>> diff --git a/src/unittests/test-mjpeg-fallback.cpp
>>> b/src/unittests/test-mjpeg-fallback.cpp
>>> index e39dc49..49eef98 100644
>>> --- a/src/unittests/test-mjpeg-fallback.cpp
>>> +++ b/src/unittests/test-mjpeg-fallback.cpp
>>> @@ -1,5 +1,5 @@
>>>    #define CATCH_CONFIG_MAIN
>>> -#include "catch/catch.hpp"
>>> +#include "spice-catch.hpp"
>>>    
>>>    #include "mjpeg-fallback.hpp"
>>>    
>>> diff --git a/src/unittests/test-stream-port.cpp
>>> b/src/unittests/test-stream-port.cpp
>>> index e7b7b89..8c77979 100644
>>> --- a/src/unittests/test-stream-port.cpp
>>> +++ b/src/unittests/test-stream-port.cpp
>>> @@ -5,7 +5,7 @@
>>>     */
>>>    
>>>    #define CATCH_CONFIG_MAIN
>>> -#include <catch/catch.hpp>
>>> +#include "spice-catch.hpp"
>>>    #include <sys/socket.h>
>>>    #include <signal.h>
>>>    
>>>
> 
> Otherwise,
> 
> Acked-by: Frediano Ziglio <fziglio at redhat.com>
> 
> Frediano
>