On Mon, 2022-01-03 at 08:32 +0100, Fabrice Fontaine wrote:
>
> Support glibmm 2.68 which has been released one year ago and is the
> first stable release in the glibmm-2.68 ABI series:
> https://gitlab.gnome.org/GNOME/glibmm/-/blob/2.68.2/NEWS
>
> As TimeVal is not available with glibmm 2.68, use DateTime which is
> available since version 2.26
As mentioned earlier I'm not familiar with C++, and my autotools
foo is rather weak. So take that response with a grain of salt.
> --- a/README
> +++ b/README
> @@ -63,7 +63,7 @@ Requirements for the C++ bindings:
> - doxygen (required for building the bindings, not only for C++ API docs!)
> - graphviz (optional, only needed for the C++ API docs)
> - Python (2 or 3) executable (development files are not needed)
> - - glibmm-2.4 (>= 2.32.0)
> + - glibmm-2.4 (>= 2.32.0) or glibmm-2.68 (>= 2.68.0)
Mention preferred versions before "also acceptable" fallbacks
perhaps? Checks probably also should reflect preference.
> header->feed_version = 1;
> - header->starttime.tv_sec = start_time.tv_sec;
> - header->starttime.tv_usec = start_time.tv_usec;
> + header->starttime.tv_sec = start_time.to_unix();
> + header->starttime.tv_usec = start_time.get_microsecond();
> auto packet = g_new(struct sr_datafeed_packet, 1);
Is this source code change compatible with either version? The
one that you target anew, as well as the versions that current
mainline already supports? A library that has only been around
for one year is rather new an external dependency, given how many
platforms the sigrok project supports and that execution on older
systems is a stated goal. Consider users' and co-developers'
needs as well as distributors' processes.
If the data type change and corresponding source update works for
glibmm-2.4 already, then I suggest to separate it from the 2.68
upgrade. Simplifies review and test, and thus mainline integration.
> --- a/configure.ac
> +++ b/configure.ac
> @@ -407,9 +407,14 @@ AS_IF([test "x$HAVE_CXX11" != x1],
> [SR_APPEND([sr_cxx_missing], [', '], ['C++11'])])
>
> # The C++ bindings need glibmm.
> -SR_PKG_CHECK([glibmm], [SR_PKGLIBS_CXX], [glibmm-2.4 >= 2.32.0])
> +SR_GLIBMM_REQUIRES=glibmm-2.4
> +SR_PKG_CHECK([glibmm], [SR_PKGLIBS_CXX], [$SR_GLIBMM_REQUIRES >= 2.32.0])
> +AS_IF([test "x$sr_have_glibmm" != xyes],
> + [SR_GLIBMM_REQUIRES=glibmm-2.68
> + SR_PKG_CHECK([glibmm], [SR_PKGLIBS_CXX], [$SR_GLIBMM_REQUIRES >=
> 2.68.0])])
> AS_IF([test "x$sr_have_glibmm" != xyes],
> [SR_APPEND([sr_cxx_missing], [', '], [glibmm])])
> +AC_SUBST(SR_GLIBMM_REQUIRES)
>
> # The C++ bindings use Doxygen to parse libsigrok symbols.
> AC_CHECK_PROG([HAVE_DOXYGEN], [doxygen], [yes], [no])
This logic confuses me, can you come up with phrases that better
reflect what's happening? Ideally search in order of preference,
search once then use whatever was found. The "conditional" check
for something newer after something acceptable yet older(?) was
found is counter intuitive. The fact that something is assigned
before the check actually happens is surprising. How robust is
that phrase, have you checked it in different configurations,
with new and with old libs and with none at all available? Some
machines could provide both versions while you seem to prefer the
newer implementation.
virtually yours
Gerhard Sittig
--
If you don't understand or are scared by any of the above
ask your parents or an adult to help you.
_______________________________________________
sigrok-devel mailing list
sigrok-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sigrok-devel
