Page MenuHomePhabricator

mediawiki/vendor REL1_* no longer ship dependencies for wmf extensions that are not in the mediawiki tarball
Open, NormalPublic

Description

Since 46bc943c161698ea90d549edab5efb735e715aa1 , mediawiki/vendor REL1_31 branch no more includes dependencies for WMF deployed extensions. It is restricted to packages used for the MediaWiki tarball.

Hence extensions not being part of the tarball would fail the CI tests on REL1_31 whenever they have a dependency that is not listed.

Examples:

AbuseFilter: PHP Fatal Error: Class 'Wikimedia\EquivSet\EquivSet' not found

[12-Mar-2018 16:01:15 UTC] PHP Fatal error:  Class 'Wikimedia\Equivset\Equivset' not found in /var/www/core/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php on line 1266
2018-03-12 16:01:15 language-cx2 my_wiki: [e4d91a1e] PHP Fatal Error: Class 'Wikimedia\Equivset\Equivset' not found

Thanks REL1_31 tests failing due to unknown class Pimple\Container

20:30:55 Fatal error: unknown class Pimple\Container in /workspace/src/extensions/Flow/includes/Container.php on line 5

I've run composer update to fetch latest dependencies.

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change 443412 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@REL1_31] Bump equivset to 1.3 to avoid PHP fatal error

https://gerrit.wikimedia.org/r/443412

Change 443407 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Bump equivset to 1.3 to avoid PHP fatal error

https://gerrit.wikimedia.org/r/443407

Change 443412 abandoned by Daimona Eaytoy:
Bump equivset to 1.3 to avoid PHP fatal error

Reason:
This doesn't solve the problem

https://gerrit.wikimedia.org/r/443412

Are there reproduction steps? I can't seem to reproduce the problem.

@dbarratt I'm not sure how to reproduce this. CI shows this error for REL1_31 branch, see e.g. https://integration.wikimedia.org/ci/job/mwext-testextension-hhvm-jessie/46253/console. I also have it on my local wiki, where equivset isn't included in vendor after updates. A composer update solves the problem, although it's not the solution we're looking for.

A composer update solves the problem, although it's not the solution we're looking for.

What's the solution you're looking for?

Also, I assume composer install also solves the problem?

@dbarratt Looking at the comments above:

Can't you just run composer update --no-dev in the AbuseFilter directory?

We should eliminate the class of issues for future, not fix a single instance by hand.

Can't you just run composer update --no-dev in the AbuseFilter directory?

This breaks the single-vendor directory principle. I think it would be better to install extensions with composer (T178137), but that's not something we support, so you're only other choice is to do what MediaWiki does and require the needed libraries in the root.

Sounds like updating composer isn't the right solution. And probably yes, composer install would also do the trick. I'd be glad to help resolving this bug, but I don't really know how to do it, mostly because I have little experience with composer.

dbarratt added a comment.EditedJul 10 2018, 1:31 PM

Sounds like updating composer isn't the right solution. And probably yes, composer install would also do the trick. I'd be glad to help resolving this bug, but I don't really know how to do it, mostly because I have little experience with composer.

Well the problem is is that AbuseFilter does not include the autoloader
https://getcomposer.org/doc/01-basic-usage.md#autoloading

so if you want to have a vendor inside of AbuseFilter we'd have to add something like:

require __DIR__ . '/vendor/autoload.php';

somewhere (I have no idea where this would go).

Although, like I said, this would mean you'd have more than one autoloader, which is an antipattern, but you can do it if you want/need. If this is a problem with just the CI, then perhaps the CI needs to be changed to either load the whole extension with Composer (which is what I would do) or to copy the dependencies into the root's composer.json (or composer.local.json) which is what Wikimedia does.

And what would be an alternative to having two autoloaders? Anyway, CI is just a case where this problem pops out, but I guess not the only one.

Reedy added a comment.Jul 10 2018, 1:49 PM

so if you want to have a vendor inside of AbuseFilter we'd have to add something like:

require __DIR__ . '/vendor/autoload.php';

somewhere (I have no idea where this would go).

This is what the following code in extension.json does:

"load_composer_autoloader": true

And what would be an alternative to having two autoloaders?

Here:

either load the whole extension with Composer (which is what I would do) or to copy the dependencies into the root's composer.json (or composer.local.json) which is what Wikimedia does.

Which basically means: informing the root composer.json of the dependencies (either the extension itself or the dependencies the extension(s) need).

This is what the following code in extension.json does:

"load_composer_autoloader": true

Perhaps this is the proper solution? I mean you'd still have two vendors, but if that's what most extensions do then that's probably fine.

Change 443412 restored by Reedy:
Bump equivset to 1.3 to avoid PHP fatal error

https://gerrit.wikimedia.org/r/443412

It's already in extension.json for AbuseFilter

https://integration.wikimedia.org/ci/job/release-quibble-vendor-mysql-hhvm-docker/5/consoleFull shows equivset being installed

13:40:58 [108.0MB/6.96s] Installs: wikimedia/utfnormal:v2.0.0, wikimedia/equivset:1.3.0, jakub-onderka/php-parallel-lint:v1.0.0, squizlabs/php_codesniffer:3.2.3, composer/spdx-licenses:1.3.0, composer/semver:1.4.2, mediawiki/mediawiki-codesniffer:v18.0.0, jakub-onderka/php-console-color:0.1, jakub-onderka/php-console-highlighter:v0.3.2, psr/log:1.0.2, symfony/debug:v3.4.12, symfony/polyfill-mbstring:v1.8.0, symfony/console:v3.4.12, mediawiki/minus-x:0.3.1

But then later, does another composer operation (on MW core?)

13:42:32 DEBUG:quibble.cmd:composer require monolog/monolog=~1.22.1 psy/psysh=0.8.11 jakub-onderka/php-parallel-lint=0.9.2 wikimedia/avro=1.8.0 nmred/kafka-php=0.1.5 jetbrains/phpstorm-stubs=dev-master#1b9906084d6635456fcf3f3a01f0d7d5b99a578a hamcrest/hamcrest-php=^2.0 composer/spdx-licenses=1.3.0 wikimedia/testing-access-wrapper=~1.0 justinrainbow/json-schema=~5.2 nikic/php-parser=3.1.3 wmde/hamcrest-html-matchers=^0.1.0 mediawiki/mediawiki-codesniffer=18.0.0 phpunit/phpunit=4.8.36 || ^6.5

It clones REL1_31 branch of Vendor... Which works fine on master, as equivset is there...

Kinda looks like CI is just doing something funky

hashar added a subscriber: hashar.Jul 10 2018, 4:17 PM

In reply to @Reedy, when a patch is sent for an extension, Quibble changes directory to it and then does:

  • npm install && npm test
  • composer install && composer test
  • git clean -xqdf

That git clean causes the npm/composer installed to be entirely dropped. And thus EquivSet is gone.

The Quibble jobs having vendor in their name have mediawiki/vendor.git added as a dependency. In that case, Quibble happily installing the composer dependencies. We assume that vendor has everything.

INFO:quibble.cmd:mediawiki/vendor used. Skipping external dependencies

Once MediaWiki has been installed and to be able to run tests, we have to complement mediawiki/vendor.git and install its dev dependencies. That is done by parsing the composer.json from vendor.git and passing the result to composer require:

quibble.cmd:composer require
monolog/monolog=~1.22.1
psy/psysh=0.8.11
jakub-onderka/php-parallel-lint=0.9.2
wikimedia/avro=1.8.0
nmred/kafka-php=0.1.5
jetbrains/phpstorm-stubs=dev-master#1b9906084d6635456fcf3f3a01f0d7d5b99a578a
hamcrest/hamcrest-php=^2.0
composer/spdx-licenses=1.3.0
wikimedia/testing-access-wrapper=~1.0
justinrainbow/json-schema=~5.2
nikic/php-parser=3.1.3
wmde/hamcrest-html-matchers=^0.1.0
mediawiki/mediawiki-codesniffer=18.0.0
phpunit/phpunit=4.8.36 || ^6.5

Eg phpunit.

TLDR: equivset is not in mediawiki/vendor REL1_31:

$ git grep wikimedia/equivset origin/master -- composer.json
origin/master:composer.json:            "wikimedia/equivset": "1.3.0",
$ git grep wikimedia/equivset origin/REL1_31 -- composer.json
$ git log --oneline -Gequivset  composer.json
e074b3b Update wikimedia/equivset to 1.3.0
c044357 Update wikimedia/equivset (1.0.0 => 1.2.0)
22ee4b9 Adding Equivset for AbuseFilter & AntiSpoof

One should probably add "wikimedia/equivset": "1.3.0" and regenerate mediawiki/vendor composer.lock :]

Reedy added a comment.Jul 10 2018, 7:38 PM

So we shouldn’t be using this job for non master then?

Reedy added a comment.Jul 10 2018, 7:39 PM

No we shouldn’t add it to the branch on vendor. AF isn’t shipped, so as we’re committing a cleaner vendor repo for core branches.., equivset shouldn’t make it in

I guess adding Equivset to vendor will be fine after T191740, or if AntiSpoof will be bundled (since it uses equivset as well). So maybe changing the job would be the best option?

Ah that is interesting. So mediawiki/vendor on REL branches should only have the dependencies for the extensions in the tarball isn't it?

It is probably a fault due to T197469. We have a set of extensions deployed on Wikimedia cluster that are tested together (eg Echo Flow Abusefilter etc). When a patch is sent to master or a wmf-branch a job prefixed wmf-quibble is run and uses mediawiki/vendor.git

Those repositories also use the release-quibble jobs with mediawiki/vendor.git. In the case of AbuseFilter, I guess for release branches we should use composer since it is not part of the tarball.

I took some notes

but that definitely deserves a nicer document. But in short we would need something like:

Wikimedia clusterIn tarballwmf/RELmaster
YesYesvendorvendorvendor (? + composer ?)
YesNovendorcomposervendor + composer
NoYesskipcomposercomposer
NoNoskipcomposercomposer

Or to say it otherwise:

  • use composer by default instead of vendor
  • when targeting wmf branches, only use vendor (I think that is the case today)
  • when targeting REL branches, if the extension is bundled use vendor, else use composer (or vendor + composer install to extend the set of dependencies)

The trick is figuring out per release branches which extensions are bundled. Maybe that can be done by cloning mediawiki/core @ RELXX, process the submodules then if the triggering extension is in the submodules use vendor, else use composer.

I guess I should copy paste that to a new task and think about how to implement it. I will probably make it a sub task of T197469.

Reedy added a comment.Jul 10 2018, 9:33 PM

Ah that is interesting. So mediawiki/vendor on REL branches should only have the dependencies for the extensions in the tarball isn't it?

Yeah, it's a relatively new thing for REL1_31 going forward - https://github.com/wikimedia/mediawiki-vendor/commit/46bc943c161698ea90d549edab5efb735e715aa1

Change 443412 abandoned by Umherirrender:
Bump equivset to 1.3 to avoid PHP fatal error

Reason:
equivset is not part of mediawiki/vendor in REL1_31, so this change does not help

https://gerrit.wikimedia.org/r/443412

hashar renamed this task from PHP Fatal Error: Class 'Wikimedia\EquivSet\EquivSet' not found to mediawiki/vendor REL1_31 no more ship dependencies for wmf extensions that are not in the mediawiki tarball.Aug 24 2018, 7:37 AM
hashar updated the task description. (Show Details)

I don't mind fiddling with the CI config, but I need a few pair of eyes to review my previous comment T189560#4413972

Legoktm claimed this task.Nov 14 2018, 12:38 AM

Change 473311 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[integration/config@master] Restore wmf-quibble-* jobs for REL1_XX branches

https://gerrit.wikimedia.org/r/473311

For the math extension, the problem is slightly different. If there was dependency management for MW extensions, I would call Wikidata and visual editor test dependencies. The mass extension extends the functionality of Visual Editor and Wikidata respective, which is tested in unit tests, but the extension works fine without these extensions. We could simply skip the tests if the VE or Wikidata extensions are not available. I did not implement it since I have kept up with the recent developments in extension loading and I have a strong feeling that the traditional way to check if the classes exists is deprecated.

Daimona renamed this task from mediawiki/vendor REL1_31 no more ship dependencies for wmf extensions that are not in the mediawiki tarball to mediawiki/vendor REL1_* no more ship dependencies for wmf extensions that are not in the mediawiki tarball.Feb 24 2019, 3:17 PM
Daimona added a subscriber: Legoktm.
Krinkle added a subscriber: Krinkle.

[integration/config@master] Restore wmf-quibble-* jobs for REL1_XX branches
https://gerrit.wikimedia.org/r/473311

@hashar If I understand correctly, it's preferred to solve this problem in a different way.

It seems multiple projects are currently unable to merge changes for 1.31, 1.32, and later release branches due to this.

Change 473311 abandoned by Hashar:
Restore wmf-quibble-* jobs for REL1_XX branches

https://gerrit.wikimedia.org/r/473311

hashar added a comment.Apr 5 2019, 7:07 PM

Sorry I have a long backlog. Yes the wmf-quibble jobs are really just meant for the wmf branches.

For release branches, I guess mediawiki/vendor used to provide enough dependencies that most repositories worked just fine.

mediawiki/vendor REL branches have been changed to only ship dependencies of the extension included in the MediaWiki tarball. So I guess we should simply use composer instead of vendor.

Namely replace jobs in zuul/layout. For example:

- release-quibble-vendor-mysql-hhvm-docker
+ release-quibble-composer-mysql-hhvm-docker

Eventually for the MediaWiki tarball, we could add a new set of jobs and a extension-tarball template in Zuul which would be applied on all repositories included in the tarball. Apparently I did such an attempt while migrating the jobs to Quibble: https://gerrit.wikimedia.org/r/#/c/integration/config/+/443578/ But that start to be a bit too complicated compared to just using composer.

Reedy renamed this task from mediawiki/vendor REL1_* no more ship dependencies for wmf extensions that are not in the mediawiki tarball to mediawiki/vendor REL1_* no longer ship dependencies for wmf extensions that are not in the mediawiki tarball.Apr 5 2019, 7:08 PM

Change 511866 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/config@master] Use composer for Mediawiki REL branches

https://gerrit.wikimedia.org/r/511866

hashar claimed this task.May 22 2019, 12:13 PM

https://gerrit.wikimedia.org/r/#/c/integration/config/+/511866 switches CI to only use composer install / composer merge plugin instead of relying on mediawiki/vendor.git

Change 517427 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/config@master] zuul: tests jobs triggered for mediawiki/vendor.git

https://gerrit.wikimedia.org/r/517427

Change 517427 merged by jenkins-bot:
[integration/config@master] zuul: tests jobs triggered for mediawiki/vendor.git

https://gerrit.wikimedia.org/r/517427

The merger of this patch means that the problem is solved?

Change 511866 merged by jenkins-bot:
[integration/config@master] Use composer for Mediawiki REL branches

https://gerrit.wikimedia.org/r/511866

I have send dummy changes against REL1_33 and also triggered the php jobs by commenting check php:

https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/ContentTranslation/+/517644/
https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/517645/
https://gerrit.wikimedia.org/r/#/c/mediawiki/vendor/+/517646/

We will see the results. There are probably a few more things that needs to be optimized :-\

Change 517662 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/config@master] Bring back release-quibble-vendor jobs

https://gerrit.wikimedia.org/r/517662

I will mess a bit more with the jobs triggered for mediawiki/vendor.git. It is a bit messy right now.

There are tests failing due to lack of dependencies, they will need to be investigate :-\

Change 517662 merged by jenkins-bot:
[integration/config@master] Bring back release-quibble-vendor jobs

https://gerrit.wikimedia.org/r/517662

greg triaged this task as Normal priority.Jul 3 2019, 10:29 PM