Revive PHP 7 connector #153
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ignore configure.ac and config.h.in~ generated files. Ignore Vim swap files for the case, when a file is opened several times.
Temporary disabled php 7.1+ and RPM/Deb packages to get working tests on php 7.0 first. Added testing against different tarantool versions. Temporary disabled tarantool-2.2 testing due to lack of support of the new schema format (see #151).
tarantool-3.ini configuration fails and it seems the reason is gh-113 ('Receiving trash from persistent connect when there was timeout before'). Disabled both tarantool-2.ini and tarantool-3.ini configurations, where persistent connections are used, until the problem will be resolved. Related to #113.
Recent pyyaml versions start to warn about using yaml.load(), which is considered unsafe for an arbitrary input (see CVE-2017-18342). Our input is controllable (because we wrote tests ourself), but anyway the warning may be annoying. The main reason for me, however, is that using of yaml.load() is disabled for dev-python/pyyaml package on Gentoo. See [1] for more information. We did the similar changed for tarantool-python, test-run and tarantool tests, see [2], [3], [4], [5]. [1]: https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=79ba924d94cb0cf8559565178414c2a1d687b90c [2]: tarantool/tarantool-python@350771d [3]: tarantool/test-run@38400e9 [4]: tarantool/test-run@89808d6 [5]: tarantool/tarantool@d5fdc53
The compatibility layer for different tarantool versions was broken in several ways: * It did use 'string.split', which was added only in tarantool-1.7.3-666-ga6800388c. * It did not support 2.* versions and uses old options on, say, tarantool-2.4.0-82-gd8a9f1d9c (because assumes the major version as 1 and didn't check it). * 'slab_alloc_arena' value was passed in bytes rather than gigabytes.
An order in which getaddrinfo() returns addresses to try to connect may vary from system to system. In php:7.0-cli based docker container 127.0.0.1 address is tried first, then ::1. The attempt to connect the IPv4 address gives errno=111 (ECONNREFUSED, 'Connection refused') and it would be correctly matched by the test. An attempt to connect the IPv6 address gives errno=99 (EADDRNOTAVAIL, 'Cannot assign requested address'), which is reported in the exception message and considered as the failure by the test case. IPv4 is likely supported everywhere, so restrict the attempts to the IPv4 address.
Under certain network conditions it is possible that getaddrinfo() will return EAI_AGAIN rather than EAI_NONAME for an unknown host. It can be easily reproduced when a network interface is down, however it is not the only case when EAI_AGAIN appears. It seems some DNS servers may report a temporary failure in case of an unknown hostname (at least it looks so, I am unable re-check now).
Provide the script, which run the whole test suite for the connector on all supported tarantool and php versions. It is useful to verify changes locally. The script uses php:<version>-cli docker images and the ./test.sh script inside (exactly what is called on Travis-CI). The ./test.sh script is modified to work under those images: * install necessary packages (gnupg2, lsb-release, apt-transport-https), when when they're not installed * support tarantool repositories for Debian (not only Ubuntu) * don't require 'sudo' when the current user is root (usual for docker) * don't require 'pip', install python-yaml from packages (pip is not available by default in php's images) * while we're here, changed GPG key and repository URLs from http:// to https:// (it is redirected to https:// anyway) * fix tarantool-1.6 installation on Debian Stretch Downside: APT repositories updating takes significant time and it is performed for each configuration.
This was referenced Mar 26, 2020
Closed
rybakit
reviewed
Mar 26, 2020
rybakit
reviewed
Mar 26, 2020
rybakit
reviewed
Mar 26, 2020
rybakit
reviewed
Mar 26, 2020
rybakit
reviewed
Mar 26, 2020
rybakit
reviewed
Mar 26, 2020
rybakit
reviewed
Mar 26, 2020
rybakit
reviewed
Mar 26, 2020
rybakit
reviewed
Mar 26, 2020
The proper initialization is important for correct work of an inherited class. I cannot provide exact description what was broken and becomes right, but at least the test case with inherited class with a property starts to work correctly. Some online materials strongly suggest to call object_properties_init() function in a create_object handler, see [1], [2], [3]. The problem was catched at the existing test suite on mocking tests when phpunit was updated to 6.5.14 (now it is 4.8.24) and appropriate changes were made for the tests (say, using of createMock() instead of getMock()). So this commit is prerequisite to run the test suite on more fresh phpunit, which is necessary to test the connector on php-7.1+. The changes for php-7.1+ will land in future commits. It seems the problem was introduced in [4]. [1]: https://wiki.php.net/internals/engine/objects [2]: http://www.phpinternalsbook.com/php5/classes_objects/custom_object_storage.html [3]: https://phabricator.wikimedia.org/T59292 [4]: 9f5a282 ('updated PHP7 implementation') Fixes #135.
It is not supported by recent phpunit versions (see [1]) and is not actually used in our testing process. So it becomes just dead code and it worth to remove it. We'll bring newer phpunit versions in further commits to test the connector against more recent PHP versions (see [2]). [1]: sebastianbergmann/phpunit#2683 [2]: https://phpunit.de/supported-versions.html
We should use different phpunit versions on different php versions (see [1]). Since we going to enable php from 7.0 to 7.4, the logic of test-run.py is changed to find `phpunit` command in PATH directories, but the Travis-CI script (test.sh) installs appropriate phpunit version to /usr/local/bin. Note: On the first glance it looks that we can skip phpunit installing on Travis-CI, because it is provided already. However it ships phpunit-7.5.0 for php-7.0 environment, but it works only on php-7.1+. Note: We can not just save downloaded phpunit into /usr/local/bin, because Travis CI have a directory with its own phpunit executable in PATH prior standard directories. So we create our own directory and add it to PATH at beginning. Tests are changed to be compatible with phpunit-6+, see [2] and [3]. Aside of this, removed some dead code from test-run.py. The next commit will remove test/phpunit.phar file, it is not used anymore. [1]: https://phpunit.de/supported-versions.html [2]: https://thephp.cc/news/2017/02/migrating-to-phpunit-6 [3]: sebastianbergmann/phpunit#2898
This file is not used anymore. Proper phpunit version is installed in the Travis-CI script (test.sh) depending of current php version. See the previous commit for details.
Added the compatibility layer for phpunit-6 and phpunit-7. The problem, which is solved here, is that PHP 7.0 does not support `void` return value declaration, but phpunit-7 (which supports php-7.[1-3]) requires it for some methods (see [1]). The compatibility layer is necessary to run the same testing code on different php/phpunit versions. The implementation idea is borrowed from Symfony's [PHPUnit Bridge][2]. There is also broad discussion about this problem in the Drupal's [issue 3063887][3]. In fact the test suite works on phpunit-8 too, but phpunit produces several warnings about using of deprecated functionality. It will be fixed in the following commits. [1]: https://phpunit.de/announcements/phpunit-7.html [2]: https://symfony.com/doc/current/components/phpunit_bridge.html#removing-the-void-return-type [3]: https://www.drupal.org/project/drupal/issues/3063887
We use a maximal phpunit version, which supports a certain php version. So phpunit-7 is in action for PHP 7.1. The previous commit enables support of phpunit-7 for the test suite, so we ready to enable PHP 7.1 in testing.
It seems the exception "No field '<...>' defined" was not raised since introducing of named fields support in 31cf8a8 ('Next version of tarantool-php for php7'). There is the test for this case (DMLTest::test_08_update_error_nosuchfield), but it accepts any error message, because of a typo in an annotation name. Fixed this typo. The problem was found by this test when the annotation was replaced by a call in order to eliminate a phpunit-8 warning and to support phpunit-9. This change will land in a next commit.
After enabling phpunit-7, phpunit-8 starts to work too, but gives several deprecation warnings: * Using of `assertContains()` for search in a string. * Using of `@expectedException` and related annotations. Functionality that is deprecated in phpunit-8 will be disabled in phpunit-9. Since we plan to use phpunit-9 for testing on php-7.3 and php-7.4, it worth to fix the warnings now. This commit updates the test suite to use the new constructions and provides the compatibility layer for phpunit-6, which does not contain `assertStringContainsString()` method. The commit also disables caching of testing results, which was enabled by default in phpunit-8 (see [1]). The test suite is quite small and the feature does not add any value. The reason for disabling is to protect ourself from side effects of this caching: to be honest I don't know how it may change testing results. [1]: https://phpunit.de/announcements/phpunit-8.html
Previous changes enable support of phpunit-8 and eliminate its deprecation warnings. Now we ready to enable PHP 7.2 testing using the maximal phpunit version, which supports it: it is phpunit-8. The commit enables testing of PHP 7.2 in Travis CI and in the script for local testing (test.all.sh).
See [1] for the list of changes of the PHP internal API. The following items are relevant here: c. Array/Object recursion protection e. AST and IS_CONSTANT f. GC_REFCOUNT() [1]: https://github.com/php/php-src/blob/PHP-7.3.16/UPGRADING.INTERNALS Fixes #139. @Totktonada: style and naming fixes.
Updated the PhpUnitCompat compatibility layer to support both expectExceptionMessageMatches() method on phpunit-6 and phpunit-7/ See the phpunit-9 announcement [1], where expectExceptionMessageRegExp() method removal was announced. We're going to use phpunit-9 for testing the connector on PHP 7.3 and PHP 7.4. [1]: https://phpunit.de/announcements/phpunit-9.html
In previous commits we updated the connector to be compatible with the internal API of PHP 7.3 and updated tests to support phpunit-9. So now we ready to enable PHP 7.3 in CI.
Just to make the build look clean.
After fixes for PHP 7.3 everything also works for PHP 7.4 (see #150). We just need to enable PHP 7.4 in CI.
Python 2 going to its real EOL and it is often easier to get Python 3 on a certain system rather than Python 2. The test runner layer, which is written in Python is quite thin, so it worth re-write it to support both Python 2 and Python 3.
Totktonada
force-pushed
the
Totktonada/revive-the-connector
branch
from
068c2c0
to
70e89cd
Compare
|
Applied the fixes suggested by @rybakit. Three new commits added:
|
Verify `packpack` based builds for CentOS and Fedora distributions using Travis CI. Deployment is not enabled here: it is subject for further commits. The key point is that we'll build the extension for php version that is provided by an OS itself. Why don't build against PHP versions from widely used Remi's repository? There are two reasons: * It will introduce dependency on a third-party repository for our third-party repository, at least in context of php-tarantool RPM packages. I guess it would quite unobvious from a user perspective. * We don't control when PHP ABI version will be changed in Remi's repositories. Is there a politic about this? I don't aware of it. We can get non-working packages after an update of PHP version in Remi's repo. So the decision for now is to provide packages for a PHP version provided by a distribution itself (from its base repositories). I plan to contact Remi and consult how to handle everything in a way that would be convenient for a user. Maybe it would be good to provide sources, spec and build tools from our side and ask Remi to include the extension into his repositories. Part of #117
Provided the script to build a RPM / Deb package, install it inside a docker container based on an appropriate image and verify that the main class of the extension (Tarantool) is available from the PHP interpreter. The script builds a package just like it is performed in CI: using packpack. It should be available in PATH directories. Only RPM packages are here now, but the following commits will enable Deb packages too. Part of #117
I guess it is the behaviour, which is expected by a user: when an extension is installed it should be available for use. Part of #117
Verify Deb packages on distributions, which provides PHP 7.0: it is Debian Stretch and Ubuntu Xenial. Now the Debian build files (debian/ directory) allow to build against PHP 7.0 only (not PHP 7.1+), because of hardcoded package name and installation paths. That is why newer Debian and Ubuntu distributions are not enabled here. It will be fixes in the following commits. Part of #117
Preprocess debian/control file to set proper package name like php7.3-tarantool. Use the package name in debian/rules file to determine installation paths. Removed php7.0-dev dependency, because php-all-dev already depends on proper php7.?-dev package. This change allows to build a Deb package for distributions, which provides PHP version other then 7.0. Those builds will be enabled in CI in further commits. Details ------- It seems it is not possible to build a package with a conditional name (depending of a distribution). `man 5 deb-substvars` explicitly states that 'Package' field in debian/control can be parametrized using a substitution varible. Another possible way would be define several 'Package' fields in debian/control and build only one for each distribution. However I don't find a way to build only one package using `debuild` (it is used inside `packpack`). So it seems that the preprocessing is the only way to build, say, php7.0-tarantool package for Debian Stretch and php7.3-tarantool for Debian Buster. The preprocessing is performed from debian/prebuild.sh script, which is invoked by `packpack`. So nothing changed in the build process from a caller perspective. Part of #117
The distributions which provides PHP 7.1+ were blocked, because 7.0 (the PHP version) was hardcoded in package name and installation paths. Now the problem is resolved and we can verify those packages in CI (and in the local verification script test.pkg.all.sh). Part of #117
There was no consistency in using spaces and tabs across test files. Now all tests are indented using four spaces. Aside of this, fixed places, where double indentation was used. It was annoying to switch a text editor between different indentation modes.
When PHP strict types mode is enabled (`declare(strict_types=1);`) the `offset` parameter passed as `null` to select() method does not pass validation. It looks quite natural to accept `null` here, because when the parameter is omitted the default value (zero) is assumed. A user may want to set the next parameter (`iterator`) and left `offset` to be default, so (s)he'll use `null`. The similar change was made for the `limit` in the scope of #58. After this commit `offset` and `limit` parameters behave in the same way. The functionality is already covered by the test suite (DMLTest::test_14_select_limit_defaults()), but the problem was not catched until now, because strict types mode is not enabled in tests. It'll be enabled in a following commit and the problem will be actually tested. Fixes #154.
It is the rule of thumb to run tests in the strict types mode, because it may reveal subtle problems. See #154 for example. Updated the test suite to cast tarantool port (acquired from PRIMARY_PORT environment variable) from string to int. Aside of this, raise an error when the port is not set: it'll allow to fail fast and provide a good diagnostic when a configuration problem occurs.
Totktonada
force-pushed
the
Totktonada/revive-the-connector
branch
from
70e89cd
to
f85d359
Compare
|
Force-pushed: a bit polish commit messages. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contributors: @Totktonada (this PR), @bigbes (PR #140), @SonSergei (PR #148), @rybakit (suggestions). Thank you all!
Supersedes PR #140.
Supersedes PR #148.
Fixes #139.
Fixes #135.
Fixes #154.
Follow up activities:
mastersince forking ofphp7-v2branch.php7-v2tomaster(Move php7-v2 to master #137).