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

In T226945#5302166, @Jdforrester-WMF wrote:

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?

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.

In T226945#5370566, @Legoktm wrote:

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).

Related: T231966: Phan job should use composer instead of vendor for release branches ( undeclared class \Wikimedia\Equivset\Equivset ) . The Phan job uses mediawiki/vendor.git which does not have all dependencies on release branches.

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

In T226945#5370566, @Legoktm wrote:

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).

In T226945#5614208, @Daimona wrote:

In T226945#5370566, @Legoktm wrote:

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 :)

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.

In T226945#5647667, @hashar wrote:

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.

In T226945#5647667, @hashar wrote:

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 :]

{{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.

In T226945#5647679, @Daimona wrote:

{{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.

Change 525878 abandoned by Jforrester:
[integration/config@master] layout: Restrict Phan jobs to only run on master, wmf/, & fundraising/ branches

Reason:
This was done by default when we split the branches into their own pipelines.

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

Change 673503 had a related patch set uploaded (by WMDE-leszek; owner: WMDE-leszek):
[integration/config@master] Run phan on changes onto Wikibase REL1_35 branch

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

Change 683033 had a related patch set uploaded (by Jforrester; author: Jforrester):

[integration/config@master] Zuul: Update T226945 comment now we've dropped PHP70/71 support

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

Change 683033 abandoned by Jforrester:

[integration/config@master] Zuul: Update T226945 comment now we've dropped PHP70/71 support

Reason:

I did this when I in-lined the phan jobs into the general quibble jobs

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

Change 984222 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[integration/config@master] Zuul: [mediawiki/extensions/Wikibase] Disable Phan on release branches

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

Change 984222 merged by jenkins-bot:

[integration/config@master] Zuul: [mediawiki/extensions/Wikibase] Disable Phan on release branches

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

Mentioned in SAL (#wikimedia-releng) [2024-01-09T08:53:17Z] <hashar> Reloaded Zuul for https://gerrit.wikimedia.org/r/984222 | [mediawiki/extensions/Wikibase] Disable Phan on release branches | T226945 T231966