Support for PHP 7 #571
Comments
|
I'd like to have PHP7 support, too |
|
I've done most of the PHP backend work recently, but this is not something I'm likely to have time to work on in the near future. I can probably make time to review a patch. No more "me too" comments please - they just waste my time, which if anything makes it less likely I'll find time to work on this. |
|
Rather than a "me too", I'd like to volunteer to help out -- do you have any guidance on where to start to help with the PHP7.0 support? I'll take a look at the code and see if I can spin up some kind of patch (but I'm new to SWIG, so please be patient :) |
|
There are essentially two approaches to this. Some background is probably needed for context. The current PHP backend uses PHP's C API to wraps C++ class methods as "flat" functions which take the object as the first parameter. And it also generates a PHP file which defines PHP classes with methods which call those flat functions. This works OK, but there's a lot of unnecessary indirection going on in PHP, so it won't win prizes for speed. How much of an issue that is depends on the API - if you're only making a few method calls and each does a lot of work, the overhead per call is not a big deal, but if there are huge numbers of method calls which return fairly quickly, you incur a lot of overhead. We've also had bugs due to the generated C++ and PHP layers not matching properly. For PHP4, we created PHP objects directly using PHP's C API. This changed for PHP5 because the old API was no longer present - it turns out there was a new one, but it wasn't documented anywhere back then, so we didn't know about it. It is now documented, but nobody's taken the time to reimplement to use it - it was offered as a GSoC project, but nobody picked it: For PHP7, we could either stick with the PHP5 approach - i.e. reimplement the flat file wrapping to use the PHP7 C API, and then adjust the PHP class wrapper generation for any relevant changes to the PHP language (not sure how much it's changed - I've not really had a chance to look at PHP7 yet). Or we could go back to the PHP4 approach, and create the classes using the PHP7 C API, and then we can scrap the PHP class wrapper generation code entirely. The second approach probably gives a better result. I'm not sure which would actually end up being more work - the current code is quite complicated to follow because it's generated two levels of wrappers in different languages, so not having to get to grips with it might balance having to actually implement more code using the PHP7 API. SWIG has a fairly extensive testsuite, which would help here. I found when implementing the PHP5 support that a good approach was to take a copy of the current output for an testcase which didn't work, adjusting it by hand so it did work, and then getting SWIG to generate what I'd done by hand. Simply repeat until the testsuite all passes... I'm not sure how different the PHP5 and PHP7 backend code in SWIG would end up being. If they will have a lot in common, then just patching the current backend makes sense. If they'll differ a lot (which is probably likely if we scrap the PHP class wrappers), having a separate PHP7 backend might make sense - we do after all know how long PHP5 will be around for - 1 year of Active Support (ending Dec 31, 2016), plus 2 years of Security Support (ending Dec 31, 2018) according to https://wiki.php.net/rfc/php56timeline |
|
Perfect! Thanks for the help! I'm not guaranteeing anything, but I'll take a stab at it. As the "maintainer", do you have a preference between the two approaches? It seems like the latter (the PHP4 approach) might be easier to maintain long-term (and eventually for supporting PHP?). |
|
I think the second approach is probably where we eventually want to be (it's what we'd have done for PHP5 support had we known how to), but I wouldn't refuse a patch for PHP7 support just because it took the first approach. |
|
DISCLAIMER: The stuff I am writing is general as I even haven't seen swig and swig generated sources for PHP. @nacc thank you for taking care of this From what I have seen in native PECL extensions, the major change is So in fact it might make sense to change the PHP5 model to generate code like this (taken from xdebug as an example): Also look at the https://github.com/mongodb/mongo-php-driver/blob/master/phongo_compat.h#L150 This with zval handling changes might provide the thin compatibility layer you are looking for. The thinnest layer I've seen so far is in php-uuid (but that doesn't need much): The full rewrite to PHP4 API style would be great, but it might be bigger task than the small changes you need right now. |
|
maybe to avoid confussions swig -php7 ... |
|
From what @oerdnj says, it sounds like the same generated wrapper file could just work with PHP5 and PHP7 - if so, there would be no point having a new version-specific command-line option. |
|
There's now https://github.com/flaupretre/pecl-compat that could be used to wrap most of the functions. That might help fixing the bug. |
|
@nacc Did you manage to make any progress on this? |
|
@ojwb sadly not a ton of progress. I was pulled onto some other things. Let me try and work on this again on my flight back from a work trip and see what I can get you next week. |
|
Another reference to keep in mind when this is updated |
|
@nacc Did you get anywhere? I'll probably have some time to work on this, but I don't want to waste any useful work others might have already done. So it's totally fine if you didn't - I really just want to know. |
|
On Aug 14, 2016 21:02, "Olly Betts" notifications@github.com wrote:
Sadly no, my work priorities have shifted. Sorry for not communicating that |
|
https://github.com/ojwb/swig/tree/php7 now has an initial version of PHP7 support which at least passes SWIG's testsuite. At this stage I really wouldn't advise using this in production or distributing packages based on this code, but I'd encourage people to try it out and report any issues. On the branch, the PHP7 support currently entirely replaces the PHP5 support, but the next step is to address that. Having looked at the pecl-compat stuff (or similar approaches adding compatibility stuff by hand) I concluded that wasn't the right approach here. Trying to refactor the PHP5 support to use compatibility functions is just asking to introduce bugs, whereas if we just leave it be it can die in not much over a year once PHP5 reaches end of life. It seems better to just create a new PHP7 backend (based on the PHP5 one) which can use the PHP7 C API directly. We may have to fix bugs in two places, but only for just over a year, and the rate of bug fixes in SWIG's PHP backend isn't high enough that this is a big issue. With a merged backend we'd need to test fixes against both PHP versions anyway. |
|
Sounds good. How will the two co-exist? Will PHP7 be invoked by a command -php7 and -php will always be PHP5? How about -php (PHP5) and -php7 for swig-3.1 and -php switches to mean -php7 in swig-3.2 when PHP5 support could be dropped. Or maybe just drop PHP5 support in swig-3.1 altogether and -php means php7 in swig-3.1. |
|
I'd not really thought about that part yet, but I guess Do we have even a rough estimate of a date for SWIG 3.1 to become a stable release? I think I'd prefer not to drop PHP5 support until it reaches EOL upstream (and it's useful to at least me to be able to generate bindings for both PHP5 and PHP7 from a single SWIG build). |
|
It seems there's something wrong in the director code, but I'm failing to work out exactly what so far. But if run under valgrind (using director_basicdirector_pass_by_value |
|
So I stopped poking it and soon realised where the issue likely was, and my hunch was right - the branch now passes the test suite under valgrind too. There are a few "definitely leaked" allocations, but from the small sample I checked those look to be in the testcases themselves. |
May I kindly ask to keep PHP 5 support in SWIG longer than until EOL at its upstream? Linux distributions with long term support (such as RHEL/CentOS or SLES) still have to support PHP 5 for multiple years from now, while upstreams using SWIG in their build chains might prefer a recent SWIG release anyway… |
It needs someone who is actually going to actively maintain that support, which isn't something I have any interest in beyond the upstream EOL. In fact my distro already dropped PHP5, so the only reason I can currently test PHP5 it is because I still have the packages installed from before they did, but those packages aren't getting security updates... If the PHP5 support is just going to sit and bitrot, it's more honest to just remove it. Upstream EOL is a natural point to do that. I'll note we also dropped PHP4 support at PHP4 EOL (2008-08-08, last SWIG release to support it was 1.3.36 on 2008-06-24) and I've not seen anyone complain about that decision at all. |
|
http://php.net/supported-versions.php shows PHP5 EOL is Jan 2019, so no doubt we can get maintained packages to test until this date from distros that will support it until EOL. We ought to have swig-3.1.x out by then, so swig-3.1.x would support both php5 and php7 until we release swig-3.2 which is probably going to be after 2019. Regarding the leaks, if the tests can be easily fixed to not leak, please do so. |
|
The distributions that will carry PHP 5 over EOL will also carry swig with php5 support, so I wouldn't worry so much. With my Debian PHP hat on, I am perfectly fine with swig dropping php5 support at the same as php 5.6 EOL (end of 2018). |
From a Debian perspective, the correct question is not really whether we keep PHP5 support in new releases of SWIG, but rather whether there's PHP5 support in the version of SWIG which Debian has in the last stable release to have PHP5 packages. |
|
@wsfulton: I'm not sure how to enable the CI builds for this. It looks like they use Ubuntu trusty (at least mostly - not all seem to specify |
The extension API has changed in PHP 7, here's some information on it: https://wiki.php.net/phpng-upgrading
The text was updated successfully, but these errors were encountered: