Discussion:
[Swig-devel] php7 commit
William S Fulton
2016-11-30 21:21:16 UTC
Permalink
Hi Olly

$ ~/swig/github/swig (master=)$ make check-php5-version
showing php5 version
PHP 5.6.27-1+deb.sury.org~xenial+1 (cli)
$ ~/swig/github/swig (master=)$ make check-php5-examples
checking Examples/php5/callback
checking Examples/php5/class
checking Examples/php5/constants
example_wrap.c:724:18: fatal error: zend.h: No such file or directory
compilation terminated.
../../Makefile:1182: recipe for target 'php' failed

This worked before. Looks like the php5/Examples are using the php_xxxx
targets which are actually php7 targets. Does 'swig -php' result in php5 or
php7 wrappers? I trust this change on a stable maintenance branch it means
exactly the same as swig-3.0.10.

If using php5 headers with php7 generated code can you add some nice
compiler error saying the wrong version has been used, and vice-versa?

Why was this commit made without any Travis tests being added? The Travis
tests show it is using php 5.5.9, however, this is with the languages as
'php'. What exactly does 'php' mean now? On my box which has php 5.6 'php'
is skipped, but php5 nearly works as shown above.

Have you tested this with both php5 and php7 installed and on a box and
with just one of the two versions installed? Probably this needs doing and
it will flush out the problems.

William
Olly Betts
2016-11-30 21:49:49 UTC
Permalink
Post by William S Fulton
$ ~/swig/github/swig (master=)$ make check-php5-version
showing php5 version
PHP 5.6.27-1+deb.sury.org~xenial+1 (cli)
$ ~/swig/github/swig (master=)$ make check-php5-examples
checking Examples/php5/callback
checking Examples/php5/class
checking Examples/php5/constants
example_wrap.c:724:18: fatal error: zend.h: No such file or directory
compilation terminated.
../../Makefile:1182: recipe for target 'php' failed
This worked before. Looks like the php5/Examples are using the php_xxxx
targets which are actually php7 targets.
Hmm, I did test this. Not sure what's going wrong, but will sort it out.
Post by William S Fulton
Does 'swig -php' result in php5 or
php7 wrappers? I trust this change on a stable maintenance branch it means
exactly the same as swig-3.0.10.
PHP5, as before (and as discussed in the ticket).
Post by William S Fulton
If using php5 headers with php7 generated code can you add some nice
compiler error saying the wrong version has been used, and vice-versa?
That wouldn't catch the case above as the failure is trying to include the PHP
headers, and we can't check what version the headers are for if we fail to
find them. Might be nice for the case of finding the wrong headers though, if
assuming it is feasible then.
Post by William S Fulton
Why was this commit made without any Travis tests being added? The Travis
tests show it is using php 5.5.9, however, this is with the languages as
'php'. What exactly does 'php' mean now? On my box which has php 5.6 'php'
is skipped, but php5 nearly works as shown above.
I attempted to ensure the new backend fulfilled all the documented
prerequisites before merging it. Those prerequisites don't include CI
support:

http://www.swig.org/Doc3.0/Extending.html#Extending_prerequisites

In fact I more than fulfilled some of them - the *ENTIRE* testsuite passes,
not just the majority.

I didn't follow the letter of #7, in that my patch was provided via
github rather than email, but I feel that's actually more useful with
our current infrastructure - it's easier to browse the changes (and
you can get a flat file patch easily) plus mailing lists tend to eat large
attachments.

As for adding CI, please see my comment in the ticket, where I asked you how to
add it. There aren't PHP7 packages in trusty, and I'm no expert in travis so I
don't know if we can just ask for a xenial VM/container/chroot/whatever, or how
best to pull packages from a PPA if we have to use trusty.
Post by William S Fulton
Have you tested this with both php5 and php7 installed and on a box and
with just one of the two versions installed? Probably this needs doing and
it will flush out the problems.
I've tested with both installed. Uninstalling PHP5 is troublesome, as those
packages are gone from Debian testing now so it's a pain to reinstall them, and
I really need to have them installed or else I can't usefully maintain SWIG's
PHP5 support (and nobody else seems interested). I guess I can rename the
header directory temporarily.

Cheers,
Olly

------------------------------------------------------------------------------
Olly Betts
2016-11-30 23:17:26 UTC
Permalink
Post by Olly Betts
Post by William S Fulton
$ ~/swig/github/swig (master=)$ make check-php5-version
showing php5 version
PHP 5.6.27-1+deb.sury.org~xenial+1 (cli)
$ ~/swig/github/swig (master=)$ make check-php5-examples
checking Examples/php5/callback
checking Examples/php5/class
checking Examples/php5/constants
example_wrap.c:724:18: fatal error: zend.h: No such file or directory
compilation terminated.
../../Makefile:1182: recipe for target 'php' failed
This worked before. Looks like the php5/Examples are using the php_xxxx
targets which are actually php7 targets.
Hmm, I did test this. Not sure what's going wrong, but will sort it out.
OK - I missed this because if you had both installed, then the PHP5 examples
were just being generate, built and run using PHP7, which works because none of
the differences between the two matter at this level. Now fixed.
Post by Olly Betts
Post by William S Fulton
If using php5 headers with php7 generated code can you add some nice
compiler error saying the wrong version has been used, and vice-versa?
That wouldn't catch the case above as the failure is trying to include the PHP
headers, and we can't check what version the headers are for if we fail to
find them. Might be nice for the case of finding the wrong headers though, if
assuming it is feasible then.
Adding checks for this, though they wouldn't have fired in the above case -
either the PHP7 stuff isn't installed and you get the error you did, or else
it is and everything consistently used PHP7.
Post by Olly Betts
Post by William S Fulton
Have you tested this with both php5 and php7 installed and on a box and
with just one of the two versions installed? Probably this needs doing and
it will flush out the problems.
I've now tested with just php5 installed and with both installed but all the
php5 binaries and header files moved aside, and the testsuite passes for
the installed version and skips for the other in each case, as it ought to.
I did "make distclean" in between tests.

I've also successfully built working PHP7 bindings for Xapian using this
new code.

Cheers,
Olly

------------------------------------------------------------------------------
William S Fulton
2016-12-01 22:06:30 UTC
Permalink
On 30 November 2016 at 23:17, Olly Betts <***@survex.com> wrote:
On Wed, Nov 30, 2016 at 09:49:49PM +0000, Olly Betts wrote:

OK - I missed this because if you had both installed, then the PHP5 examples
Post by Olly Betts
were just being generate, built and run using PHP7, which works because
none of
the differences between the two matter at this level. Now fixed.
Looking better on my local box now.
Post by Olly Betts
Adding checks for this, though they wouldn't have fired in the above case
-
Post by Olly Betts
either the PHP7 stuff isn't installed and you get the error you did, or
else
Post by Olly Betts
it is and everything consistently used PHP7.
Yes, I see you've put this in. I was just thinking it'll generally be a
little more user friendly as the world transitions from php5 to php7.
Post by Olly Betts
Post by William S Fulton
Have you tested this with both php5 and php7 installed and on a box and
with just one of the two versions installed? Probably this needs doing and
it will flush out the problems.
I've now tested with just php5 installed and with both installed but all
the
Post by Olly Betts
php5 binaries and header files moved aside, and the testsuite passes for
the installed version and skips for the other in each case, as it ought
to.
Post by Olly Betts
I did "make distclean" in between tests.
I've also successfully built working PHP7 bindings for Xapian using this
new code.
Yes, congratulations on great work to all involved, this is a great step
forward.
Post by Olly Betts
Post by William S Fulton
Does 'swig -php' result in php5 or
php7 wrappers? I trust this change on a stable maintenance branch it
means
Post by William S Fulton
exactly the same as swig-3.0.10.
PHP5, as before (and as discussed in the ticket).
Understood now, thanks.
Why was this commit made without any Travis tests being added? The Travis
Post by William S Fulton
tests show it is using php 5.5.9, however, this is with the languages as
'php'. What exactly does 'php' mean now? On my box which has php 5.6
'php'
Post by William S Fulton
is skipped, but php5 nearly works as shown above.
I attempted to ensure the new backend fulfilled all the documented
prerequisites before merging it. Those prerequisites don't include CI
http://www.swig.org/Doc3.0/Extending.html#Extending_prerequisites
In fact I more than fulfilled some of them - the *ENTIRE* testsuite passes,
not just the majority.
Yes, having extensive test coverage is ideal. I'm sure we can get this
working on Travis soon enough. All the extensive effort put into the
testing will be wasted if they aren't run in a CI environment.
Post by Olly Betts
From what I can make out there is something completely out of step though.
make-xx-test-suite and make-xx-examples etc should invoke swig -xx, where
xx is a target language. This is how the Makefiles and all the testing
works. From what I can see make-php-examples does not invoke swig -php, it
invokes swig -php7 when it should invoke swig -php (php5), so actually
invokes the wrong version of php. I don't want this inconsistency and I
believe is why Travis is constantly failing now. Can you fix it without
renaming lots of directories and file names? If not, let's git reset --hard
and go back to an earlier revision and fix it up so the history on master
isn't a mess.

I didn't follow the letter of #7, in that my patch was provided via
Post by Olly Betts
github rather than email, but I feel that's actually more useful with
our current infrastructure - it's easier to browse the changes (and
you can get a flat file patch easily) plus mailing lists tend to eat large
attachments.
That page needs updating, I'll have a go at it. I'm sure you've noticed
the precedent over the last few years since we moved to using Github +
Travis of nearly always insisting on a test that can run on Travis before
accepting code changes. Adding a whole module clearly should be no
different and in my view a non-negotiable prerequisite
Post by Olly Betts
As for adding CI, please see my comment in the ticket, where I asked you how to
add it. There aren't PHP7 packages in trusty, and I'm no expert in travis so I
don't know if we can just ask for a xenial VM/container/chroot/whatever, or how
best to pull packages from a PPA if we have to use trusty.
Just seen the comment. A bit more time to reply and an email warning to
swig-devel before the merge into master would have been appreciated.

Travis supports docker images. Anyone here familiar with Docker? In theory
it should be a simple job for someone familiar with this to set up a docker
image with OS of choice as the SWIG dependencies are so simple. Otherwise
it looks really simple to upgrade to php7 on Trusty (which is available on
Travis), see
https://launchpad.net/~ondrej/+archive/ubuntu/php?field.series_filter=trusty
and
https://www.digitalocean.com/community/tutorials/how-to-upgrade-to-php-7-on-ubuntu-14-04.
Anyone can test Travis using a personal account and I suggest doing this as
the .travis file can be whittled down to just php in any personal branch
with a quick turn-around.
Post by Olly Betts
Have you tested this with both php5 and php7 installed and on a box and
Post by William S Fulton
with just one of the two versions installed? Probably this needs doing
and
Post by William S Fulton
it will flush out the problems.
I've tested with both installed. Uninstalling PHP5 is troublesome, as those
packages are gone from Debian testing now so it's a pain to reinstall them, and
I really need to have them installed or else I can't usefully maintain SWIG's
PHP5 support (and nobody else seems interested). I guess I can rename the
header directory temporarily.
I've got php5.6 on Xenial, but Xenial does come with php7 too. I will try
install both on a VM and see how that goes. If you havn't got familiar with
virtual machines yet, I highly recommend it. It is exactly case like this
where one can do some risky/awkward testing and mix old and new versions
and easily roll back the changes without upsetting your main computer. Kind
of essential for the myriad of versions of everything we need to support in
SWIG.

William
Olly Betts
2016-12-01 22:35:57 UTC
Permalink
Post by Olly Betts
From what I can make out there is something completely out of step though.
make-xx-test-suite and make-xx-examples etc should invoke swig -xx, where
xx is a target language. This is how the Makefiles and all the testing
works.
That requirement doesn't seem to be documented in Extending.html.

It also seems we currently have "make check-perl5-test-suite" but the
documented option is "swig -perl" (though the -perl5 is an undocumented
alias, and what the testsuite seems to use).

FWIW, the logic I used was to avoid renaming everything and to make the
PHP7 stuff "php" so that when PHP5 is stripped out we're back to just
having "php".
Post by Olly Betts
From what I can see make-php-examples does not invoke swig -php, it
invokes swig -php7 when it should invoke swig -php (php5), so actually
invokes the wrong version of php. I don't want this inconsistency and I
believe is why Travis is constantly failing now. Can you fix it without
renaming lots of directories and file names? If not, let's git reset --hard
and go back to an earlier revision and fix it up so the history on master
isn't a mess.
"git reset --hard" seems a terrible idea for changes that have been pushed
publically to a branch that isn't marked as being one that might have its
history changed.

I think it would just need everything "php" renaming to "php7". That's
quite a lot of stuff, but the change so far was to copy the "php" stuff as
"php5" and then modifying the "php" stuff to be for PHP7, so the only
difference that renaming now would make to the history is that the "php" to
"php7" renaming isn't the same commit as the "php" to "php5" copying. I
don't think that's really more of "a mess" than a change like this
inherently is, and certainly not enough to justify breaking history.

The downside is then we either have to live with "php7" indefinitely, even
once "php5" is gone, or else we do another mass rename back to "php" once
"php5" is gone (which would make the history messier, which is why I
avoided the "php" to "php7" rename to start with).

The other option is to flip "-php" to alias "-php7" now, but that will
break a lot of user build systems - while "-php5" is supported, the
documented option has been "-php" for some time.
Post by Olly Betts
I've got php5.6 on Xenial, but Xenial does come with php7 too. I will try
install both on a VM and see how that goes. If you havn't got familiar with
virtual machines yet, I highly recommend it. It is exactly case like this
where one can do some risky/awkward testing and mix old and new versions
and easily roll back the changes without upsetting your main computer. Kind
of essential for the myriad of versions of everything we need to support in
SWIG.
I try to avoid creating single purpose VMs such as this because of the
overhead (both in my time and in bandwidth) of keeping them up to date.

Cheers,
Olly
William S Fulton
2016-12-01 23:32:07 UTC
Permalink
Post by Olly Betts
Post by Olly Betts
From what I can make out there is something completely out of step
though.
Post by Olly Betts
make-xx-test-suite and make-xx-examples etc should invoke swig -xx, where
xx is a target language. This is how the Makefiles and all the testing
works.
That requirement doesn't seem to be documented in Extending.html.
Not everything is documented and is why pull requests and reviews are a
good idea before merging to master. Like a lot of things, It is forced by
the (Travis) build system.

It also seems we currently have "make check-perl5-test-suite" but the
Post by Olly Betts
documented option is "swig -perl" (though the -perl5 is an undocumented
alias, and what the testsuite seems to use).
Good observation, we could document this.
FWIW, the logic I used was to avoid renaming everything and to make the
PHP7 stuff "php" so that when PHP5 is stripped out we're back to just
having "php".
Yes, this makes sense when considered in isolation, but unfortunately is
not that simple :(
Post by Olly Betts
From what I can see make-php-examples does not invoke swig -php, it
Post by Olly Betts
invokes swig -php7 when it should invoke swig -php (php5), so actually
invokes the wrong version of php. I don't want this inconsistency and I
believe is why Travis is constantly failing now. Can you fix it without
renaming lots of directories and file names? If not, let's git reset
--hard
Post by Olly Betts
and go back to an earlier revision and fix it up so the history on master
isn't a mess.
"git reset --hard" seems a terrible idea for changes that have been pushed
publically to a branch that isn't marked as being one that might have its
history changed.
I'm not a great fan of a messy commit history with lots of reverts, hence
my conservative nature for the most important branch of all ... master...
am just looking for solutions given a non-ideal situation.

I think it would just need everything "php" renaming to "php7". That's
Post by Olly Betts
quite a lot of stuff, but the change so far was to copy the "php" stuff as
"php5" and then modifying the "php" stuff to be for PHP7, so the only
difference that renaming now would make to the history is that the "php" to
"php7" renaming isn't the same commit as the "php" to "php5" copying. I
don't think that's really more of "a mess" than a change like this
inherently is, and certainly not enough to justify breaking history.
I havn't the time to look into what exactly what needs changing, maybe you
can think of something to just change a few lines in the Makefiles rather
than whole directories renames. Doesn't the Lib/php directory contain code
for php5 and php7 though, but Examples/test-suite/php contain php7 only?
Examples/test-suite/php should be for -php and make check-php-test-suite,
ie php5, seems like a lot of reverts back to what we had before the commit
as Examples/test-suite/php5 will no go back to being
Examples/test-suite/php once the php7 has been moved to a php directory.
Not sure how anyone will track this in the history, hence the drastic step
of hard reset. Maybe it won't be so bad as the contents of the two
test-suite subdirectories are near enough identical?
Post by Olly Betts
The downside is then we either have to live with "php7" indefinitely, even
once "php5" is gone, or else we do another mass rename back to "php" once
"php5" is gone (which would make the history messier, which is why I
avoided the "php" to "php7" rename to start with).
This is probably why perl5 subdirectories are not called perl now. Maybe
we can have make-php-test-suite running the tests in the php5 subdirectory
and then when php5 is dropped it can run the tests in the php7 subdirectory
(so much like perl5).
Post by Olly Betts
The other option is to flip "-php" to alias "-php7" now, but that will
break a lot of user build systems - while "-php5" is supported, the
documented option has been "-php" for some time.
Agreed, this can't be done for the stable 3.0.x releases. Would be fine
for swig-3.1 though.
Post by Olly Betts
I've got php5.6 on Xenial, but Xenial does come with php7 too. I will try
Post by Olly Betts
install both on a VM and see how that goes. If you havn't got familiar
with
Post by Olly Betts
virtual machines yet, I highly recommend it. It is exactly case like this
where one can do some risky/awkward testing and mix old and new versions
and easily roll back the changes without upsetting your main computer.
Kind
Post by Olly Betts
of essential for the myriad of versions of everything we need to support
in
Post by Olly Betts
SWIG.
I try to avoid creating single purpose VMs such as this because of the
overhead (both in my time and in bandwidth) of keeping them up to date.
While I would also rather run just one system, there is no way all the SWIG
maintenance can be done using just one operating system. Maybe it is just
me, but I don't think it is very time consuming or difficult to set up a
new VM with the required dependencies for SWIG and once done, there isn't
even a need to update the VM as it'll be in the state required... just a
git pull and build and run tests is required and this can be quietly run in
the background if you have a modern machine with a few CPUs and Gigabytes
of RAM.

William
Olly Betts
2016-12-02 00:34:46 UTC
Permalink
Post by William S Fulton
Post by Olly Betts
Post by William S Fulton
make-xx-test-suite and make-xx-examples etc should invoke swig -xx, where
xx is a target language. This is how the Makefiles and all the testing
works.
That requirement doesn't seem to be documented in Extending.html.
Not everything is documented and is why pull requests and reviews are a
good idea before merging to master. Like a lot of things, It is forced by
the (Travis) build system.
I'm not seeing where "check-php-test-suite" matching "swig -php" is
forced by the Travis build system, or the SWIG build system - the
command line options used for each languages are explicitly specified
in "Examples/Makefile.in".

In fact the change that broke travis is just that one now needs to use
"check-php5-test-suite", etc for PHP5. So we just need to change the travis
stuff to use php5 instead of php for PHP5. I've pushed a commit to do that.

Anyway, I don't think it's at all reasonable to have an explicitly documented
list of requirements for merging a new backend and then magic up more
requirements when someone carefully makes sure they've fulfilled all the
documented requirements. That's just bait and switch. It already hard enough
to get a new backend merged into SWIG, and as a result we have several backends
bitrotting on branches, and this sort of thing isn't going to encourage someone
to pick those up and get them in a fit state to merge.

Cheers,
Olly
William S Fulton
2016-12-04 23:26:01 UTC
Permalink
Hi Olly,
Post by Olly Betts
Post by William S Fulton
Post by Olly Betts
Post by William S Fulton
make-xx-test-suite and make-xx-examples etc should invoke swig -xx,
where
Post by William S Fulton
Post by Olly Betts
Post by William S Fulton
xx is a target language. This is how the Makefiles and all the
testing
Post by William S Fulton
Post by Olly Betts
Post by William S Fulton
works.
That requirement doesn't seem to be documented in Extending.html.
Not everything is documented and is why pull requests and reviews are a
good idea before merging to master. Like a lot of things, It is forced by
the (Travis) build system.
I'm not seeing where "check-php-test-suite" matching "swig -php" is
forced by the Travis build system, or the SWIG build system - the
command line options used for each languages are explicitly specified
in "Examples/Makefile.in".
Most of this is just consistent convention in the Makefiles. What I was
thinking is the SWIGLANG used in the .travis.yml is expected to be the same
as the make targets, so SWIGLANG=php didn't work as it runs
make-php-examples, which invokes php7. But as you mentioned below you
worked around that by switching it to php5.

In fact the change that broke travis is just that one now needs to use
Post by Olly Betts
"check-php5-test-suite", etc for PHP5. So we just need to change the travis
stuff to use php5 instead of php for PHP5. I've pushed a commit to do that.
Anyway, I don't think it's at all reasonable to have an explicitly documented
list of requirements for merging a new backend and then magic up more
requirements when someone carefully makes sure they've fulfilled all the
documented requirements. That's just bait and switch.
I'll document the Travis requirements so there are no undocumented
surprises going forwards. This isn't a tactic to frustrate you or anyone
else, it is good development practice to show new work is up to standard
via CI testing, and one that I've always understood you supported and
enforced. I'm there to help get Travis working too. And to that end, php
7.0 and php 7.1 are now running on Travis. You may want to have a look at
the 7.1 'reference' example on 7.1 as it fails, but otherwise 7.1 looks
good too. Incidentally, php5.6, 7.0, 7.1 seem to co-exist and work fine for
the SWIG tests (on a Trusty box).
Post by Olly Betts
It already hard enough
to get a new backend merged into SWIG, and as a result we have several backends
bitrotting on branches, and this sort of thing isn't going to encourage someone
to pick those up and get them in a fit state to merge.
Adding a new backend into Travis is a trivial task compared to the
development time required to get code into a state that is of sufficient
quality to release, so it shouldn't discourage anyone from collaborating
and developing given there is also help for Travis.

William
Olly Betts
2016-12-05 20:24:25 UTC
Permalink
Post by William S Fulton
I'll document the Travis requirements so there are no undocumented
surprises going forwards. This isn't a tactic to frustrate you or anyone
else, it is good development practice to show new work is up to standard
via CI testing, and one that I've always understood you supported and
enforced.
I try to push for regression tests for fixes and feature tests for new
features. The key thing is we have good test coverage and add regression
tests for fixed bugs wherever possible. Without that the CI is not going
to help at all. Whereas without CI, the tests can still be run by hand to
test a commit before merging it, as we used to do not that long ago (looks
like .travis.yml was created in 2013).

CI testing can certainly be helpful, particularly in being able to test
with more versions of the target language, but the travis setup seems
problematic to me. The integration with the pull request work flow is
about the only good feature really. And that they provide a lot of
infrastructure for free.

But to look a gift horse in the mouth, I don't find it very usable - we
seem to have frequent false failures due to transient issues in their
infrastructure, and unhelpfully these show up in the status as the tests
having failed (not some separate "travis had an issue" status).

To see the log (in order to see if the failure is real or not) you have to
click through to their website, which always seems to be painfully slow.

I don't find the configuration easy to work with either.
Post by William S Fulton
I'm there to help get Travis working too. And to that end, php
7.0 and php 7.1 are now running on Travis. You may want to have a look at
the 7.1 'reference' example on 7.1 as it fails, but otherwise 7.1 looks
good too.
There's nothing useful in the logs - looks like PHP sends errors and
warnings to stdout by default, and RUNPIPE in swig's build system throws
that away by default. I've tweaked things so for PHP7 we send them to
stderr instead. My guess from seeing the warnings from running that one
case under 7.0 is that this change is the cause:

http://php.net/manual/en/migration71.incompatible.php#migration71.incompatible.too-few-arguments-exception

If so, the generated PHP constructor wrapper needs fixing.
Post by William S Fulton
Adding a new backend into Travis is a trivial task compared to the
development time required to get code into a state that is of sufficient
quality to release, so it shouldn't discourage anyone from collaborating
and developing given there is also help for Travis.
The discouraging part is to have got a branch to a stage where you think
it is mergable only to be told "oh but you've only fulfilled the
*documented* requirements, now you need to do all these undocumented ones
too".

The real benefit of CI testing is in the longer term, when patches are
submitted and get tested by it, especially if the backend's maintainer
has gone MIA and maintainers unfamiliar with the language need to decide
if a patch is suitable to merge. So merging the backend and adding CI
testing as a second step doesn't seem a huge problem anyway.

Cheers,
Olly

------------------------------------------------------------------------------
William S Fulton
2016-12-16 09:08:13 UTC
Permalink
Post by Olly Betts
Post by William S Fulton
I'll document the Travis requirements so there are no undocumented
surprises going forwards. This isn't a tactic to frustrate you or anyone
else, it is good development practice to show new work is up to standard
via CI testing, and one that I've always understood you supported and
enforced.
I try to push for regression tests for fixes and feature tests for new
features. The key thing is we have good test coverage and add regression
tests for fixed bugs wherever possible. Without that the CI is not going
to help at all. Whereas without CI, the tests can still be run by hand to
test a commit before merging it, as we used to do not that long ago (looks
like .travis.yml was created in 2013).
CI testing can certainly be helpful, particularly in being able to test
with more versions of the target language, but the travis setup seems
problematic to me. The integration with the pull request work flow is
about the only good feature really. And that they provide a lot of
infrastructure for free.
But to look a gift horse in the mouth, I don't find it very usable - we
seem to have frequent false failures due to transient issues in their
infrastructure, and unhelpfully these show up in the status as the tests
having failed (not some separate "travis had an issue" status).
To see the log (in order to see if the failure is real or not) you have to
click through to their website, which always seems to be painfully slow.
I don't find the configuration easy to work with either.
While these are valid criticisms and it is easy to complain about Travis,
Travis has revolutionized the development and maintenance of SWIG. Of
course everything can be done manually, but it is really slow and painful
and hence hardly ever happened when it was meant to. Before Travis, I spent
most of my time testing, testing, checking new code and likewise fixing
problem weeks after they were introduced which was really inefficient and
wasteful of everyone's time. There is no ways that while I am making SWIG
releases that I am going back to those bad days and hence my insistence
that new backends can be tested on Travis. It is just automating what can
be done by hand, so don't see that it is much extra ask given I am willing
to help get Travis set up.
Post by Olly Betts
I'm there to help get Travis working too. And to that end, php
Post by William S Fulton
7.0 and php 7.1 are now running on Travis. You may want to have a look at
the 7.1 'reference' example on 7.1 as it fails, but otherwise 7.1 looks
good too.
There's nothing useful in the logs - looks like PHP sends errors and
warnings to stdout by default, and RUNPIPE in swig's build system throws
that away by default. I've tweaked things so for PHP7 we send them to
stderr instead. My guess from seeing the warnings from running that one
http://php.net/manual/en/migration71.incompatible.php#
migration71.incompatible.too-few-arguments-exception
If so, the generated PHP constructor wrapper needs fixing.
Post by William S Fulton
Adding a new backend into Travis is a trivial task compared to the
development time required to get code into a state that is of sufficient
quality to release, so it shouldn't discourage anyone from collaborating
and developing given there is also help for Travis.
The discouraging part is to have got a branch to a stage where you think
it is mergable only to be told "oh but you've only fulfilled the
*documented* requirements, now you need to do all these undocumented ones
too".
The real benefit of CI testing is in the longer term, when patches are
submitted and get tested by it, especially if the backend's maintainer
has gone MIA and maintainers unfamiliar with the language need to decide
if a patch is suitable to merge. So merging the backend and adding CI
testing as a second step doesn't seem a huge problem anyway.
Yes, most of this is about the longer term. We added CI for php7 and it
wasn't a problem as you are actively involved. A lot of maintainers will go
missing before the work is complete though and then we mainstream
developers have to pick up the mess. As there are very few active
developers with spare time, I see the priority as keeping the current code
base maintainable and easily managed and this is a higher priority than
adding in new code that can't be shown to work.

Back to php7, I knew there was another reason why I was uncomfortable with
the change of Lib/php being renamed Lib/php5 in a point release. The
subdirectory name is available as a way to override files in the library,
which is clear from the path lookups when running with the -v option:

~/swig/github/swig/Examples/php5/class (master>)$ env
SWIG_LIB=../../../Lib ../../../swig -php -cppext cxx -c++ -v -o
example_wrap.cxx example.i
Language subdirectory: php5
Search paths:
./
./swig_lib/php5/
../../../Lib/php5/
./swig_lib/
../../../Lib/

./swig_lib is also documented as a location to search. Users will have to
change swig_lib/php to swig_lib/php5 now. Ideally this would only happen in
the major release 3.1.0, so it will at a minimum have to be documented as a
potential incompatibility unless you change 3.0.11 back to how it was in
3.0.10. I use this library override quite a bit, but it probably isn't
widely used by others.

William
Olly Betts
2016-12-20 22:07:03 UTC
Permalink
Post by William S Fulton
SWIG_LIB=../../../Lib ../../../swig -php -cppext cxx -c++ -v -o
example_wrap.cxx example.i
Language subdirectory: php5
./
./swig_lib/php5/
../../../Lib/php5/
./swig_lib/
../../../Lib/
./swig_lib is also documented as a location to search. Users will have to
change swig_lib/php to swig_lib/php5 now. Ideally this would only happen in
the major release 3.1.0, so it will at a minimum have to be documented as a
potential incompatibility unless you change 3.0.11 back to how it was in
3.0.10. I use this library override quite a bit, but it probably isn't
widely used by others.
I suspect this quirk isn't something that's widely relied on (I wasn't
aware of/had forgotten about it), and documenting is less work and less
disruptive to the history, so I've pushed that change.

Cheers,
Olly

Olly Betts
2016-12-06 03:48:19 UTC
Permalink
You may want to have a look at the 7.1 'reference' example on 7.1 as it
fails, but otherwise 7.1 looks good too.
Now working; CI config tweaked.

Cheers,
Olly
Continue reading on narkive:
Loading...