On 27-09-2010 17:34, Thien-Thi Nguyen wrote: > () Rodrigo Rodrigues da Silva <pita...@members.fsf.org> > () 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
