Re: svn commit: r1813665 - in /subversion/branches/swig-py3: ./ build/ac-macros/ subversion/bindings/swig/ subversion/bindings/swig/include/ subversion/bindings/swig/python/libsvn_swig_py/

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1813665 - in /subversion/branches/swig-py3: ./ build/ac-macros/ subversion/bindings/swig/ subversion/bindings/swig/include/ subversion/bindings/swig/python/libsvn_swig_py/

Daniel Shahaf-2
[hidden email] wrote on Sun, Oct 29, 2017 at 03:37:38 -0000:

> On branch swig-py3: Use py3c library in Python swig bindings.
>
> Add the py3c Python compatibility library and update the python swig bindings
> to use the compatibility functions that it provides. This is the first step to
> getting the swig bindings to support Python 3.
>
> * ac-macros/py3c.m4:
>   Create a new ac-macro for the py3c library.
>
> * aclocal.m4:
>   Include the new py3c.m4 file.
>
> * ac-macros/swig.m4:
>   (SVN_FIND_SWIG): Call the new SVN_PY3C macro to add py3c to the build.

s#ac-macros#build/ac-macros# (two places)

> +++ subversion/branches/swig-py3/build/ac-macros/py3c.m4 Sun Oct 29 03:37:37 2017
> @@ -0,0 +1,90 @@
> +AC_DEFUN(SVN_PY3C,

As mentioned in a previous thread: at some point the windows build
system glue would need to be done, too.  (At least to make sure swig-py2
can still be built on windows)  This doesn't need to happen now; just
something to keep in mind.

Actually, don't keep it in mind; keep it in the BRANCH-README file.
That's where usually we keep such TODO's.  (That file should also
describe the purpose of the branch, possibly by referencing the dev@
thread.)

> +[
> +  py3c_found=no
> +  py3c_skip=no
> +
> +  AC_ARG_WITH(py3c,AS_HELP_STRING([--with-py3c=PREFIX],
> +                                  [py3c python extension compatibility library]),
> +  [
> +    if test "$withval" = "yes"; then
> +      py3c_skip=no
> +    elif test "$withval" = "no"; then
> +      py3c_skip=yes
> +    else
> +      py3c_skip=no
> +      py3c_prefix="$withval"
> +    fi
> +  ])
> +
> +  if test "$py3c_skip" = "yes"; then
> +    AC_MSG_ERROR([subversion swig python bindings require py3c])
> +  fi
> +

This conditional couples the function to its caller: it only makes sense
if SVN_PY3C is (only) called from build/ac-macros/swig.m4.

Could you please either decouple the two, or alternatively document the
coupling clearly so any potential future uses of SVN_PY3C would be aware
of it?

Additionally, it's a matter of taste, but I'd have found a comment to
the effect of "Locate py3c, or error out if it's not found" useful.

The semantics here are that even building swig-py2 would require py3c:
users of swig-py2 1.9 would have to install py3c for rebuilding against
1.10.  I think that's acceptable: py2 has been EOL for years and py3c is
a small dependency.

> +  if test -n "$py3c_prefix"; then
> +    AC_MSG_NOTICE([py3c library configuration via prefix])

> +  else

> +    if test "$py3c_found" = "no"; then
> +      AC_MSG_NOTICE([py3c library configuration])

Suggest to have the notice say "via foo" like the other notices do (for
some appropriate value of $foo).

> +AC_DEFUN(SVN_PY3C_PKG_CONFIG,
> +[
> +  AC_MSG_NOTICE([py3c library configuration via pkg-config])

> +])
>
> Modified: subversion/branches/swig-py3/subversion/bindings/swig/core.i

I've skimmed the bindings/ changes and they LGTM.

Cheers,

Daniel