Re: Introduction and new PHP extension: php_rsync

Hi, On Mon, 2011-05-09 at 13:56 +0200, Michele Catalano wrote: > The Extension is under BSD Licence. This is good. > The librsync library itself can be found at > http://librsync.sourceforge.org/ and implements rsync > remote-delta > algorithm (http://rsync.samba.org/tech_report). Currently you are bundling librsync. librsync is licensed under the "GNU Lesser General Public License [...] version 2.1 [...], or [...] any later version." By this bundling one might argue that you are creating a single library which includes LGPL code. LGPL 2.1 then requires that "the whole of the work to be licensed [...] under the terms of this License." Which means that the whole extension has to be licensed under the terms of the LGPL, not BSD License. Best is not to bundle the LGPL code (or bundle it in a way that two distinct libraries (.so, .dynlib, .dll) are being created. Others might have other interpretations. I won't start interpreting the consequences of "any later version" and LGPL3. :-) Besides that: I had a quick(!) look on the code, no real issues were apparent. But one small comment: I suggest refactoring php_rsync.h so it doesn't include librsync.h, for instance by splitting it up into two headers. The php_$extname header is included from main/builtin_functions.c in case it is statically compiled into PHP including too much stuff there increases the risk of symbol conflicts. In the same file I also suggest a set of parentheses for #define RSYNC_HAVE_PHP_53 ZEND_MODULE_API_NO >= 20071006 like #define RSYNC_HAVE_PHP_53 (ZEND_MODULE_API_NO >= 20071006) which should be a standard practice for macros to avoid conflicts. (this might also be more readable when comparing PHP_VERSION_ID >= 50300) In the MINIT implementation I see you are using Exceptions, which we typically don't do in procedural code. That aside I suggest making RsyncException an interface and having the other Exceptions extend from SPL Exception types. If the license thing is cleared this is a +1. johannes