troycurtisjr_at_apache.org 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
Received on 2017-10-29 13:23:45 CET