> 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@xxxxxxxxxx>
> >>> ---
> >>>
> >>> 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@xxxxxxxxxx>
> >
Still ack :-)
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel