On Mon, May 24, 2021 at 12:14:31PM -0400, Mark Michelson wrote: > On 5/21/21 11:02 PM, Ben Pfaff wrote: > > This tool is also needed and also varies from one version of DDlog to > > another, so we should find it and check its version in the same way. > > > > Signed-off-by: Ben Pfaff <b...@ovn.org> > > --- > > acinclude.m4 | 24 ++++++++++++++++-------- > > northd/automake.mk | 4 ++-- > > 2 files changed, 18 insertions(+), 10 deletions(-) > > > > diff --git a/acinclude.m4 b/acinclude.m4 > > index 7009f1dd39c2..20c5ed02309e 100644 > > --- a/acinclude.m4 > > +++ b/acinclude.m4 > > @@ -85,14 +85,22 @@ AC_DEFUN([OVS_CHECK_DDLOG], [ > > AC_MSG_ERROR([ddlog is required to build with DDlog]) > > fi > > - AC_MSG_CHECKING([$DDLOG version]) > > - $DDLOG --version >&AS_MESSAGE_LOG_FD 2>&1 > > - ddlog_version=$($DDLOG --version | sed -n 's/^DDlog v\([[^ > > ]]*\).*/\1/p') > > - AC_MSG_RESULT([$ddlog_version]) > > - m4_if([$1], [], [], [ > > - AS_CASE([$ddlog_version], > > - [$1 | $1.*], [], > > - [*], [AC_MSG_ERROR([DDlog version $1.x is required, but > > $ddlog_version is installed])])]) > > + AC_ARG_VAR([OVSDB2DDLOG], [path to ovsdb2ddlog binary]) > > + AC_PATH_PROGS([OVSDB2DDLOG], [ovsdb2ddlog], [none], [$DDLOG_PATH]) > > + if test X"$OVSDB2DDLOG" = X"none"; then > > + AC_MSG_ERROR([ovsdb2ddlog is required to build with DDlog]) > > + fi > > + > > + for tool in "$DDLOG" "$OVSDB2DDLOG"; do > > + AC_MSG_CHECKING([$tool version]) > > + $tool --version >&AS_MESSAGE_LOG_FD 2>&1 > > + tool_version=$($tool --version | sed -n 's/^.* v\([[^ ]]*\).*/\1/p') > > (I should make this comment on patch 1, but I'm making it here so I can just > send one email response for this patch series) > > There is a small danger of this regex matching on something unexpected in > case a DDLog tool ever has a word starting with a lowercase v in it. For > example, a hypothetical "DDLog virtualization suite" would give us the > version string "irtualization". It may be worthwhile to at least ensure the > character directly after "v" is a number. > > Maybe I'm just being overly nitpicky here though. > > > + AC_MSG_RESULT([$tool_version]) > > + m4_if([$1], [], [], [ > > + AS_CASE([$tool_version], > > + [$1 | $1.*], [], > > + [*], [AC_MSG_ERROR([DDlog version $1.x is required, but > > $tool is version $ddlog_version])])]) > > This error message should print $tool_version instead of $ddlog_version. > $ddlog_version is no longer set, so this just always prints an empty string > for the version if you have the wrong one installed.
Thanks for the review. I fixed both of these and posted v3. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
