Re: [libredwg] Python bindings

[Thien-Thi Nguyen]

() 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..]) ])

* 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.

* bindings/python/Makefile.am: style nits
  I find the extra whitespace at beginning-of-line to be strange.
  Also, why is the leading underscore in ‘_libredwg_la’ necessary?
  You should add a comment explaining this anomaly.
  
* 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
  ?

* 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.

* [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!

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

thi