Page MenuHomePhabricator

mediawiki/core and mediawiki/vendor both skip composer.lock checks
Closed, ResolvedPublic

Description

Today

A patch to mediawiki/core runs two kinds of jobs:

  1. Jenkins jobs that freshly install the latest third-party dependencies from packagist.org based the constraints in composer.json, and then run all PHPUnit tests with that.
  2. Jenkins jobs that download mediawiki/vendor.git, verify its lock file to be compatible, and then run with that.

They differ in exact versions of dependencies, since some ranges allow multiple versions.

Except.. this isn't what happens today. In reviewing https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1054944, which increments a mediawiki/core dependency across a semver-major version (wikimedia/less.php 4.x to 5.0), there was no failure in the mediawiki-vendor job.

Output of "mediawiki-quibble-vendor-mysql-php74" job, for a mediawiki/core pach:


mediawiki/vendor is used, skip composer.lock check

Now, comparing this to:

README file of mediawiki/vendor:

Note that you MUST pair patches changing versions of libraries used by MediaWiki itself with ones for the "core" repo. Specifically, the patch in mediawiki/core must have a Depends-On footer to the patch in mediawiki/vendor [otherwise it will fail CI].
The vendor repo has special configuration, which skips the integrity checks and so allowing a circular dependency Gordian knot to be fixed. However, this means that, if merged alone without a pair, you'll cause ALL patches in MediaWiki and ALL extensions to fail their continuous integration tests.

This is claiming the exact opposite, by saying it's vendor that skips the checks and core will do them instead.

Unfortunately both of these are correct in describing what they do, which means neither performs the checks anymore. It appears this has been broken since November 2023.

Background

From T333412: mediawiki/vendor can't be updated due to phpunit reporting composer.lock failure in https://gerrit.wikimedia.org/r/c/integration/quibble/+/903832:

# In the mediawiki/vendor update workflow, we patch vendor (and its
# lock file) first, and update the mediawiki/core composer.json after.
# As such, when testing mediawiki/vendor, it's normal that its lock
# file doesn't match mediawiki/core. This is tested by the
# mediawiki/core commit instead.
#
# T333412
if use_vendor:
    os.environ['MW_SKIP_EXTERNAL_DEPENDENCIES'] = '1'

It appears this has conflated the source with the destination.

It should be bypassing the check when running patches to the mediawiki/vendor repository.

Instead, it is bypassing the check for all mediawiki/core patches in the job that uses mediawiki/vendor. The check is neccessary there, since that's why we use Depends-On, and why it is generally safe to merge composer changes in mediawiki/core, and why the part where extra care is needed is in mediawiki/vendor. However, right now composer changes in core are unprotected and unverified.

Event Timeline

Change #1054966 had a related patch set uploaded (by Krinkle; author: Krinkle):

[integration/quibble@master] Fix MW_SKIP_EXTERNAL_DEPENDENCIES to be set for is_vendor not use_vendor

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

Change #1054966 merged by jenkins-bot:

[integration/quibble@master] Fix MW_SKIP_EXTERNAL_DEPENDENCIES to be set for is_vendor not use_vendor

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

Change #1055442 had a related patch set uploaded (by Hashar; author: Hashar):

[integration/quibble@master] release: Quibble 1.9.4

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

Change #1055452 had a related patch set uploaded (by Hashar; author: Hashar):

[integration/config@master] dockerfiles: update Quibble to 1.9.4

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

Change #1055453 had a related patch set uploaded (by Hashar; author: Hashar):

[integration/config@master] jjb: switch jobs from Quibble 1.9.3 to 1.9.4

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

Change #1055442 merged by jenkins-bot:

[integration/quibble@master] release: Quibble 1.9.4

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

Change #1055452 merged by jenkins-bot:

[integration/config@master] dockerfiles: update Quibble to 1.9.4

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

The fix for MW_SKIP_EXTERNAL_DEPENDENCIES has been applied to the Jenkins jobs.

Change #1055453 merged by jenkins-bot:

[integration/config@master] jjb: switch jobs from Quibble 1.9.3 to 1.9.4

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

Thanks @hashar. Moving back to our workboard to verify on the MediaWiki side.

When writing a patch to mediawiki/core that updates composer.json, the CI job should report a failure unless the mediawiki/vendor repository supports the same version already, or the patch has a Depends-On to a patch in mediawiki/vendor that performs the same update.

Krinkle triaged this task as Medium priority.

Change #1057272 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] [WIP] Is composer.lock verification is working?

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

Change #1057272 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] [WIP] Is composer.lock verification is working?

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

The good news: CI is correctly detecting mismatches and failing the build.

From Jenkins job mediawiki-quibble-vendor-mysql-php74
> phpunit '--colors=always' '--testsuite=core:unit,extensions:unit,skins:unit' '--exclude-group' 'Broken,ParserFuzz,Stub'
Using PHP 7.4.33
Running without MediaWiki settings because there are no integration tests
wikimedia/less.php: 5.0.0 installed, 4.4.1 required.

Error: your composer.lock file is not up to date. Run "composer update --no-dev" to install newer dependencies
Script phpunit handling the phpunit event returned with error code 1

The bad news: The output also still contains this message much earlier on, around the time where the MW installer and update.php run:

mediawiki/vendor is used, skip composer.lock check

Change #1057272 abandoned by Krinkle:

[mediawiki/core@master] [WIP] Is composer.lock verification is working?

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

@hashar Do you remember why we control these separately? If we can toggle both on or off in the same circumstances, perhaps we can use the ENV for both?

The bad news: The output also still contains this message much earlier on, around the time where the MW installer and update.php run:

mediawiki/vendor is used, skip composer.lock check

Looking at the console log, the output has:

22:13:04 Database was successfully set up
22:13:04 MediaWiki has been successfully installed. You can now visit <http://127.0.0.1:9413> to view your wiki. If you have questions, check out our frequently asked questions list: <https://www.mediawiki.org/wiki/Special:MyLanguage/Manual:FAQ> or use one of the support forums linked on that page.
22:13:04 Copying /workspace/src/LocalSettings.php to /workspace/log/LocalSettings.php
22:13:04 Copying /workspace/src/LocalSettings-installer.php to /workspace/log/LocalSettings-installer.php
22:13:04 No syntax errors detected in /workspace/src/LocalSettings.php
22:13:04 mediawiki/vendor is used, skip composer.lock check
22:13:04 php maintenance/addSite.php wikidb CI --filepath=http://127.0.0.1:9413/$1 --pagepath=http://127.0.0.1:9413/index.php?title=$1

That is Quibble InstallMediaWiki command which output that message whenever use_vendor is set in order (T88211, T333412):

update_args = []
if self.use_vendor:  
    # When trying to update a library in mediawiki/core and
    # mediawiki/vendor, a circular dependency is produced as both
    # patches depend upon each other.
    #
    # All non-mediawiki/vendor jobs will skip checking for matching
    # versions and continue "at their own risk". mediawiki/vendor will
    # still check versions to make sure it stays in sync with MediaWiki
    # core.
    #
    # T88211, T333412
    log.info('mediawiki/vendor is used, skip composer.lock check')
    update_args.append('--skip-external-dependencies')

quibble.mediawiki.maintenance.addSite(
    args=[
        self.db.dbname,  # globalid
        'CI',  # site-group
        '--filepath=%s/$1' % self.web_url,
        '--pagepath=%s/index.php?title=$1' % self.web_url,
    ],
    mwdir=self.mw_install_path,
)

Which at first glance, is the same issue you fixed at f1e4f3af1847d03f9f0997bb68952be2b3866a98 for setting up the environment. I guess the fix is to change InstallMediaWiki the same way and switch to is_vendor? The caller in quibble/cmd.py:

parallel_steps.append(
    quibble.commands.InstallMediaWiki(
        mw_install_path=mw_install_path,
        db=database_backend,
        web_url=web_backend.url,
        log_dir=log_dir,
        tmp_dir=tmp_dir,
        use_vendor=use_vendor,   # <---- *whistles*
    )
)

Yeah, if there's no reason to skip this in other repos with or without using vendor, we can do that.

Alternatively, I can change (or "fix") core to check the same ENV. Then, instead of fixing/syncing this duplicate code, we can remove it.

Right now in Quibble, the step I fixed previously checks is_vendor and sets an ENV. Now, this other step is still checking use_vendor (wrong) and uses it to append the --skip-external-dependencies option. But if update.php honours the ENV, we don't need that.

I'm wondering why we implemented it in two different ways.

Change #1063911 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] installer: Support MW_SKIP_EXTERNAL_DEPENDENCIES in update.php

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

Change #1063912 had a related patch set uploaded (by Krinkle; author: Krinkle):

[integration/quibble@master] Remove update.php --skip-external-dependencies in favor of MW_SKIP_EXTERNAL_DEPENDENCIES

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

Change #1063911 merged by jenkins-bot:

[mediawiki/core@master] installer: Support MW_SKIP_EXTERNAL_DEPENDENCIES in update.php

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

Change #1067358 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@fundraising/REL1_39] installer: Support MW_SKIP_EXTERNAL_DEPENDENCIES in update.php

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

Change #1067359 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@REL1_39] installer: Support MW_SKIP_EXTERNAL_DEPENDENCIES in update.php

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

Change #1067359 merged by jenkins-bot:

[mediawiki/core@REL1_39] installer: Support MW_SKIP_EXTERNAL_DEPENDENCIES in update.php

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

The jobs for https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/1064132 just failed with Error: your composer.lock file is not up to date. Run "composer update" to install newer dependencies, is the work here related?

The jobs for https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/1064132 just failed with Error: your composer.lock file is not up to date. Run "composer update" to install newer dependencies, is the work here related?

That happens if the vendor patch has merged and the core patch hasn't, which is a race condition of a few minutes as patches land. The failure was previously hidden (as it was untested), yes.

Krinkle added a subscriber: Ejegg.

Change #1067358 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@fundraising/REL1_39] installer: Support MW_SKIP_EXTERNAL_DEPENDENCIES in update.php

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

@Ejegg Could you have a look at this one? We generally maintain one version of Quibble that runs CI the same way on all branches. Unless we create a different version for fundraising, the above patch is blocking https://gerrit.wikimedia.org/r/c/integration/quibble/+/1063912 which resolves the bug described in this task.

The above patch has landed in the REL1_39 branch meanwhile, so a general fundraising/REL1_39 branch update would also achieve the same effect. Whichever you prefer :)

greg removed Krinkle as the assignee of this task.Sep 19 2024, 9:26 PM

Thanks for the suggestion, @Krinkle . I've just merged the REL1_39 branch in to fundraising/REL1_39 and it merged without an issue.

Change #1078355 had a related patch set uploaded (by Hashar; author: Krinkle):

[mediawiki/core@REL1_41] installer: Support MW_SKIP_EXTERNAL_DEPENDENCIES in update.php

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

Change #1078356 had a related patch set uploaded (by Hashar; author: Krinkle):

[mediawiki/core@REL1_42] installer: Support MW_SKIP_EXTERNAL_DEPENDENCIES in update.php

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

The fundraising/REL1_39 branch has been updated by https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1075989

I have made the cherry picks for REL1_41 and REL1_42

I would have CR+2 them but I don't know what is the process nowadays for releases backports. I guess they can be approved as is?

Once merged, all branches would no more rely on --skip-external-dependencies and we can merge and deploy https://gerrit.wikimedia.org/r/c/integration/quibble/+/1063912 :)

Change #1078355 merged by jenkins-bot:

[mediawiki/core@REL1_41] installer: Support MW_SKIP_EXTERNAL_DEPENDENCIES in update.php

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

Change #1078356 merged by jenkins-bot:

[mediawiki/core@REL1_42] installer: Support MW_SKIP_EXTERNAL_DEPENDENCIES in update.php

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

Change #1063912 merged by jenkins-bot:

[integration/quibble@master] Remove update.php --skip-external-dependencies in favor of MW_SKIP_EXTERNAL_DEPENDENCIES

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

This is not resolved since https://gerrit.wikimedia.org/r/c/integration/quibble/+/1063912 hasn't been released nor deployed on the CI infrastructure. That was pending the backport to all supported MediaWiki branches.

Ack. We'll need a Quibble release and corresponding CI job updates. I'll finish a few other things this week first, and then in a few days if there's no other release by then, I'll do this.

Change #1084158 had a related patch set uploaded (by Krinkle; author: Krinkle):

[integration/quibble@master] release: Quibble 1.11.0

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

Change #1084158 merged by jenkins-bot:

[integration/quibble@master] release: Quibble 1.11.0

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

Change #1085417 had a related patch set uploaded (by Hashar; author: Hashar):

[integration/config@master] dockerfiles: update Quibble to 1.11.0

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

Change #1085417 merged by jenkins-bot:

[integration/config@master] dockerfiles: update Quibble to 1.11.0

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

Change #1085426 had a related patch set uploaded (by Hashar; author: Hashar):

[integration/config@master] jjb: switch jobs to Quibble 1.11.0

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

Change #1085426 merged by jenkins-bot:

[integration/config@master] jjb: switch jobs to Quibble 1.11.0

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

The Quibble release is now deployed. Unfortunately the main core/vendor patch pair right now (1078411 and 1078410) have an actual cyclic dependency, so this isn't a great test of the new logic.