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

[Frediano Ziglio]

> 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.
> 

Yes, you are right, I though they were nested as in previous patch.

> 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.
> 

For me it's fine here.

> > 
> > 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.
> 

No, but does have only preprocessor macro definition, so is
not a problem if included multiple times.
Is a problem if part of the source has a some definition and
part no and this affects ABI.

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

No, expected, multiple equal preprocessor definition are
not a problem.

> > 
> >>> +#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>
> > 

Still ack :-)

Frediano