Page MenuHomePhabricator

Upgrade php-ast to 1.0.1
Open, Needs TriagePublic

Description

CI currently runs with version 0.1.2 of php-ast, as can be seen in https://integration.wikimedia.org/ci/job/composer-package-php70-docker/5497/console. However, we need version 0.1.4 (at least) to test seccheck, and 0.1.2 is very old anyway.

Event Timeline

Daimona created this task.Mar 19 2019, 6:58 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 19 2019, 6:58 PM

@Legoktm could you please take a look?

Will the current plugin continue to work properly with ast 0.1.4? Or do we also need to keep 0.1.2 for back/compat reasons?

@Legoktm I think the current version should work with 0.1.4. But actually, could you please add 0.1.5 as well? We'll need it for the next upgrade. Thanks!

@Legoktm I think the current version should work with 0.1.4.

Gotcha. I can do that then.

But actually, could you please add 0.1.5 as well? We'll need it for the next upgrade. Thanks!

Building newer versions of php-ast is trivial, the main problem is determining which version to use when running phan. If you look at https://gerrit.wikimedia.org/r/plugins/gitiles/integration/config/+/master/dockerfiles/mediawiki-phan/run.sh#36 you can see how I worked around this for standard phan.

I suppose we coud look at the phan version number being installed and do some conditional based on that.

@Legoktm Cool, thanks! I think for now you can use whatever hack comes more handy for seccheck, as at the end of the upgrade we should ideally have a single version installed.
A conditional checking the required phan version seems fine.

From a CI perspective, we need to keep supporting older branches of MediaWiki that we run tests against, so the "migration" of php-ast from my perspective only ends once the last branch using the older version is phased out.

@Legoktm Right. But anyway at the end of the migration we'll only have 0.1.4 for the current version, and ?[0] for the final version based on 1.2.x.

[0]: To be determined according to phan's requirement, I guess 1.0.x as in the phan job.

hashar renamed this task from Upgrade ext-ast to 0.1.4 to Upgrade php-ast to 0.1.4.Mar 20 2019, 8:27 AM
hashar updated the task description. (Show Details)
hashar added a subscriber: hashar.Mar 20 2019, 8:34 AM

Tentatively that has been solved already?

We have T174339 to upgrade to phan 0.8.5, which requires a newer version of php-ast than the one we had.

T174338: Provide php-ast 0.1.5 or later as a Debian package for CI was the task to get a newer one, I have forked the Debian repository back in January 2018 and rebuild it but the resulting Debian package never got published. I got distracted by an unrelated build failure.

Debian Stretch ships with 0.1.2, but testing has 0.1.6 so we can probably ask Operations to rebuild it for us and publish it to apt.wikimedia.org . As noted, the package is also available in sury.org, but that is not ideal (that might brings a lot more different packages as well).

Finally cscott found that Phan provides a parser to replace php-ast when it is not present phan --allow-polyfill-parser. And it seems to be deemed good enough.

From a CI perspective, we need to keep supporting older branches of MediaWiki that we run tests against, so the "migration" of php-ast from my perspective only ends once the last branch using the older version is phased out.

Potentially we could use per branches jobs that would be stalled to some arbirtary old version of php-ast/phan. However the current system is centrally managed which is a bit of a maintenance pain. For jobs that use php-ast we could only trigger them for the master/wmf branch, but most of the jobs should probably just loose the php-ast requirements via phan --allow-polyfill-parser.

We have T174339 to upgrade to phan 0.8.5, which requires a newer version of php-ast than the one we had.

IIUC that's for the phan job itself which yes, already has php-ast v.1.0.1 installed.

Debian Stretch ships with 0.1.2, but testing has 0.1.6 so we can probably ask Operations to rebuild it for us and publish it to apt.wikimedia.org . As noted, the package is also available in sury.org, but that is not ideal (that might brings a lot more different packages as well).

I think we should proceed with little steps to avoid breaking anything, and thus upgrade to 0.1.4 first, then 0.1.5 (and then ...?).

Finally cscott found that Phan provides a parser to replace php-ast when it is not present phan --allow-polyfill-parser. And it seems to be deemed good enough.

True, but it's been introduced in 0.8.13, which means that we still need ast until we get there.

@Legoktm I checked all the remaining versions up to 1.2.6 and I can now confirm that we only need 0.1.5 for php-ast. 0.1.2 should be kept for the current version only.

Daimona renamed this task from Upgrade php-ast to 0.1.4 to Upgrade php-ast to 1.0.1.Apr 28 2019, 8:48 AM

OK, so the final request is:

  • Have both an old version (0.1.2? 0.1.4?) and 1.0.1 for the seccheck job, just like it happens for the phan job
  • Have 1.0.1 enabled for jobs running in the seccheck repo, when seccheck 2.0 will be released.

Ideally, though, any non-phan-related job should have 1.0.1 installed, because 0.1.2 is very old anyway.

@Legoktm Per T216974#5143707, would it be possible to implement the same solution used for the phan job?

Ah, tests for the 2.x branch were just re-enabled, but now they fail due to ast 0.1.2 being installed. See for instance https://integration.wikimedia.org/ci/job/composer-package-php70-docker/6239/console.

At this point, we'd need php-ast 1.0.1 to be installed for the following jobs:

  • composer-package-php70-docker
  • composer-package-php71-docker
  • composer-package-php72-docker

For what concerns the 70 job, we would also need to keep ast 0.1.2 if the commit being analyzed is for the SecurityCheckPlugin repo, but NOT in the 2.x branch. If that's too complicated, I guess we can just upgrade to ast 1.0.1 everywhere and make tests on master temporarily fail, given that the release of 2.0 is close and I don't think we'll have to push anything to master until then.
@hashar Could you please take a look?