Re: [libredwg] Python bindings

[Rodrigo Rodrigues da Silva]

On 27-09-2010 17:34, Thien-Thi Nguyen wrote:
> () Rodrigo Rodrigues da Silva <address@hidden>
> () Wed, 22 Sep 2010 03:10:49 +0200
> 
>    Thi, can you please take a look at configure.ac and the new Makefile.am?
>    They work, at least for me, but there's too much magic there =P
> 
> Overall, looks good to me.  Here are some specific suggestions:
> 
> * configure.ac: improve m4 quoting
>   Every macro invocation should ideally be converted from:
>   
>     foo(bar, baz)
> 
>   to
> 
>     foo([bar],[baz])
> 
>   This protects against the potential for confusion should ‘bar’ or
>   ‘baz’ become macros in their own right (for whatever reason).  In
>   this case, we don't need to worry (dotted numbers like 1.3.17 are
>   unlikely candidates for macroization), but better safe than sorry:
> 
>   AM_PATH_PYTHON([2.3])
>   AX_PKG_SWIG([1.3.17], [], [ AC_MSG_ERROR([SWIG is required to build..]) ])

done

> 
> * configure.ac: SWIG should be optional
>   Presently, i cannot build LibreDWG because i don't have SWIG installed
>   (or rather, the SWIG i do have installed is too ancient and broken :-D).
>   I see and agree fully with the comment:
> 
>   dnl for SWIG - should be optional
> 
>   This is a job for ‘AC_ARG_ENABLE’ (info "(autoconf) Package Options")
>   or ‘AC_ARG_WITH’ (info "(autoconf) External Software"); you need to
>   decide which kind of "optional" this falls under (personally, i can
>   see the case for either one).  If you let me know your decision, i
>   can post an appropriate patch.

I really don't know which is the best way to go. I'd go with
AC_ARG_ENABLE - specially because we plan to have other bindings:
--enable-python, --enable-perl. Feel free to do whatever you prefer.
BTW, SWIG should be checked (it is currently not) if any bindings are
enabled.

Nonetheless, I think we should go even further in the future: different
versions of swig might generate different (and possibly buggy) wrappers.
Therefore, we might want to have a separate make target that will
generate (and not build) sources for tarball releases, and if the
sources are there make won't generate them again.

> 
> * bindings/python/Makefile.am: style nits
>   I find the extra whitespace at beginning-of-line to be strange.

Well, I was just following an example that was formatted like this for
some reason I am not aware of.

>   Also, why is the leading underscore in ‘_libredwg_la’ necessary?

This is Python specific. When bar python module is loaded, the
interpreter will look for _bar.so

>   You should add a comment explaining this anomaly.

done

>   
> * bindings/python/Makefile.am: ‘pkgpythondir’ override
>   I don't understand why this is necessary:
> 
>   ## magic to override pkgpythondir = $(pythondir)/$(PACKAGE)
>   ## at least in Ubuntu python wouldn't find the module there
>       pkgpythondir = $(pythondir)
> 
>   I am concerned that this override will create problems in the future.
>   Specifically, what implications does this override have for:
>   - interop with other non-SWIG packages
>   - interop with other SWIG-processed packages
>   - interop with future (possibly incompatible) versions of LibreDWG
>   ?

Yes, it will create problems. At least in Fedora I already know that it
doesn't work.

If I don't override it, at least in ubuntu, the .so files will be placed
in /usr/local/lib/python-2.6/dist-packages/libredwg and not in
/usr/local/lib/python-2.6/dist-packages/ and the interpreter won't find
them. In Fedora, the interpreter can't find the module either way, so it
might be an external problem - and might also be the case in ubuntu.

> 
> * bindings/python/Makefile.am: hardcoded directory name
>   The /usr/include/python2.6 is not cool:
> 
>   ## more magic: SWIG_PYTHON_CPPFLAGS resolves to null and python includes
>   ## are not passed to gcc via -I
>   ##    _libredwg_la_CPPFLAGS = $(SWIG_PYTHON_CPPFLAGS) -I$(top_srcdir)/src
>       _libredwg_la_CPPFLAGS = -I$(top_srcdir)/src -I/usr/include/python2.6
> 
>   Once we figure out how to make SWIG+python optional, probably the answer
>   to "how to propagate ‘SWIG_PYTHON_CPPFLAGS’" will become evident.

Anyway, Autotools macros for SWIG are possibly very buggy.

> 
> * [tangential] commit log syntax
>   Please add a space between the asterisk (*) and the filename.
>   Also, i see that you added categories ‘bind’ and ‘api’ (cool)
>   but haven't actually used them!

I used [b] but then for some reason decided to label it [bind] and use
it again in the future. [api] will be used too in future work that I am
planning to do.

> 
> That's all for now.  I am happy to see progress in this area.
> 
> thi
> 

Thanks for your advice!

-- 
Rodrigo Rodrigues da Silva

PoliGNU - Grupo de Estudos de Software Livre da Poli/USP
GNU Project

0xE67BD84E.asc
Description: application/pgp-keys

0xE67BD84E.asc
Description: application/pgp-keys

signature.asc
Description: OpenPGP digital signature