[ovs-dev,v5] JSON serialization via Python's json lib

There is no particularly good reason to use our own Python JSON
serialization implementation when serialization can be done faster
with Python's built-in JSON library.

A few tests were changed due to Python's default JSON library
returning slightly more precise floating point numbers.

Signed-off-by: Terry Wilson <twilson@redhat.com>
---
 configure.ac       |   2 +
 m4/openvswitch.m4  |  47 ++++++++++++++++++++++++
 python/automake.mk |  27 +++++++++++++-
 python/ovs/json.py | 106 +++++------------------------------------------------
 tests/json.at      |  26 ++++++++++---
 5 files changed, 104 insertions(+), 104 deletions(-)

Sigh. And of course I had libopenvswitch installed on the system as
well and removing it breaks building the extensions with the above
patch. This is the kind of thing that would be much easier if the
Python lib was its own project with its own test suite and it could
just always assume libopenvswitch was installed on the system as a
build dependency.

There is much to say about why all of this is a pain, but the main
takeaway is that I'm going to split out the serialization part of this
patch from the build system stuff because there is no reason for one
part to hold up the other.

Terry

On Fri, Jul 22, 2016 at 9:57 PM, Terry Wilson <twilson@redhat.com> wrote:
> There is no particularly good reason to use our own Python JSON
> serialization implementation when serialization can be done faster
> with Python's built-in JSON library.
>
> A few tests were changed due to Python's default JSON library
> returning slightly more precise floating point numbers.
>
> Signed-off-by: Terry Wilson <twilson@redhat.com>
> ---
>  configure.ac       |   2 +
>  m4/openvswitch.m4  |  47 ++++++++++++++++++++++++
>  python/automake.mk |  27 +++++++++++++-
>  python/ovs/json.py | 106 +++++------------------------------------------------
>  tests/json.at      |  26 ++++++++++---
>  5 files changed, 104 insertions(+), 104 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 05d80d5..5472a52 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -96,6 +96,8 @@ OVS_CHECK_LIBCAPNG
>  OVS_CHECK_LOGDIR
>  OVS_CHECK_PYTHON
>  OVS_CHECK_PYTHON3
> +OVS_CHECK_PYTHON_HEADERS
> +OVS_CHECK_PYTHON3_HEADERS
>  OVS_CHECK_FLAKE8
>  OVS_CHECK_DOT
>  OVS_CHECK_IF_PACKET
> diff --git a/m4/openvswitch.m4 b/m4/openvswitch.m4
> index a448223..60ae114 100644
> --- a/m4/openvswitch.m4
> +++ b/m4/openvswitch.m4
> @@ -589,3 +589,50 @@ AC_DEFUN([OVS_CHECK_PRAGMA_MESSAGE],
>       [AC_DEFINE(HAVE_PRAGMA_MESSAGE,1,[Define if compiler supports #pragma
>       message directive])])
>    ])
> +
> +dnl OVS_CHECK_PYTHON_HEADERS
> +AC_DEFUN([OVS_CHECK_PYTHON_HEADERS],
> +  [AC_REQUIRE([OVS_CHECK_PYTHON])
> +    AC_PATH_PROG([PYTHON_CONFIG], python-config, no)
> +    if test "$PYTHON_CONFIG" != no; then
> +      PYTHON_INCLUDES=`$PYTHON_CONFIG --includes`
> +      PYTHON_LIBS=`$PYTHON_CONFIG --libs`
> +      PYTHON_LDFLAGS=`$PYTHON_CONFIG --ldflags`
> +      save_LIBS="$LIBS"
> +      save_CPPFLAGS="$CPPFLAGS"
> +      save_LDFLAGS="$LDFLAGS"
> +      LIBS="$PYTHON_LIBS"
> +      LDFLAGS="$PYTHON_LDFLAGS"
> +      CPPFLAGS="$PYTHON_INCLUDES"
> +      AC_LINK_IFELSE(
> +        [AC_LANG_PROGRAM([#include <Python.h>],[])],
> +                         [have_py_headers=true])
> +      LIBS="$save_LIBS"
> +      CPPFLAGS="$save_CPPFLAGS"
> +      LDFLAGS="$save_LDFLAGS"
> +    fi
> +   AM_CONDITIONAL([HAVE_PYTHON_HEADERS], [test "$have_py_headers" = "true"])])
> +  ])
> +
> +AC_DEFUN([OVS_CHECK_PYTHON3_HEADERS],
> +  [AC_REQUIRE([OVS_CHECK_PYTHON3])
> +    AC_PATH_PROG([PYTHON3_CONFIG], python3-config, no)
> +    if test "$PYTHON3_CONFIG" != no; then
> +      PYTHON3_INCLUDES=`$PYTHON3_CONFIG --includes`
> +      PYTHON3_LIBS=`$PYTHON3_CONFIG --libs`
> +      PYTHON3_LDFLAGS=`$PYTHON3_CONFIG --ldflags`
> +      save_LIBS="$LIBS"
> +      save_CPPFLAGS="$CPPFLAGS"
> +      save_LDFLAGS="$LDFLAGS"
> +      LIBS="$PYTHON3_LIBS"
> +      LDFLAGS="$PYTHON3_LDFLAGS"
> +      CPPFLAGS="$PYTHON3_INCLUDES"
> +      AC_LINK_IFELSE(
> +        [AC_LANG_PROGRAM([#include <Python.h>],[])],
> +                         [have_py3_headers=true])
> +      LIBS="$save_LIBS"
> +      CPPFLAGS="$save_CPPFLAGS"
> +      LDFLAGS="$save_LDFLAGS"
> +    fi
> +   AM_CONDITIONAL([HAVE_PYTHON3_HEADERS], [test "$have_py3_headers" = "true"])])
> +  ])
> diff --git a/python/automake.mk b/python/automake.mk
> index ecad39d..874a178 100644
> --- a/python/automake.mk
> +++ b/python/automake.mk
> @@ -79,10 +79,10 @@ ovs-install-data-local:
>         rm python/ovs/dirs.py.tmp
>
>  python-sdist: $(srcdir)/python/ovs/version.py $(ovs_pyfiles) python/ovs/dirs.py
> -       (cd python/ && $(PYTHON) setup.py sdist)
> +       cd $(srcdir)/python/ && $(PYTHON) setup.py sdist
>
>  pypi-upload: $(srcdir)/python/ovs/version.py $(ovs_pyfiles) python/ovs/dirs.py
> -       (cd python/ && $(PYTHON) setup.py sdist upload)
> +       cd $(srcdir)/python/ && $(PYTHON) setup.py sdist upload
>  else
>  ovs-install-data-local:
>         @:
> @@ -112,3 +112,26 @@ $(srcdir)/python/ovs/dirs.py: python/ovs/dirs.py.template
>                 < $? > $@.tmp && \
>         mv $@.tmp $@
>  EXTRA_DIST += python/ovs/dirs.py.template
> +
> +.PHONY : clean_python_extensions
> +clean_python_extensions:
> +       cd $(srcdir)/python/ && rm -f ovs/*.so
> +
> +SETUPPY_CFLAGS = -I$(abs_top_srcdir)/include -I$(abs_top_builddir)/include
> +if HAVE_PYTHON_HEADERS
> +$(srcdir)/python/ovs/_json.so: lib/libopenvswitch.la
> +       cd $(srcdir)/python/ && CFLAGS="$(SETUPPY_CFLAGS)" $(PYTHON) setup.py build_ext --inplace
> +
> +ALL_LOCAL += $(srcdir)/python/ovs/_json.so
> +endif
> +
> +if HAVE_PYTHON3_HEADERS
> +PY3_EXT_NAME=$(srcdir)/python/ovs/_json$(shell $(PYTHON3) -c \
> +                        "from distutils import sysconfig;print(sysconfig.get_config_var('EXT_SUFFIX'))")
> +$(PY3_EXT_NAME): lib/libopenvswitch.la
> +       cd $(srcdir)/python/ && CFLAGS="$(SETUPPY_CFLAGS)" $(PYTHON3) setup.py build_ext --inplace
> +
> +ALL_LOCAL += $(PY3_EXT_NAME)
> +endif
> +
> +CLEAN_LOCAL += clean_python_extensions
> diff --git a/python/ovs/json.py b/python/ovs/json.py
> index ea0400a..ddf5dd2 100644
> --- a/python/ovs/json.py
> +++ b/python/ovs/json.py
> @@ -12,11 +12,13 @@
>  # See the License for the specific language governing permissions and
>  # limitations under the License.
>
> +from __future__ import absolute_import
> +import functools
> +import json
>  import re
>  import sys
>
>  import six
> -from six.moves import range
>
>  try:
>      import ovs._json
> @@ -25,112 +27,24 @@ except ImportError:
>
>  __pychecker__ = 'no-stringiter'
>
> -escapes = {ord('"'): u"\\\"",
> -           ord("\\"): u"\\\\",
> -           ord("\b"): u"\\b",
> -           ord("\f"): u"\\f",
> -           ord("\n"): u"\\n",
> -           ord("\r"): u"\\r",
> -           ord("\t"): u"\\t"}
> -for esc in range(32):
> -    if esc not in escapes:
> -        escapes[esc] = u"\\u%04x" % esc
> -
>  SPACES_PER_LEVEL = 2
> -
> -
> -class _Serializer(object):
> -    def __init__(self, stream, pretty, sort_keys):
> -        self.stream = stream
> -        self.pretty = pretty
> -        self.sort_keys = sort_keys
> -        self.depth = 0
> -
> -    def __serialize_string(self, s):
> -        self.stream.write(u'"%s"' % ''.join(escapes.get(ord(c), c) for c in s))
> -
> -    def __indent_line(self):
> -        if self.pretty:
> -            self.stream.write('\n')
> -            self.stream.write(' ' * (SPACES_PER_LEVEL * self.depth))
> -
> -    def serialize(self, obj):
> -        if obj is None:
> -            self.stream.write(u"null")
> -        elif obj is False:
> -            self.stream.write(u"false")
> -        elif obj is True:
> -            self.stream.write(u"true")
> -        elif isinstance(obj, six.integer_types):
> -            self.stream.write(u"%d" % obj)
> -        elif isinstance(obj, float):
> -            self.stream.write("%.15g" % obj)
> -        elif isinstance(obj, six.text_type):
> -            # unicode() on Python 2, or str() in Python 3 (always unicode)
> -            self.__serialize_string(obj)
> -        elif isinstance(obj, str):
> -            # This is for Python 2, where this comes out to unicode(str()).
> -            # For Python 3, it's str(str()), but it's harmless.
> -            self.__serialize_string(six.text_type(obj))
> -        elif isinstance(obj, dict):
> -            self.stream.write(u"{")
> -
> -            self.depth += 1
> -            self.__indent_line()
> -
> -            if self.sort_keys:
> -                items = sorted(obj.items())
> -            else:
> -                items = six.iteritems(obj)
> -            for i, (key, value) in enumerate(items):
> -                if i > 0:
> -                    self.stream.write(u",")
> -                    self.__indent_line()
> -                self.__serialize_string(six.text_type(key))
> -                self.stream.write(u":")
> -                if self.pretty:
> -                    self.stream.write(u' ')
> -                self.serialize(value)
> -
> -            self.stream.write(u"}")
> -            self.depth -= 1
> -        elif isinstance(obj, (list, tuple)):
> -            self.stream.write(u"[")
> -            self.depth += 1
> -
> -            if obj:
> -                self.__indent_line()
> -
> -                for i, value in enumerate(obj):
> -                    if i > 0:
> -                        self.stream.write(u",")
> -                        self.__indent_line()
> -                    self.serialize(value)
> -
> -            self.depth -= 1
> -            self.stream.write(u"]")
> -        else:
> -            raise Exception("can't serialize %s as JSON" % obj)
> +dumper = functools.partial(json.dumps, separators=(",", ":"),
> +                           ensure_ascii=False)
>
>
>  def to_stream(obj, stream, pretty=False, sort_keys=True):
> -    _Serializer(stream, pretty, sort_keys).serialize(obj)
> +    stream.write(dumper(obj, indent=SPACES_PER_LEVEL if pretty else None,
> +                        sort_keys=sort_keys))
>
>
>  def to_file(obj, name, pretty=False, sort_keys=True):
> -    stream = open(name, "w")
> -    try:
> +    with open(name, "w") as stream:
>          to_stream(obj, stream, pretty, sort_keys)
> -    finally:
> -        stream.close()
>
>
>  def to_string(obj, pretty=False, sort_keys=True):
> -    output = six.StringIO()
> -    to_stream(obj, output, pretty, sort_keys)
> -    s = output.getvalue()
> -    output.close()
> -    return s
> +    return dumper(obj, indent=SPACES_PER_LEVEL if pretty else None,
> +                  sort_keys=sort_keys)
>
>
>  def from_stream(stream):
> diff --git a/tests/json.at b/tests/json.at
> index 32d7fff..ba7d4bb 100644
> --- a/tests/json.at
> +++ b/tests/json.at
> @@ -1,4 +1,4 @@
> -m4_define([JSON_CHECK_POSITIVE_C],
> +m4_define([JSON_CHECK_POSITIVE_C],
>    [AT_SETUP([$1])
>     AT_KEYWORDS([json positive])
>     AT_CHECK([printf %s "AS_ESCAPE([$2])" > input])
> @@ -11,7 +11,7 @@ m4_define([JSON_CHECK_POSITIVE_C],
>  # JSON_CHECK_POSITIVE_PY(TITLE, INPUT, OUTPUT, TEST-JSON-ARGS,
>  #                        PYTHON-CHCEK, PYTHON-BIN)
>  #
> -m4_define([JSON_CHECK_POSITIVE_PY],
> +m4_define([JSON_CHECK_POSITIVE_PY],
>    [AT_SETUP([$1])
>     AT_KEYWORDS([json positive Python])
>     AT_SKIP_IF([test $5 = no])
> @@ -41,6 +41,12 @@ m4_define([JSON_CHECK_POSITIVE],
>     JSON_CHECK_POSITIVE_PY([$1 - Python3], [$2], [$3], [$4],
>                            [$HAVE_PYTHON3], [$PYTHON3])])
>
> +m4_define([JSON_CHECK_POSITIVE_PY23],
> +  [JSON_CHECK_POSITIVE_PY([$1 - Python2], [$2], [$3], [$4],
> +                          [$HAVE_PYTHON], [$PYTHON])
> +   JSON_CHECK_POSITIVE_PY([$1 - Python3], [$2], [$3], [$4],
> +                          [$HAVE_PYTHON3], [$PYTHON3])])
> +
>  m4_define([JSON_CHECK_NEGATIVE_C],
>    [AT_SETUP([$1])
>     AT_KEYWORDS([json negative])
> @@ -216,10 +222,14 @@ JSON_CHECK_POSITIVE(
>  # It seems likely that the following test will fail on some system that
>  # rounds slightly differently in arithmetic or in printf, but I'd like
>  # to keep it this way until we run into such a system.
> -JSON_CHECK_POSITIVE(
> -  [large integers that overflow to reals],
> +JSON_CHECK_POSITIVE_C(
> +  [C - large integers that overflow to reals],
>    [[[9223372036854775807000, -92233720368547758080000]]],
>    [[[9.22337203685478e+21,-9.22337203685478e+22]]])
> +JSON_CHECK_POSITIVE_PY23(
> +  [large integers that overflow to reals],
> +  [[[9223372036854775807000, -92233720368547758080000]]],
> +  [[[9.223372036854776e+21,-9.223372036854776e+22]]])
>
>  JSON_CHECK_POSITIVE(
>    [negative zero],
> @@ -237,10 +247,14 @@ JSON_CHECK_POSITIVE(
>  # It seems likely that the following test will fail on some system that
>  # rounds slightly differently in arithmetic or in printf, but I'd like
>  # to keep it this way until we run into such a system.
> -JSON_CHECK_POSITIVE(
> -  [+/- DBL_MAX],
> +JSON_CHECK_POSITIVE_C(
> +  [C - +/- DBL_MAX],
>    [[[1.7976931348623157e+308, -1.7976931348623157e+308]]],
>    [[[1.79769313486232e+308,-1.79769313486232e+308]]])
> +JSON_CHECK_POSITIVE_PY23(
> +  [+/- DBL_MAX],
> +  [[[1.7976931348623157e+308, -1.7976931348623157e+308]]],
> +  [[[1.7976931348623157e+308,-1.7976931348623157e+308]]])
>
>  JSON_CHECK_POSITIVE(
>    [negative reals],
> --
> 1.8.3.1
>

On Mon, Jul 25, 2016 at 06:55:49PM -0500, Terry Wilson wrote:
> Sigh. And of course I had libopenvswitch installed on the system as
> well and removing it breaks building the extensions with the above
> patch. This is the kind of thing that would be much easier if the
> Python lib was its own project with its own test suite and it could
> just always assume libopenvswitch was installed on the system as a
> build dependency.

The Open vSwitch build process uses some of the OVS Python libraries, so
as far as I can tell that would create a circular dependency.

I don't know what is the right thing to do.

On Fri, Jul 29, 2016 at 12:27 AM, Ben Pfaff <blp@ovn.org> wrote:
> On Mon, Jul 25, 2016 at 06:55:49PM -0500, Terry Wilson wrote:
>> Sigh. And of course I had libopenvswitch installed on the system as
>> well and removing it breaks building the extensions with the above
>> patch. This is the kind of thing that would be much easier if the
>> Python lib was its own project with its own test suite and it could
>> just always assume libopenvswitch was installed on the system as a
>> build dependency.
>
> The Open vSwitch build process uses some of the OVS Python libraries, so
> as far as I can tell that would create a circular dependency.
>
> I don't know what is the right thing to do.

I know everyone seems to hate them, but git submodules aren't a
*completely* horrible fit for this issue; except for that whole
changing how people have to clone the repo with --recursive thing.
Since they're pinned to commits, there wouldn't be any breaking of the
build system, etc. I don't know. I may be something to think about,
anyway.