Page MenuHomePhabricator

Phan fails with php72 on WikibaseLexeme
Closed, ResolvedPublic

Description

Currently, all Lexeme builds fail with an error in phan:

[InvalidArgumentException]
Package phan/phan at version 0.8 has a PHP requirement incompatible with your PHP version (7.2.16)

As far as I can see, this might be caused by the interplay of 3 different patches. First having WikibaseLexeme's phan integration-config explicitly depend on Wikibase for T225524, then adjusting Wikibase's local phan configuration to use phan 1.3.4 for T217018: Upgrade Wikibase extension to use newer phan and finally to raise the php version of the phan runner for Wikibase in the integration config from 7.0 to 7.2. However, that left WikibaseLexeme with the old phan 0.8, but a new PHP 7.2 phan runner.

Full log:

00:01:17.870 [mwext-php72-phan-docker] $ /bin/bash -eu /tmp/jenkins1200382226019678461.sh
00:01:17.874 + chmod 2777 src
00:01:17.900 [mwext-php72-phan-docker] $ /bin/bash /tmp/jenkins6530407008460609516.sh
00:01:17.903 ++ pwd
00:01:17.903 ++ pwd
00:01:17.904 ++ pwd
00:01:17.905 ++ pwd
00:01:17.906 + exec docker run --volume /srv/jenkins-workspace/workspace/mwext-php72-phan-docker/src:/mediawiki --volume /srv/jenkins-workspace/workspace/mwext-php72-phan-docker/src:/src --volume /srv/jenkins-workspace/workspace/mwext-php72-phan-docker/cache:/cache --volume /srv/jenkins-workspace/workspace/mwext-php72-phan-docker/log:/log --init --rm --label jenkins.job=mwext-php72-phan-docker --label jenkins.build=278 --env-file /dev/fd/63 docker-registry.wikimedia.org/releng/mediawiki-phan:0.3.0 -m checkstyle
00:01:17.908 ++ /usr/bin/env
00:01:17.908 ++ egrep -v '^(HOME|SHELL|PATH|LOGNAME|MAIL|HHVM_REPO_CENTRAL_PATH)='
00:01:18.311 + umask 002
00:01:18.311 + cd /mediawiki/extensions/WikibaseLexeme
00:01:18.311 + '[' '!' -f /mediawiki/extensions/WikibaseLexeme/.phan/config.php ']'
00:01:18.311 + jq -e '.extra."phan"' composer.json
00:01:18.312 null
00:01:18.312 + CFG_COMPOSER=vendor/mediawiki/mediawiki-phan-config/composer.json
00:01:18.313 + jq -e '.extra."phan"' vendor/mediawiki/mediawiki-phan-config/composer.json
00:01:18.317 jq: error: Could not open file vendor/mediawiki/mediawiki-phan-config/composer.json: No such file or directory
00:01:18.318 + PHAN_VERSION=0.8
00:01:18.318 + '[' -f .phan/config.php ']'
00:01:18.318 + export PHP_ARGS=-dextension=ast_101.so
00:01:18.318 + PHP_ARGS=-dextension=ast_101.so
00:01:18.318 + install_phan
00:01:18.318 + cd /srv/phan
00:01:18.319 + composer require phan/phan:0.8
00:01:19.466 
00:01:19.477                                                                                                       
00:01:19.478   [InvalidArgumentException]                                                                          
00:01:19.478   Package phan/phan at version 0.8 has a PHP requirement incompatible with your PHP version (7.2.16)

It seems to be the same error as in T226156: Phan job fails on CI for mediawiki/core REL1_3[12].

Details

Related Gerrit Patches:
mediawiki/extensions/WikibaseLexeme : masterUpdate phan, cleanup config
mediawiki/extensions/WikibaseLexeme : masterUse newer phan version on WikibaseLexeme and make pass
mediawiki/extensions/WikibaseLexeme : masterTemporarily suppress phan checks to unbreak build.

Event Timeline

Michael created this task.Jun 21 2019, 10:12 AM
Restricted Application added a project: Wikidata. · View Herald TranscriptJun 21 2019, 10:12 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Michael triaged this task as High priority.Jun 21 2019, 10:12 AM
Michael updated the task description. (Show Details)
Michael moved this task from 🗃️ Incoming to 👁️ watching on the User-Michael board.

AIUI, only the last mentioned patch is culprit. The dependency on Wikibase should only be used to tell what MW extensions to install before testing Lexeme, but that shouldn't be related to the phan version to use. What happened, instead, is that IMHO phan72 jobs were made live too early. Currently, the phan runner still supports phan 0.8.0, which in turn can only run on PHP70. Lexeme doesn't specify a phan version to run, and for some reason the 0.8.0 version is picked.
I think some solutions could be to:

  1. Explicitly require phan config 0.6.1 in Lexeme's composer.json
  2. Make the phan72 job install a recent version of phan (not 0.8.0)

CC @Jdforrester-WMF

Change 518236 had a related patch set uploaded (by Noa wmde; owner: Noa wmde):
[mediawiki/extensions/WikibaseLexeme@master] Use newer phan version on WikibaseLexeme

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

@Daimona unfortunately requiring phan config 0.6.1 in composer.json did not seem to have an effect.

hashar added a subscriber: hashar.Jun 21 2019, 12:03 PM

That did cause the container to trigger the installation of phan 1.3.4 instead of the outdated 0.8.0 fallback.

https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/WikibaseLexeme/+/518236/ now fails because of new Phan reported errors. I guess the easiest is to just suppress them using @suppress comment:
https://github.com/phan/phan/wiki/Annotating-Your-Source-Code#suppress

I guess we have missed WikibaseLexeme was still using 0.8.0 :-\

@noarave It actually did. It's just that phan completed its analysis and found several errors. Of note, you should also add a phan config file - I can amend your patch, although I don't know if I'll have time to fix all the errors.
@hashar Actually, it's not Lexeme. It's the phan runner, specifically this line which chooses 0.8.0 as fallback if it cannot find phan in the composer. So basically, this issue also affects any repo where no phan version is specified. Changing the fallback option to phan 1.3.4 could solve it - unless it creates more troubles.

Another possible source?
WikibaseLexeme .phan/config.php looks different from Wikibase one.

For example, one reported error on Lexeme build:

12:37:08   <file name="src/DataAccess/ChangeOp/AddFormToLexemeChangeOp.php">
12:37:08     <error line="7" severity="warning" message="Possibly zero references to use statement for classlike/namespace Form (\Wikibase\Lexeme\Domain\Model\Form)" source="PhanUnreferencedUseNormal"/>
12:37:08   </file>

Looking in Lexemes phan config, PhanUnreferencedUseNormal is not configured to be ignored, while it is configured to be ignored in Wikibase https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/Wikibase/+/refs/heads/master/.phan/config.php#371

So ignoring those checks for now in Lexeme phan config should as well be enough until we enable them again later and fix one by one

Change 518249 had a related patch set uploaded (by Alaa Sarhan; owner: Alaa Sarhan):
[mediawiki/extensions/WikibaseLexeme@master] Temporarily suppress phan checks to unbreak build.

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

Change 518256 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/WikibaseLexeme@master] [WIP] Update phan, cleanup config

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

Change 518256 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/WikibaseLexeme@master] [WIP] Update phan, cleanup config
https://gerrit.wikimedia.org/r/518256

@Daimona think you are trying to achieve the same effect I did with https://gerrit.wikimedia.org/r/518249 (https://phabricator.wikimedia.org/T226244#5273778).

Though I wouldn't add an explicit dependency here on Wikibase phan config. Lexeme and Wikibase can have different configurations in the long run. What do you think?

Change 518256 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/WikibaseLexeme@master] [WIP] Update phan, cleanup config
https://gerrit.wikimedia.org/r/518256

@Daimona think you are trying to achieve the same effect I did with https://gerrit.wikimedia.org/r/518249 (https://phabricator.wikimedia.org/T226244#5273778).
Though I wouldn't add an explicit dependency here on Wikibase phan config. Lexeme and Wikibase can have different configurations in the long run. What do you think?

@alaa_wmde Actually, not really. With the phan dependency in composer, we can use the standard MediaWiki config (source), which is what all extensions should do. For some reason, Wikibase redeclares its own config, so that it'll inevitably drift from the default. My change introduces a dependency on the standard config, not the Wikibase one, so this is the right way to go :-)
I also tried to *remove* all suppressions to see what's really left to fix. The first thing I noticed is that all "A inherits B which is declared at X and also at Y" warnings have gone, and that was due to misconfiguration including Wikibase twice, probably.
Now it complains about some methods not being declared (instead of being declared twice, eheh), but that's probably something we can fix via config. You'll also notice many warnings like "...can usually be simplified to...". They come from a phan plugin which is included in the standard config, but not in Wikibase - that's what I meant with "it'll inevitably drift from the default".

At this point, I'd like to fix all issues reported for my patch, but that could take some time. And at the end, the same config change will have to be applied to Wikibase itself.

alaa_wmde added a comment.EditedJun 21 2019, 1:30 PM

@Daimona understood. I also think the clean-up work is necessary. I'm not such a fan of statements of the sort "extensions must use mediawiki config rather than overriding/changing them" but that's a separate topic anyway.

At this point, I'd like to fix all issues reported for my patch, but that could take some time. And at the end, the same config change will have to be applied to Wikibase itself.

I would actually like to merge my patch first to fix the build.. your patch can then rebase on it. Is that fine by you? mind as well tagging https://phabricator.wikimedia.org/T226254 that I created for unsuppressing the checks by fixing the errors?

@Daimona understood. I also think the clean-up work is necessary. I'm not such a fan of statements of the sort "extensions must use mediawiki config rather than overriding/changing them" but that's a separate topic anyway.

Ah, I haven't been clear. Extensions *should* override the default config, e.g. to define additional dependencies (many extensions do that). But it's vital that the default config is first included, then overridden for the necessary part. What happened for Lexeme (and WB itself) is that the default config has just been copypasted. And you'll probably agree that copypastas are always very bad for maintainability in the long term.

I would actually like to merge my patch first to fix the build first.. your patch can then rebase on it. Is that fine by you?

Definitely, I implicitly meant that.

Ah, I haven't been clear. Extensions *should* override the default config, e.g. to define additional dependencies (many extensions do that). But it's vital that the default config is first included, then overridden for the necessary part. What happened for Lexeme (and WB itself) is that the default config has just been copypasted. And you'll probably agree that copypastas are always very bad for maintainability in the long term.

Ah I see .. I might have also read through your comment too quickly .. thanks for the second clarification.

Definitely, I implicitly meant that.

Awesome! thanks for taking care of the clean-up too :)

Change 518249 abandoned by Jforrester:
Temporarily suppress phan checks to unbreak build.

Reason:
Squashed into parent.

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

Change 518236 merged by Jforrester:
[mediawiki/extensions/WikibaseLexeme@master] Use newer phan version on WikibaseLexeme and make pass

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

Jdforrester-WMF closed this task as Resolved.Jun 21 2019, 6:13 PM
Jdforrester-WMF assigned this task to alaa_wmde.
Jdforrester-WMF removed a project: Patch-For-Review.

Thank you for fixing this.

Change 518256 merged by jenkins-bot:
[mediawiki/extensions/WikibaseLexeme@master] Update phan, cleanup config

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