Page MenuHomePhabricator

Decide on future of running Phan tests on release branches
Open, Needs TriagePublic

Description

Following on from T226766: Avoid explicit commit references in composer.json in code meant for public consumption, T226156: Phan job fails on CI for mediawiki/core REL1_3[12], https://gerrit.wikimedia.org/r/#/c/integration/config/+/519793/ and the mediawiki-core-php72-phan-docker jobs

Phan in 1.31 and 1.32 won't run on mediawiki-core-php72-phan-docker because it has PHP 7.2, not 7.0.

As such, the phan jobs have been completely disabled in the branch, pending a longer term solution.

I don't believe we want to continue running jobs on PHP 7.0... So the options are to leave things disabled, or backporting whatever is necessary to run phan compatible with PHP 7.2 to 1.31 and 1.32

Event Timeline

Reedy created this task.Jun 30 2019, 9:35 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 30 2019, 9:35 PM

Yeah, I'd propose just running them on master.

Reedy added a comment.Jul 3 2019, 12:23 PM

Yeah, I'd propose just running them on master.

Doesn't seem unreasonable. Are we going to blanket disable on branches? Or just disable when it stops working due to external factors?

I think we don't really need it on WMF branches (though, in most cases if master works, they should too) as they're fairly short lived too, and we're generally picking from master anyway

Do we need to document it anywhere?

hashar added a subscriber: hashar.Jul 24 2019, 5:14 PM

A real life example https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/UserMerge/+/525286/ for REL1_33

Package mediawiki/phan-taint-check-plugin at version 1.5.1 has a PHP requirement incompatible with your PHP version (7.2.16)

So I guess for now we should apply a branch filter to the job mwext-php72-phan-seccheck-docker so that it only runs against master. For the old branches, we would need to keep providing some php7.0 based container.

I'd like to make sure they keep running on wmf branches at least. Especially our practice of cherry picking things quite frequently, it becomes quite likely that an implicit dependency gets forgotten in a way that isn't detected by Git (e.g. not in the same diff context).

Phan is pretty much our only line of defence there to detect things like missing variables and (renamed) classes etc.

Change 525878 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[integration/config@master] layout: Restrict Phan jobs to only run on master, wmf/, & fundraising/ branches

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

We absolutely need phan for release branches. Many of the regressions in previous releases could've been caught if phan were running.

We absolutely need phan for release branches. Many of the regressions in previous releases could've been caught if phan were running.

People pushing commits directly into legacy release branches, rather than back-ports of merged and tested code in master, are already risking quite a bit, but this is a fair concern.

Given that we're able to backport fixes to make other CI infra upgrades work, I think we should be able to do the same for phan. The easy way is to keep the php70 containers around until we drop support for those branches.

Alternatively we could consider looking into using phan's pure-PHP fallback/polyfill parser so we don't need to mess with php-ast versions as much. But that comes with its own problems (though I'm skeptical about what the performance will be like).

Change 534426 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/config@master] zuul: disable phan on extensions release branches

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

https://gerrit.wikimedia.org/r/#/c/integration/config/+/534426 disables the phan job for extensions/skins on release branches due to T231966

Change 534426 merged by jenkins-bot:
[integration/config@master] zuul: disable phan on extensions release branches

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

Mentioned in SAL (#wikimedia-releng) [2019-09-04T12:20:37Z] <hashar> Dropped Phan jobs for extensions from release branches T226945

Daimona added a subscriber: Daimona.Sep 4 2019, 2:55 PM
Krinkle moved this task from Inbox to Checkers on the MediaWiki-Core-Testing board.Sep 4 2019, 5:48 PM

We absolutely need phan for release branches. Many of the regressions in previous releases could've been caught if phan were running.

This. It's just too easy to introduce regressions when backporting changes, especially big changes like security changes. Example: https://gerrit.wikimedia.org/r/#/q/Ie23e8234ae550273bf3f6f9c5ac45b7fc54eec2a this would've broken 1.3[123], but no tests failed.

A quick note: in order to keep phan working on release branches, we cannot remove the docker script part where it checks for "extra" in composer.json to install phan/phan (which isn't needed anymore for repos using phan-config 0.8.0).

We absolutely need phan for release branches. Many of the regressions in previous releases could've been caught if phan were running.

This. It's just too easy to introduce regressions when backporting changes, especially big changes like security changes. Example: https://gerrit.wikimedia.org/r/#/q/Ie23e8234ae550273bf3f6f9c5ac45b7fc54eec2a this would've broken 1.3[123], but no tests failed.

I obviously concur :)

hashar added a comment.Fri, Nov 8, 2:11 PM

I am not active on the Phan front, but could we imagine upgrading Phan in our release branches toward 0.8 to get rid of that extra in composer.json? It might be possible simply by adding some ignore/exclude for newly introduced rules we probably do not want to fix in release branches.

Same for phan versions that still require 7.0. We might be able to upgrade Phan in release branches to whatever Phan version supporting 7.2.

I am not active on the Phan front, but could we imagine upgrading Phan in our release branches toward 0.8 to get rid of that extra in composer.json? It might be possible simply by adding some ignore/exclude for newly introduced rules we probably do not want to fix in release branches.

That's currently happening, yes. See T235049: Upgrade mediawiki-phan-config to 0.8.0 for all repos.

Same for phan versions that still require 7.0. We might be able to upgrade Phan in release branches to whatever Phan version supporting 7.2.

Yes, after this point we may move phan into the main quibble run and so run the appropriate version for repos automatically. That doesn't fix old release branches, however.

I am not active on the Phan front, but could we imagine upgrading Phan in our release branches toward 0.8 to get rid of that extra in composer.json? It might be possible simply by adding some ignore/exclude for newly introduced rules we probably do not want to fix in release branches.

It's probably doable. I don't think we have a script to do that (like LibUp does for PHPCS), but I think it shouldn't be terribly hard to write.

Same for phan versions that still require 7.0. We might be able to upgrade Phan in release branches to whatever Phan version supporting 7.2.

I believe that we don't have any phan version requiring PHP 7.0, it's either 5.6.99+, or 7.2.0+.

You guys are amazing. As soon as the extras field is not needed anymore, lets definitely clear it up from the CI phan shell scripts :]

Daimona added a comment.EditedFri, Nov 8, 2:21 PM

{{edit conflict}} Belatedly, T235049 is only for master, nothing is being done on rel branches.

Also, I'm unsure whether phan should be merged into another existing job. Phan can already take a decent amount of time (and memory) to execute, and it will get even worse with T235390.

{{edit conflict}} Belatedly, T235049 is only for master, nothing is being done on rel branches.

Well, yeah, it'll affect REL1_35 onwards though. ;-)

Also, I'm unsure whether phan should be merged into another existing job. Phan can already take a decent amount of time (and memory) to execute, and it will get even worse with T235390.

The tension between speed and simplicity is hard to work out, yeah.