Re: Request for a PECL account to maintain NuoDB driver.

Hi Tom, On Mon, February 17, 2014 14:30, Tom Gates wrote: > Hello Anatol, > > > > So C++ style comments are not allowed? No problem. I can change them to > use C style comments. There's coding styles http://git.php.net/?p=php-src.git;a=blob;f=CODING_STANDARDS regading this, whereby C++ is of course another story. > > What is suspicious about those global variables? They are initialized in > pdo_nuodb_handle_factory(). Is there some risk in that? Is there a > preferred way to do it? Yeah, that factory function is called on each and every PDO::__construct(), that means it'll overwrite the descriptor. Plus a thread safe variant and it'll be leaking descriptors even more. Here's the relevant code http://lxr.php.net/xref/PHP_5_6/ext/pdo/pdo_dbh.c#383 I'd anyway suggest to use the PHP streams IO routines for this, php_stream_fopen() and co, and move the custom logging initialization into the MINIT phase (and closing descriptors in MSHUTDOWN). Actually global variables are always suspicious :) > Thanks, > -tom > > > > > On Thu, Feb 13, 2014 at 3:14 PM, Anatol Belski <anatol.php@belski.net > <mailto:anatol.php@belski.net> > wrote: > > > > Hi Tom, > > > > On Thu, February 13, 2014 20:11, Tom Gates wrote: > >>> Some can compile, but from the experience most of users prefer just >>> to have it. That's why that build system was brought to life (since >>> like early spring 2013). The binaries are then available from the PECL >>> site along with the tarballs, like here for instance > >>> http://pecl.php.net/package/oci8 >>> <http://pecl.php.net/package/oci8> . >>> > >>> >> >> Yes. We would like the do the same as other drivers. >> >> >> >> Is there something else that I need to do, or should I apply for an >> account now? >> > > as for me yes. From the code perspective there are some minor issues like > c++ style comments and presence of confirm_pdo_nuodb_compiled() > function. Also the global variables here > > https://github.com/nuodb/nuodb-php-pdo/blob/master/nuodb_driver.c#L74 > look > suspicious to me. > > >> -tom >> >> >> >> >> >> On Thu, Feb 13, 2014 at 1:53 PM, Anatol Belski >> <anatol.php@belski.net >> <mailto:anatol.php@belski.net> >wrote: >> >> >> >>> Hi Tom, >>> >>> >>> >>> On Thu, February 13, 2014 19:15, Tom Gates wrote: >>> >>> >>>> Hello Anatol, >>>> >>>> >>>> >>>> >>>> I see. The build of the NuoDB PHP PDO Driver depends on a NuoDB >>>> client libraries and header files. Just like the MySQL PHP PDO >>>> Driver >>>> depends >>> on >>>> the MySQL client libraries and header files. Our product: NuoDB >>>> contains the library NuoRemote.lib (which is built using VC10), and >>>> NuoRemote_vc9.lib (which is built using VC9). We choose to name >>>> it that way on Windows. Because the major distribution for PHP 5.3 & >>>> 5.4 >>>> is built using VC9, then the PDO driver must "link" to the >>>> "_vc9" >>>> version of our library (to avoid MSVCRT skew). >>>> >>> That's a smart decision anyway not to mix CRT. Also with the naming - >>> if it's distributed that way, so the main thing is that config.w32 >>> works. >>> >>>> Yes, The build system won't be able to build the NuoDB PHP PDO >>>> Driver >>>> without the NuoDB client libraries and header files. Just like the >>>> MySQL >>>> PHP PDO Driver can't be built without the MySQL client libraries and >>>> header files. I can provide a NuoDB dependency package for >>>> http://windows.php.net/downloads/pecl/deps/ >>>> >>>> >>>> >>> We actually don't have to redistribute it there, just have it on the >>> build box. That's how it also being done for several extensions with >>> dependencies in binary form only. Given there's no legal issue, of >>> course. >>> >>>> We expect that NuoDB customers will download the NuoDB PHP PDO >>>> Driver >>>> extension from PECL and build it on their local machines. >>>> >>> Some can compile, but from the experience most of users prefer just >>> to have it. That's why that build system was brought to life (since >>> like early spring 2013). The binaries are then available from the PECL >>> site along with the tarballs, like here for instance >>> http://pecl.php.net/package/oci8 . >>> >>> >>> >>>> NuoDB does not (yet) provide a VC11 version of our client library. >>>> When >>>> it does, then the NuoDB PHP PDO driver can be modified to use the >>>> VC11 >>>> version of NuoRemote for PHP 5.5. >>>> >>>> Does that sound ok? >>>> >>>> >>>> >>> Yes, if we only have VC9 dependencies, the corresponding extension >>> builds only will succeed. When the VC11 deps are there, they can be >>> added and in this case probably config.w32 will have to be brought >>> inline. And we can anyway do some snapshots before the release to > ensure >>> it'll succeed. >>> >>>> >>>> -tom >>>> >>>> >>>> >>>> >>>> >>>> On Thu, Feb 13, 2014 at 12:39 PM, Anatol Belski >>>> <anatol.php@belski.net >>>> <mailto:anatol.php@belski.net> >wrote: >>>> >>>> >>>> >>>> >>>>> Hi Tom, >>>>> >>>>> >>>>> >>>>> >>>>> On Thu, February 13, 2014 17:18, Tom Gates wrote: >>>>> >>>>> >>>>> >>>>>> Hello Anatol, >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Thank you for looking at the driver. We currently don't >>>>>> support PHP >>>>>> >>>>>> >>>>>> >>>> 5.5 >>>> >>>> >>>> >>>>>> on Windows because NuoDB does not yet have client library >>>>>> support for >>>>>> >>>>> VC11. >>>>> >>>>> >>>>> >>>>>> However, we do support PHP 5.5 on Linux. I don't see the >>>>>> hardcoded >>>>>> >>>> VC9 >>>> >>>> >>>> >>>>>> lib you are talking about in config.w32. What line number do >>>>>> you see >>>> that >>>>>> on? >>>>>> >>>>> I was looking at these lines >>>>> >>>>> >>>>> >>>>> >>>>> if (CHECK_LIB("NuoRemote_vc9.lib", "pdo_nuodb", >>>>> PHP_PDO_NUODB + >>>> "\\lib32") >>>> >>>> >>>> >>>>> && CHECK_HEADER_ADD_INCLUDE('NuoDB.h', >>>>> 'CFLAGS_PDO_NUODB', >>>>> PHP_PDO_NUODB + "\\include") >>>>> && CHECK_HEADER_ADD_INCLUDE('stdint.h', >>>>> 'CFLAGS_PDO_NUODB', >>>>> "include_extras\\VC9")) >>>>> { >>>>> >>>>> >>>>> >>>>> >>>>> and saw names with hardcoded "vc9", so basically my guess was >>>>> correct :) We usually pack the deps by CRT+platform into separated >>>>> zips. There is also usual naming for the subdirs like lib, include >>>>> and bin. You can >>>> look >>>>> at the dependency packages for other extensions opening any from >>>>> here >>>>> http://windows.php.net/downloads/pecl/deps/ . So ideally it >>>>> should be like NuoRemote.lib. Also, you could use >>>>> win32/php_stdint.h which is present starting with PHP-5.3 IRRC. >>>>> >>>>> But I meant that only if you plan to make use of the build >>>>> system, as >>>>> >>>> once >>>>> a release tarball is uploaded, it will try to build it. For that >>>>> to succeed we'd need the dependency library to be present in the >>>>> build >>>> system >>>>> before and perhaps meet some additional config. >>>>> >>>>> Thanks >>>>> >>>>> >>>>> >>>>> >>>>> Anatol >>>>> >>>>> >>>>> >>>>> >>>>> >>>> >>>> >>>> -- >>>> Tom Gates >>>> Software Engineer >>>> *www.nuodb.com <http://www.nuodb.com> * >>>> <http://www.nuodb.com/> >>>> c: 603-490-4013 <tel:603-490-4013> >>>> e: tgates@nuodb.com <mailto:tgates@nuodb.com> >>>> >>>> >>>> >>>> >>>> The information contained in this email is confidential and >>>> proprietary and intended for the sole use of the intended recipient. >>>> If the reader of >>>> this message is not the intended recipient, you are hereby notified >>>> that any dissemination, distribution or copying of this >>>> communication is strictly prohibited. If you have received this >>>> email in error, please notify the sender and delete all copies. >>>> >>> >>> Regards >>> >>> >>> >>> Anatol >>> >>> >>> >>> >> >> >> -- >> Tom Gates >> Software Engineer >> *www.nuodb.com <http://www.nuodb.com> * >> <http://www.nuodb.com/> >> c: 603-490-4013 <tel:603-490-4013> >> e: tgates@nuodb.com <mailto:tgates@nuodb.com> >> >> >> >> The information contained in this email is confidential and proprietary >> and intended for the sole use of the intended recipient. If the reader >> > of >> this message is not the intended recipient, you are hereby notified >> that any dissemination, distribution or copying of this communication is >> strictly prohibited. If you have received this email in error, please >> notify the sender and delete all copies. >> > > Regards > > > Anatol > > > > > > > -- > > > Tom Gates > Software Engineer > <http://www.nuodb.com/> www.nuodb.com > > > c: 603-490-4013 > > > e: tgates@nuodb.com <mailto:tgates@nuodb.com> > Regards Anatol