-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prepare the 'php72' branch #16
Comments
Note: PHP 7.2 (now still rc, |
Latest update is:
When that lands, I'll prepare the 72 branch. |
Ref: solr & php72: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=224078 |
With solr disabled because of the link above... I've ended with this one, all databases working ok so far (have run unit tests in mysql, pgsql, mariadb, oracle and sqlsrv). Surely not final... but hope it will save you some time (finding libraries and other details). In fact it has helped to find https://tracker.moodle.org/browse/MDL-61240. Just running a few more tests... |
TODO: Can switch to newer instaclient 12.2... (running 12.1.. now). But don't want to get distracted... |
With sqlsrv latest 5.2-RC1 drivers and MSODBC 17 preview it seems that it's possible to make the extension to work under Debian 9. it requires to install MSODBC 13, uninstall it and then install MSODBC 17, but seems to be working ok. oci also requires some little changes and, finally, what I've been unable to fix/make work is solr. it seems that still is not ready, so I've disabled it for now. This will help fixing moodlehq#16.
With sqlsrv latest 5.2-RC1 drivers and MSODBC 17 preview it seems that it's possible to make the extension to work under Debian 9. it requires to install MSODBC 13, uninstall it and then install MSODBC 17, but seems to be working ok. oci also requires some little changes and, finally, what I've been unable to fix/make work is solr. it seems that still is not ready, so I've disabled it for now. This will help fixing moodlehq#16.
With sqlsrv latest 5.2-RC1 drivers and MSODBC 17 preview it seems that it's possible to make the extension to work under Debian 9. it requires to install MSODBC 13, uninstall it and then install MSODBC 17, but seems to be working ok. oci also requires some little changes and, finally, what I've been unable to fix/make work is solr. it seems that still is not ready, so I've disabled it for now. This will help fixing moodlehq#16.
With sqlsrv latest 5.2-RC1 drivers and MSODBC 17 preview it seems that it's possible to make the extension to work under Debian 9. it requires to install MSODBC 13, uninstall it and then install MSODBC 17, but seems to be working ok. oci also requires some little changes, have been updated to 12.2 libraries and is using proper ENV for both interactive and non-interactive sessions. and, finally, what I've been unable to fix/make work is solr. it seems that still is not ready, so I've disabled it for now. This will help fixing moodlehq#16.
(and got it passing after updating oci to 12.2 and setting library path properly) |
Great! If this will be merged:
TIA, |
In this repo (in all in general), I'm against using the 😱 "latest" tag (de facto "default" one). So I'd keep current behavior of having to, always, specify a tag.
+💯 |
That's strange: Remi repo for CentOS has that extension available since months, https://rpms.remirepo.net/enterprise/7/php72/x86_64/repoview/php-pecl-solr2.html. I found just a mention about Stretch long time ago, oerdnj/deb.sury.org#664 (comment): what about dropping a new issue there to know where we actually are in Debian 9 for solr and PHP 7.2? Edit: it is there as a package, https://packages.sury.org/php/dists/stretch/main/binary-amd64/Packages:
Nothing has been filed into https://github.com/docker-library/php - found the fix by Remi: https://git.php.net/?p=pecl/search_engine/solr.git;a=commitdiff;h=744e32915d5989101267ed2c84a407c582dc6f31 (php/pecl-search_engine-solr@744e329) - and if I give it a try on an plain 7.2-apache container instance it breaks on something that looks like already fixed by Remi but not in the main repo, https://bugs.php.net/bug.php?id=75631. Edit^2: added a proposal in stronk7@aaf7d05#r27038188, to enable solr and then track the availability of a new official pecl version in a separate issue, here. HTH, |
Great findings, Matteo! As commented @ the commit... 100% welcome a new .sh for adding manually compiled solr till there is a pecl one with the needed fixes. +1 ! Tell me where to pick it from (or if you want me to try it), let's do some tests and, if everything is working... surely we can convert this into a proper PR a let it advance. Thanks, ciao :-) |
(have created #19 about to monitor the availability of a proper php-solr extension for PHP 7.2) |
Resolves solr issue in moodlehq#16, waiting for the upstream as described in moodlehq#19
Hi @stronk7, Edit, here is the result quite interesting since we need to disable
HTH, |
Pinging @dmonllao, to kindly ask for a check about the above errors being an issue in compiling Guessing the first i.e. my commit should not be added: @stronk7 please do not just install the HTH, |
Hi @scara, I will look at it with great pleasure. I am guessing that it is because of http://php.net/manual/en/migration72.incompatible.php#migration72.incompatible.warn-on-non-countable-types and response->docs not being countable. I am verifying its type now. |
Confirmed that they are caused by a problem in moodle code: https://tracker.moodle.org/browse/MDL-61281 (up for peer review) |
Hi @dmonllao, Will run the tests again this weekend and comment in the Tracker. |
More details in MDL-60978 Fixes redis unit tests failures in moodlehq#16
Resolves solr issue in moodlehq#16, waiting for the upstream as described in moodlehq#19
Hi @stronk7 and @dmonllao,
Here is the result of the run:
No more errors from PHP 7.2 and At the end:
TIA, |
@dmonllao for the record, quickly debugging that test case, selecting just one failed test: bin/moodle-docker-compose exec webserver php vendor/bin/phpunit --filter test_search search_solr_engine_testcase search/engine/solr/tests/engine_test.php show me that the response coming from the search engine is actually empty i.e. w/ no matching.
HTH, |
Hi Matteo, I didn't compile it myself. search_solr php unit runs with MDL-61281 patch in are passing for me. `➜ master git:(MDL-61281_master) ✗ vendor/bin/phpunit -c search ............................................................... 63 / 63 (100%) Time: 52.74 seconds, Memory: 82.50MB OK (63 tests, 504 assertions) |
TNX for your superfast feedback 👍. I already filed an issue into Docker PHP about solr extension and its outdated v2.4.0 (docker-library/php#571). |
Well, this is what I've got (1st attempt!):
And what I did (manually for now, within the 7.3 container, committing it, was (exactly what macports does):
And moodle tests passed pretty well! I must confess that I also ran solr tests (make test) and got a number of failures... but haven't looked if they were because cannot access to any solr instance or what. So the question here is... no matter it's using an old codebase and patch, and some own extension tests are failing... as far as it makes moodle happy (and really works, I've it enabled in my dev sites and use it a number of times)... should we go for it (until the problem is fixed upstream and we process it at #19 ) ? I feel myself inclined to go for it, and have it working ASAP. Later we can look if there are newer combinations also working but, for now, I'd consider this (imperfect but) good enough. Thoughts? |
Just have seen this, from April: http://git.php.net/?p=pecl/search_engine/solr.git;a=commitdiff;h=98a8bf540bcb4e9b2e1378cce2f3a9bf6cd772b8 Going to give it a run... |
Hi @stronk7,
+1 for me 👍 For the record: the patch you used matches php/pecl-search_engine-solr@744e329 which is about ten commits before the commit I used for my first tests... I still wonder why I had those failures given the strange HTTP Response pattern.
They are both unit and integration tests.
🤞 😉 Matteo P.S.: time for me to buy a Mac? Looking for a sponsor 😆 |
Just to confirm that using current master (php/pecl-search_engine-solr@98a8bf5), it's a total disaster:
Not sure how it can be so bad, it's crazy! |
Ok, so what if:
Don't think it's worth the effort looking at the details when there is at very least one combination working. I really hate mysteries, because they usually hide some interesting truth to learn from (maybe we are doing something wrong in our engine implementation?), but right now I'd prioritize to have them working. Then investigations can continue (here or @ #19). I'm going to prepare a patch for 72 and 73 (stretch only). Will link once available... feel free to comment anything...ciao :-) |
Yeah, I saw that there were request failures and friends and imagined that I was missing something like that (a server instance). But was so impatient to verify if the combination was going to work that didn't look further, heh. And yes, as said above, agree with you... it doesn't make much sense that they have broken it so badly... so maybe there is some config/setting on connection or whatever that we are missing and cause all the surprising failures... so we should keep investigating, yeah. |
TNX @stronk7 !
Great: you take my point 😉! Matteo |
BTW, I just tried a bisect session here, starting from php/pecl-search_engine-solr@744e329 just to discover that the upstream patch doesn't lead to a working (for Moodle) version, grrr. So whatever killed it, was between 2.4.0 and that patch. Maybe later will continue the bisect section to see where the hell things became broken (maybe that will give us a clue of the reasons). Back to the 2.4.0 + patch preparations... |
Using 2.4.0 release + custom patch (from macports project) seems to be the only working combination right now. Read the comments in the code and the links there. Resolves solr issue in moodlehq#16, waiting for the upstream as described in moodlehq#19
Using 2.4.0 release + custom patch (from macports project) seems to be the only working combination right now. Read the comments in the code and the links there. Resolves solr issue in moodlehq#16, waiting for the upstream as described in moodlehq#19
Using 2.4.0 release + custom patch (from macports project) seems to be the only working combination right now. Read the comments in the code and the links there. Resolves solr issue in moodlehq#16, waiting for the upstream as described in moodlehq#19
Ok, have created #57 (php73) and #58 (php72) to get solr working there. And, in #19 we'll do any next research about it, where did it became broken and so on. So, right now, in order to have the images 100% ok... we are still missing:
Tomorrow more, have got enough of docker for today :-) |
With moodlehq/moodle-php-apache#16 adding support to mongodb tests back for PHP 7.1, 7.1 and 7.3 this enables them to be executed from moodle-docker. Also, unrelated, README has been updated to show the 7.2 and 7.3 versions as available.
Ok, created #60 , #61 , #62 and moodlehq/moodle-docker#102. With them we can consider this 100% achieved but #9 (analytics python backend) that can be handled apart. Once everything is in place... will run some tests to verify that there aren't other leftovers... Ciao :-) |
With moodlehq/moodle-php-apache#16 adding support to mongodb tests back for PHP 7.1, 7.1 and 7.3 this enables them to be executed from moodle-docker. Also, unrelated, README has been updated to show the 7.2 and 7.3 versions as available.
+1 |
Sold, thanks all! Let's continue in related issues! |
Using 2.4.0 release + custom patch (from macports project) seems to be the only working combination right now. Read the comments in the code and the links there. Resolves solr issue in moodlehq#16, waiting for the upstream as described in moodlehq#19
Using 2.4.0 release + custom patch (from macports project) seems to be the only working combination right now. Read the comments in the code and the links there. Resolves solr issue in moodlehq#16, waiting for the upstream as described in moodlehq#19 This is basically a backport of moodlehq#58 that was applied to php72 and up. The problem with php71 was detected @ moodlehq/moodle-docker#134 (comment)
PHP 7.2 is now RC5: time to prepare a branch/image for it.
Will look for the extensions required here, spare time permitted.
The text was updated successfully, but these errors were encountered: