Skip to content
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

Monitor PECL's php-solr releases towards standard php 7.2/7.3 support #19

Closed
stronk7 opened this issue Jan 25, 2018 · 17 comments
Closed

Comments

@stronk7
Copy link
Member

stronk7 commented Jan 25, 2018

In #16 we faced the problem of PECL missing a php-solr release supporting PHP 7.2. Current 2.4.0 one does not support it: https://pecl.php.net/package/solr

So, thanks to @scara we proceeded bringing support by manually compiling the extension when building the container.

This is issue is about to monitor new php-solr releases and, whenever one supporting PHP 7.2 is available... switch to it from our custom one.

Ciao :-)

scara added a commit to scara/moodle-php-apache that referenced this issue Jan 25, 2018
Resolves solr issue in moodlehq#16, waiting for the upstream as described in moodlehq#19
scara added a commit to scara/moodle-php-apache that referenced this issue Jan 27, 2018
Resolves solr issue in moodlehq#16, waiting for the upstream as described in moodlehq#19
@caperneoignis
Copy link

I was able to get around this in my version of 7.2 by cloning the repository directly and running the extension install. However, I did run into an issue with Oracle and a missing library SO file.

@stronk7 stronk7 changed the title Monitor PECL's php-solr releases towards standard php 7.2 support Monitor PECL's php-solr releases towards standard php 7.2/7.3 support Jan 18, 2019
stronk7 added a commit to stronk7/moodle-php-apache that referenced this issue Jan 18, 2019
stronk7 added a commit to stronk7/moodle-php-apache that referenced this issue Jan 18, 2019
stronk7 added a commit to stronk7/moodle-php-apache that referenced this issue Jan 18, 2019
This was referenced Jan 19, 2019
stronk7 pushed a commit to stronk7/moodle-php-apache that referenced this issue May 21, 2019
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
stronk7 pushed a commit to stronk7/moodle-php-apache that referenced this issue May 21, 2019
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
stronk7 pushed a commit to stronk7/moodle-php-apache that referenced this issue May 21, 2019
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
@stronk7
Copy link
Member Author

stronk7 commented May 21, 2019

For the records, I've performed a bisect between:

  • 6e9e097c981e810d452657f23bf1945b7955f3cf (2.4.0 release). Good.
  • 744e32915d5989101267ed2c84a407c582dc6f31 (Remi's patch). Bad.

By applying the patch, building the extension and running Moodle's unit tests... and, at the end, git bisect is telling me that:

b1b44e0a22bcce8f625707248d0b4d3630935359 is the first bad commit
commit b1b44e0a22bcce8f625707248d0b4d3630935359
Author: Omar Shaban <omars@php.net>
Date:   Fri Nov 4 13:09:15 2016 +0100

    Fixes Bug #72740: addQueryField return wrong query

:100644 100644 dcedabc742c89cbaba0a2c31dcfc184c4d453666 6e69fdf5f0da89ac2a0149397dd549f5e3ea2352 M	NEWS
:040000 040000 312a3d0f9130c9db0c88f7cc02ef5431dd6c4d1f 3ced7a9ac7dc7161309a0b4212bdf65b676d00d3 M	src

Hope that can bring us some clue about what's happening since that commit. So it broke 4-5 commits before Remi's patch.

Ciao :-)

@stronk7
Copy link
Member Author

stronk7 commented May 21, 2019

(also, have left a comment with our findings @ https://bugs.php.net/bug.php?id=75631 )

@stronk7
Copy link
Member Author

stronk7 commented May 22, 2019

Wow, more yet... if I get upstream master, revert b1b44e0a22bcce8f625707248d0b4d3630935359, and then, without applying any (macports) patch, compile the extension... all our tests are passing!

@glensc
Copy link

glensc commented May 22, 2019

@scara
Copy link
Contributor

scara commented Jul 7, 2019

Official 2.5.0 has been released since 3 days; failure confirmed w/ this PECL release, as expected:

# export MOODLE_DOCKER_WWWROOT=/path/to/moodle-master
# export MOODLE_DOCKER_DB=mysql
# export MOODLE_DOCKER_PHP_VERSION=7.2
# export MOODLE_DOCKER_PHPUNIT_EXTERNAL_SERVICES=true
# bin/moodle-docker-compose up -d && bin/moodle-docker-wait-for-db
# bin/moodle-docker-compose exec webserver php admin/tool/phpunit/cli/init.php
# bin/moodle-docker-compose exec webserver apt-get update
# bin/moodle-docker-compose exec webserver apt-get install -y --no-install-recommends libcurl4-openssl-dev libxml2-dev
# bin/moodle-docker-compose exec webserver pecl install solr
# bin/moodle-docker-compose exec webserver php --ri solr

solr

Solr Support => enabled
Version => 2.5.0
Last Build Date => Jul  7 2019
Last Build Time => 09:09:40
# bin/moodle-docker-compose exec webserver php vendor/bin/phpunit --filter test_search search_solr_engine_testcase search/engine/solr/tests/engine_test.php -v
Moodle 3.8dev (Build: 20190606), f3507273e9f5eb05bb05c24f390d2fc8bf2dcf0a
Php: 7.2.18, mysqli: 5.7.26, OS: Linux 3.10.0-957.21.3.el7.x86_64 x86_64
PHPUnit 7.5.12 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.2.18
Configuration: /var/www/html/phpunit.xml

FF                                                                  2 / 2 (100%)

Time: 2.55 seconds, Memory: 40.00 MB

There were 2 failures:

1) search_solr_engine_testcase::test_search with data set "file-indexing-on" (1)
Failed asserting that actual size 0 matches expected size 2.

/var/www/html/search/engine/solr/tests/engine_test.php:209
/var/www/html/lib/phpunit/classes/advanced_testcase.php:80

To re-run:
 vendor/bin/phpunit -v "search_solr_engine_testcase" search/engine/solr/tests/engine_test.php

2) search_solr_engine_testcase::test_search with data set "file-indexing-off" (0)
Failed asserting that actual size 0 matches expected size 2.

/var/www/html/search/engine/solr/tests/engine_test.php:209
/var/www/html/lib/phpunit/classes/advanced_testcase.php:80

To re-run:
 vendor/bin/phpunit -v "search_solr_engine_testcase" search/engine/solr/tests/engine_test.php

FAILURES!
Tests: 2, Assertions: 2, Failures: 2.

Time to spend some spare time of mine on it, starting on the offending commit found by Eloy - guessing a solr server version dep since its own tests on 5.3 pass.

HTH,
Matteo

@scara
Copy link
Contributor

scara commented Jul 7, 2019

Mmmhhh... using solr server 5.5 doesn't make any difference:

# git diff
diff --git a/phpunit-external-services.yml b/phpunit-external-services.yml
index ca405f0..066b35c 100644
--- a/phpunit-external-services.yml
+++ b/phpunit-external-services.yml
@@ -10,9 +10,8 @@ services:
   redis:
     image: redis:3
   solr:
-    image: solr:6.5
-    entrypoint:
-      - docker-entrypoint.sh
+    image: solr:5.5
+    command:
       - solr-precreate
       - test
   ldap:

# bin/moodle-docker-compose up -d && bin/moodle-docker-wait-for-db
# bin/moodle-docker-compose exec webserver php admin/tool/phpunit/cli/init.php
# bin/moodle-docker-compose exec webserver php --ri solr

solr

Solr Support => enabled
Version => 2.4.0
Last Build Date => May 27 2019
Last Build Time => 00:58:51
# bin/moodle-docker-compose exec webserver php vendor/bin/phpunit --filter test_search search_solr_engine_testcase search/engine/solr/tests/engine_test.php -v
Moodle 3.8dev (Build: 20190606), f3507273e9f5eb05bb05c24f390d2fc8bf2dcf0a
Php: 7.2.18, mysqli: 5.7.26, OS: Linux 3.10.0-957.21.3.el7.x86_64 x86_64
PHPUnit 7.5.12 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.2.18
Configuration: /var/www/html/phpunit.xml

..                                                                  2 / 2 (100%)

Time: 7.32 seconds, Memory: 42.00 MB

OK (2 tests, 28 assertions)
# bin/moodle-docker-compose exec webserver apt-get update
# bin/moodle-docker-compose exec webserver apt-get install -y --no-install-recommends libcurl4-openssl-dev libxml2-dev
# bin/moodle-docker-compose exec webserver pecl install solr
# bin/moodle-docker-compose exec webserver php --ri solr

solr

Solr Support => enabled
Version => 2.5.0
Last Build Date => Jul  7 2019
Last Build Time => 12:57:52
# bin/moodle-docker-compose exec webserver php vendor/bin/phpunit --filter test_search search_solr_engine_testcase search/engine/solr/tests/engine_test.php -v
[...]
FAILURES!
Tests: 2, Assertions: 2, Failures: 2.

Neither using the custom 5.3.1 used for PECL package tests:

[cut]
# git diff
diff --git a/phpunit-external-services.yml b/phpunit-external-services.yml
index ca405f0..7a2a3f3 100644
--- a/phpunit-external-services.yml
+++ b/phpunit-external-services.yml
@@ -10,10 +10,6 @@ services:
   redis:
     image: redis:3
   solr:
-    image: solr:6.5
-    entrypoint:
-      - docker-entrypoint.sh
-      - solr-precreate
-      - test
+    image: omars/solr53
   ldap:
     image: larrycai/openldap
# bin/moodle-docker-compose up -d && bin/moodle-docker-wait-for-db
# bin/moodle-docker-compose exec solr bin/solr create_core -c test

Setup new core instance directory:
/opt/solr/server/solr/test

Creating new core 'test' using command:
http://localhost:8983/solr/admin/cores?action=CREATE&name=test&instanceDir=test

{
  "responseHeader":{
    "status":0,
    "QTime":322},
  "core":"test"}

# bin/moodle-docker-compose exec webserver php admin/tool/phpunit/cli/init.php
# bin/moodle-docker-compose exec webserver php --ri solr | grep -i version
Version => 2.4.0
# bin/moodle-docker-compose exec webserver php vendor/bin/phpunit --filter test_search search_solr_engine_testcase search/engine/solr/tests/engine_test.php -v
[...]
OK (2 tests, 28 assertions)
# bin/moodle-docker-compose exec webserver apt-get update
# bin/moodle-docker-compose exec webserver apt-get install -y --no-install-recommends libcurl4-gnutls-dev libxml2-dev
# bin/moodle-docker-compose exec webserver pecl install solr
# bin/moodle-docker-compose exec webserver php --ri solr | grep -i version
Version => 2.5.0
# bin/moodle-docker-compose exec webserver php vendor/bin/phpunit --filter test_search search_solr_engine_testcase search/engine/solr/tests/engine_test.php -v
[...]
FAILURES!
Tests: 2, Assertions: 2, Failures: 2.

Note: here I've used libcurl4-gnutls-dev since it is the library used by the maintainer to compile the package for testing purposes.

Guessing now a PHP code "issue" in Moodle core which doesn't fall into the solr PECL package test coverage.

Still investigating...

HTH,
Matteo

@scara
Copy link
Contributor

scara commented Jul 7, 2019

Finally found the issue into the PECL solr package 🎉 😄 : scara/pecl-search_engine-solr@c2c94b4.
I'll cleanup the WIP including tests (amending tests\bug_72740.phpt too) and send it to the maintainer; pinging here @remicollet being the kindest package maintainer ever 😉 to alert him about the issue.

Already tested on Moodle Docker Toolbox via:

# git diff /home/matteo/php/solr-extension.sh /home/matteo/php/solr-extension-matteo.sh
diff --git a/home/matteo/php/solr-extension.sh b/home/matteo/php/solr-extension-matteo.sh
index 404b877..a2da5f0 100755
--- a/home/matteo/php/solr-extension.sh
+++ b/home/matteo/php/solr-extension-matteo.sh
@@ -5,31 +5,20 @@ set -e
 # Install 2.4.0 release version + macports patch. Only combination working right now.
 # See #16 and #19 for more information.
 hash=6e9e097c981e810d452657f23bf1945b7955f3cf
-patch=https://raw.githubusercontent.com/macports/macports-ports/ed5c081eee78cbcc0f893cb845b20d9d98b3771c/php/php-solr/files/php72.pa
+hash=c2c94b459ceab41df93cfa8a635f0578aef73a83
+#patch=https://raw.githubusercontent.com/macports/macports-ports/ed5c081eee78cbcc0f893cb845b20d9d98b3771c/php/php-solr/files/php72.p

 # Download our 'tagged' source code from git.
 echo "Downloading solr extension source archive (${hash})"
 curl --location \
-    https://github.com/php/pecl-search_engine-solr/archive/${hash}.tar.gz \
+    https://github.com/scara/pecl-search_engine-solr/archive/${hash}.tar.gz \
     -o /tmp/pecl-search_engine-solr-${hash}.tar.gz

-# Download patch
-if [ -n $patch ]; then
-    curl --location \
-        $patch \
-        -o /tmp/solr.patch
-fi
-
 # Extract the compressed archive.
 cd /tmp
 tar -xvzf pecl-search_engine-solr-${hash}.tar.gz
 cd pecl-search_engine-solr-${hash}

-# Apply the patch
-if [ -n $patch ]; then
-    patch -p0 < ../solr.patch
-fi
-
 # Compile the extension as required by a manual PECL installation.
 echo "Compile solr extension"
 phpize

@stronk7: maybe "we" should mention not to use official PECL 2.5.0 e.g. Wiki and Moodle Community: what do you think?

HTH,
Matteo

@scara
Copy link
Contributor

scara commented Jul 11, 2019

Hi @stronk7,
I've finalized the PR against solr ext and added a note into https://bugs.php.net/bug.php?id=72740#1562881713.

While waiting for the next step on that side, if @dmonllao and you will agree, I'd open a new MDL issue to mitigate such a regression by explicitly adding the default boost factor which avoid any issue even w/ the current 2.5.0.
And then I'd add a PR here to use the official 2.5.0.

Otherwise, we should tell "somewhere" that 2.5.0 is not compatible w/ Moodle core solr engine driver.

Thoughts?

HTH,
Matteo

@dmonllao
Copy link
Contributor

Thanks for looking at this @scara. I assume that there is no harm in boosting all the query fields as none of them would get boosted over the other fields. The alternative (2.5.0 not compatible) would be also ok in my opinion if we would have a 2.5.1 with your fix in but it is not the case yet.

@scara
Copy link
Contributor

scara commented Jul 12, 2019

TNX @dmonllao!
I'm not sure my PR will be merge in a reasonable time.

Moodle PR is available here: MDL-66140.

HTH,
Matteo

@stronk7
Copy link
Member Author

stronk7 commented Aug 28, 2019

Thanks @scara, nice to see that at very least MDL-66140 was integrated recently. Now let's see if php/pecl-search_engine-solr#16 finally lands.

Crossing toes, ciao :-)

stronk7 pushed a commit to stronk7/moodle-php-apache that referenced this issue Sep 12, 2020
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
stronk7 pushed a commit to stronk7/moodle-php-apache that referenced this issue Sep 12, 2020
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)
@stronk7
Copy link
Member Author

stronk7 commented Sep 12, 2020

From #116 , by @scara

Hi @stronk7,
for the record PHP solr extension 2.5.1 has been recently - 2020-09-09! - released, including the patch proposed in php/pecl-search_engine-solr#16 so we could give 2.5.1 a try and finally close #19 🚀 😎 .
It also includes fixes to run w/ PHP 7.4 and PHP 8.0.0beta03.

So, yes, we should give it a try.

@stronk7
Copy link
Member Author

stronk7 commented Sep 12, 2020

Just tried with local php72 image, using the new solr 2.5.1 extension:

php --ri solr

solr

Solr Support => enabled
Version => 2.5.1
Last Build Date => Sep 12 2020
Last Build Time => 17:38:44
TESTSUITE=search_solr_testsuite

Moodle 3.5.14 (Build: 20200914), 598064e4d9958b22e8b8c81016440b1aea48771e
Php: 7.2.33, pgsql: 9.6.18, OS: Linux 4.19.76-linuxkit x86_64
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.2.33
Configuration: /var/www/html/phpunit.xml

..............................                                    30 / 30 (100%)

Time: 20.43 seconds, Memory: 82.25MB

OK (30 tests, 238 assertions)

So, yay, going to prepare the PRs for 72, 73 and 74 (buster).

@stronk7
Copy link
Member Author

stronk7 commented Sep 12, 2020

Done, have created #117 (php72), #118 (php73) and #119 (php74), to move to upstream solr 2.5.1.
(also have applied the same change to the ongoing php80 branch, so we have it there already)

Once they are merged and running over a few days... I think we'll can close this once and forever.

Ciao :-)

@stronk7
Copy link
Member Author

stronk7 commented Sep 18, 2020

Yesterday we rolled changes and have been running ok over the last 24h... will close this in a couple of days to allow all jobs and combos to be run.

@stronk7
Copy link
Member Author

stronk7 commented Sep 24, 2020

Yay! One less thing™, thanks @scara for all the support!

@stronk7 stronk7 closed this as completed Sep 24, 2020
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

No branches or pull requests

5 participants