Skip to content

Conversation

grooverdan
Copy link
Contributor

In testing how well the libmysqlclient performs in mysqli and the pdo_mysql I've made the following changes to get them running on travis.

There are about 33 test failures in the mysqli/pdo_mysql test suites associated with these so I've left the as travis 'allow_failure" jobs for now. Marking these as XFAIL was going to be detrimental to the mysqlnd coverage.

I've included a bump from trusty to bionic. As I was looking at running docker container version of the mysql major versions on the server side. With the existing failures, and bionic running a 5.7 MySQL version, I thought I'd wait off adding more complexity.

A few other minor tests needed small corrections to run on bionic/the travis environment.

libmysqlclient_r was replaces with libmysqlclient consistent with Oracle upstream. The libmysqlclient_r symlink is only in mysql-5.6, the currently earliest support upstream version.

$ find /usr/local/m* -name libmysqlclient\* -ls
 12194632   8856 -rwxr-xr-x   1 7161     31415     9065576 Jun  2 16:15 /usr/local/mysql-5.6.49-linux-glibc2.12-x86_64/lib/libmysqlclient.so.18.1.0
 12193009      0 lrwxrwxrwx   1 7161     31415          20 Jun  2 16:17 /usr/local/mysql-5.6.49-linux-glibc2.12-x86_64/lib/libmysqlclient_r.so.18 -> libmysqlclient.so.18
 12193010      0 lrwxrwxrwx   1 7161     31415          24 Jun  2 16:17 /usr/local/mysql-5.6.49-linux-glibc2.12-x86_64/lib/libmysqlclient_r.so.18.1.0 -> libmysqlclient.so.18.1.0
 12193006      0 lrwxrwxrwx   1 7161     31415          24 Jun  2 16:17 /usr/local/mysql-5.6.49-linux-glibc2.12-x86_64/lib/libmysqlclient.so.18 -> libmysqlclient.so.18.1.0
 12194631  16880 -rw-r--r--   1 7161     31415    17284614 Jun  2 16:15 /usr/local/mysql-5.6.49-linux-glibc2.12-x86_64/lib/libmysqlclient.a
 12193007      0 lrwxrwxrwx   1 7161     31415          16 Jun  2 16:17 /usr/local/mysql-5.6.49-linux-glibc2.12-x86_64/lib/libmysqlclient_r.a -> libmysqlclient.a
 12193008      0 lrwxrwxrwx   1 7161     31415          17 Jun  2 16:17 /usr/local/mysql-5.6.49-linux-glibc2.12-x86_64/lib/libmysqlclient_r.so -> libmysqlclient.so
 12193005      0 lrwxrwxrwx   1 7161     31415          20 Jun  2 16:17 /usr/local/mysql-5.6.49-linux-glibc2.12-x86_64/lib/libmysqlclient.so -> libmysqlclient.so.18
 11803348   9224 -rwxr-xr-x   1 7161     31415     9445360 Jun  2 22:55 /usr/local/mysql-5.7.31-linux-glibc2.12-x86_64/lib/libmysqlclient.so.20.3.18
 11803257      0 lrwxrwxrwx   1 7161     31415          25 Jun  2 23:08 /usr/local/mysql-5.7.31-linux-glibc2.12-x86_64/lib/libmysqlclient.so.20 -> libmysqlclient.so.20.3.18
 11803347  18548 -rw-r--r--   1 7161     31415    18991104 Jun  2 22:55 /usr/local/mysql-5.7.31-linux-glibc2.12-x86_64/lib/libmysqlclient.a
 11803256      0 lrwxrwxrwx   1 7161     31415          20 Jun  2 23:08 /usr/local/mysql-5.7.31-linux-glibc2.12-x86_64/lib/libmysqlclient.so -> libmysqlclient.so.20
 10879166  21428 -rwxr-xr-x   1 7161     31415    21942240 Jun 17 07:01 /usr/local/mysql-8.0.21-linux-glibc2.12-x86_64/lib/libmysqlclient.so.21.1.21
 10879162      0 lrwxrwxrwx   1 7161     31415          25 Jun 17 07:53 /usr/local/mysql-8.0.21-linux-glibc2.12-x86_64/lib/libmysqlclient.so.21 -> libmysqlclient.so.21.1.21
 10879154  56568 -rw-r--r--   1 7161     31415    57922986 Jun 17 07:01 /usr/local/mysql-8.0.21-linux-glibc2.12-x86_64/lib/libmysqlclient.a
 10879165      0 lrwxrwxrwx   1 7161     31415          20 Jun 17 07:53 /usr/local/mysql-8.0.21-linux-glibc2.12-x86_64/lib/libmysqlclient.so -> libmysqlclient.so.21

@grooverdan grooverdan force-pushed the travis-mysqli-test-improvements branch from 281d682 to 82f1792 Compare September 15, 2020 06:42
@grooverdan
Copy link
Contributor Author

In looking forward I see that 7.4 has moved to azure and a CI pipeline.

I tried to get this back to a simpler trusty based build however failed.

I've pulled the test cases from the master branch just to get maximum test successes. Combining this with #6127 results in https://travis-ci.org/github/grooverdan/php-src/builds/727264652, all supported mysql latest versions being able to be compiled and passing most tests.

@nikic
Copy link
Member

nikic commented Sep 15, 2020

Thanks for working on this! As the libmysqlclient backend is not typically used, nobody has really put effort into maintaining it in recent times.

As you have already found out, PHP 7.4 upwards uses a completely different CI setup running on Azure Pipelines. We try to avoid making changes to the CI setup on PHP 7.3 for that reason, as higher branches will need a different implementation and 7.3 only has a couple months of active support left at this point.

The second problem here is that we don't have the CI capacity to run multiple mysql drivers on each build (this would add three additional jobs to each commit and PR build). The way we usually handle this is to create a separate job for this on Azure Pipelines that only runs for scheduled nightly builds. In this case, the job would just repeatedly build PHP against different libraries and run the relevant parts of the tests (ext/mysqli, ext/pdo_mysql).

Another thing that will turn up as a problem when targeting 7.4 upwards is that builds use -Werror and as such need to be warning clean. I remember that this was an issue the last time I tried to build PHP against libmysqlclient.

@grooverdan
Copy link
Contributor Author

I started looking at this because someone reported a fault that could have only happened with libmysqlclient. Given the feature difference listed https://www.php.net/manual/en/mysqlinfo.library.choosing.php I'm still don't understand why it was chosen (which I'm sure you'd find interesting too).

Given the current state of not compiling against mysql-8.0 I certainly see its less maintained which was easy enough to fix (#6127). If nothing else this was a useful exercise in proving what works/doesn't work.

On CI resource minimization I was looking at something like the following as the criteria to extend(/switch from) mysqlnd in the build.

for file in $(git diff --name-only $TRAVIS_COMMIT_RANGE)
do
        case "$file" in
        ext/mysqli/*)
                mysqli=1
                mysqlpdo=1 
                ;;
        ext/pdo_mysql/*)
                mysqlpdo=1 
                ;;
        esac
done

This doesn't address that even with this change, even on azure, you're only testing against 1 mysql version (and one postgres version too). I fully understand the CI resource contraints and the labour effort that goes into them at MariaDB.

On -Werror;

  • mysqli native driver - enable mysql-8.0 #6127 solves #define PACKAGE_XXX redefined (and enables MySQL-8.0 to be compiled)
  • bunch, probably a single location source, of string format %ul vs %ull (that goes away in 8.0 - its a ABI break, but does fix some internal mysqlclient warnings)
  • one signed/unsigned fault that easy enough to fix
  • bunch of unused variables
  • and a few ptr comparison to 0 always true.

Overall not too hard.

Tougher is the 33-46 test failures including memory leaks. I suspect some will be rather simple server configuration or assuming mysqlnd type conversion.

So my deeper, possibly blunt question but not in an intentionally offensive way, is the libmysqlclient really supported?

@Girgias
Copy link
Member

Girgias commented Sep 16, 2020

libmysql doesn't have a maintainer (like a lot of bundled extensions tbh) and there was an email thread last year (see: https://externals.io/message/106086) about dropping support but nothing got enacted.

Worst case about the tests which leak is that you mark them as XFAIL (see: https://qa.php.net/phpt_details.php#xfail_section) just so that it passes CI

@nikic
Copy link
Member

nikic commented Sep 17, 2020

So my deeper, possibly blunt question but not in an intentionally offensive way, is the libmysqlclient really supported?

That's a good question :) I think the current state of libmysqlclient support is basically "the code is there" and will get occasionally fixed to continue working. I'd be happy to move it towards being more actively maintained though, as I don't like the idea of dropping support altogether.

Mysqlnd might not support something that libmysqlclient does (especially if it's new), and this offers an escape hatch. I also used the support just yesterday to compare wireshark traces between libmysqlclient and mysqlnd to track down a bug, though I guess that's one of the more exotic use-cases :)

I've fixed the build warnings against the 8.0 client in b0661a9 and a small number of tests in 86e0027 just to get a feeling for how things stand now. Many of the test failures are fairly superficial, but there are definitely some genuine issues in there. In particular

stmt->result.buf[ofs].buflen =
reads a very large length, leading to out of memory conditions.

@nikic
Copy link
Member

nikic commented Sep 17, 2020

And then there is this gem: c3944c4 Mysqlnd implementation was broken, and the test expected the wrong result...

@grooverdan
Copy link
Contributor Author

Thanks @nikic for the test/warning fixed and showing how its still useful.

closing this as a pr against travis. Its usefulness in testing was as good as the discussion generated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants