troycurti...@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
