Maniphest T196956

mwext-MobileFrontend-npm-run-lint-modules-docker failing - node script running as php
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Jdlrobson
	Jun 11 2018, 10:20 PM

Project Tags

Referenced Files

None

Subscribers

Description

Recently, the resource-modules node modules has started executing as php in CI rather than via Node.
Presumably something has changed in the pipeline relating to Docker

https://integration.wikimedia.org/ci/job/mwext-MobileFrontend-npm-run-lint-modules-docker/724/console

Please advise! This is blocking all merges to MobileFrontend!

Sample log output:

22:06:29 + exec docker run --workdir /src/extensions/MobileFrontend --rm --env-file /dev/fd/63 --volume /srv/jenkins-workspace/workspace/mwext-MobileFrontend-npm-run-lint-modules-docker/log:/log --volume /srv/jenkins-workspace/workspace/mwext-MobileFrontend-npm-run-lint-modules-docker/cache:/cache --volume /srv/jenkins-workspace/workspace/mwext-MobileFrontend-npm-run-lint-modules-docker/src:/src docker-registry.wikimedia.org/releng/npm-php:0.1.0 run-script lint:modules
22:06:30 
22:06:30 > @ lint:modules /src/extensions/MobileFrontend
22:06:30 > resource-modules ./
22:06:30 
22:06:31 { Error: Command failed: php /src/extensions/MobileFrontend/node_modules/resource-modules/src/php/resources.php /src /resources/Resources.php
22:06:31 PHP Parse error:  syntax error, unexpected '(' in /src/includes/SiteConfiguration.php on line 435
22:06:31 
22:06:31     at ChildProcess.exithandler (child_process.js:204:12)
22:06:31     at emitTwo (events.js:106:13)
22:06:31     at ChildProcess.emit (events.js:191:7)
22:06:31     at maybeClose (internal/child_process.js:891:16)
22:06:31     at Socket.<anonymous> (internal/child_process.js:342:11)
22:06:31     at emitOne (events.js:96:13)
22:06:31     at Socket.emit (events.js:188:7)
22:06:31     at Pipe._handle.close [as _onclose] (net.js:497:12)
22:06:31   killed: false,
22:06:31   code: 255,
22:06:31   signal: null,
22:06:31   cmd: 'php /src/extensions/MobileFrontend/node_modules/resource-modules/src/php/resources.php /src /resources/Resources.php' }
22:06:31 
22:06:31 npm ERR! Linux 4.9.0-0.bpo.6-amd64
22:06:31 npm ERR! argv "/usr/bin/nodejs" "/usr/local/bin/npm" "run-script" "lint:modules"
22:06:31 npm ERR! node v6.11.0
22:06:31 npm ERR! npm  v3.8.3
22:06:31 npm ERR! code ELIFECYCLE
22:06:31 npm ERR! @ lint:modules: `resource-modules ./`
22:06:31 npm ERR! Exit status 1
22:06:31 npm ERR! 
22:06:31 npm ERR! Failed at the @ lint:modules script 'resource-modules ./'.

Details

Related Changes in Gerrit:

Subject	Author	Repo	Branch	Lines +/-
Use npm-php:0.2.0	Legoktm	integration/config	master	+2 -2
docker: Switch npm-php image over to PHP 7	Legoktm	integration/config	master	+9 -3
Switch mwext-MobileFrontend-npm-run-lint-modules-docker to PHP 7	Legoktm	integration/config	master	+2 -0

Customize query in gerrit

Related Objects

Mentioned In: T224997: Update MobileFrontend-npm-run-lint-modules-docker to run node10
Mentioned Here: T224997: Update MobileFrontend-npm-run-lint-modules-docker to run node10

Event Timeline

Jdlrobson created this task.Jun 11 2018, 10:20 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 11 2018, 10:20 PM

I think the tool is executing PHP?? https://github.com/joakin/resource-modules/blob/master/src/php/resources.php is pretty scary. This seems like something that should be done inside MediaWiki itself, maybe a structure test.

https://github.com/joakin/resource-modules/blob/master/src/php/index.ts#L12 is where it shells out.

This is probably running on PHP 5.6 and not 7, so it fails on the new syntax.

Change 439829 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[integration/config@master] Switch mwext-MobileFrontend-npm-run-lint-modules-docker to PHP 7

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

gerritbot added a project: Patch-For-Review.Jun 12 2018, 1:49 AM

Change 439829 merged by jenkins-bot:
[integration/config@master] Switch mwext-MobileFrontend-npm-run-lint-modules-docker to PHP 7

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

Uh, that won't work. Hmm.

Hey! The linter needs the resource modules configuration as much as possible but given that mediawiki's is not in a JSON file like extensions or skins, it needs to run some PHP to get it out. It is unfortunate and ugly, but needed. That's why that is there. If there is a better way to get the resource modules config from mediawiki core it would be great to know to improve the tool.

I've tested locally and it works with PHP 7.0, 7.1 and 7.2, and it fails to run with that exact error with 5.6, as you mentioned.

I've re-checked 439827 but still a failure with the php 5.6 error.

Let me know if there is anything I can do on the tooling side to help out.

I think the tool is executing PHP?? https://github.com/joakin/resource-modules/blob/master/src/php/resources.php is pretty scary. This seems like something that should be done inside MediaWiki itself, maybe a structure test.

When talking to @hashar we figured this would be very useful in core since it actually looks at the code and verifies that messages are used by JavaScript and listed in extension.json and verifies that the dependencies are correct (e.g. defined), something the existing unit tests do not fully cover.

Right now the main issue however is being able to merge in the repo.

Jdlrobson moved this task from To Do to Blocked on Others on the Readers-Web-Kanbanana-Board-Old board.Jun 12 2018, 4:06 PM

Change 440027 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[integration/config@master] docker: Switch npm-php image over to PHP 7

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

gerritbot added a project: Patch-For-Review.Jun 12 2018, 11:23 PM

Change 440027 merged by jenkins-bot:
[integration/config@master] docker: Switch npm-php image over to PHP 7

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

Change 440042 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[integration/config@master] Use npm-php:0.2.0

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

Change 440042 merged by jenkins-bot:
[integration/config@master] Use npm-php:0.2.0

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

The job runs properly now, see https://integration.wikimedia.org/ci/job/mwext-MobileFrontend-npm-run-lint-modules-docker/734/console

It's still failing, due to other reasons apparently.

I would *strongly* recommend moving this into a proper PHPUnit structure test (or JS equivalent), it appears extremely fragile (I'm pretty surprised it hadn't broken earlier), and requires custom CI support to maintain.

In T196956#4274851, @Jhernandez wrote:

Hey! The linter needs the resource modules configuration as much as possible but given that mediawiki's is not in a JSON file like extensions or skins, it needs to run some PHP to get it out. It is unfortunate and ugly, but needed. That's why that is there. If there is a better way to get the resource modules config from mediawiki core it would be great to know to improve the tool.

Don't run it as a nodejs tool, and use a PHPUnit test instead?

Thanks for the fix. We’ll have to fix the normal errors now.

@Legoktm The reason it runs as a node.js tool is because it is doing JS source parsing, and the node environment has the better libraries for JavaScript source to AST parsing and tools. This linter among other things reads the JS sources and identifies patterns and definitions which it then checks against the extension.json definitions to see that they match and that there are no errors.

It is not formally sound as for that we would actually need to run the JS but it is quite helpful and the best we could do at the time, as it has helped us find implicit dependencies on modules that could not be loaded in some situations and have race conditions producing runtime exceptions that we would never know about because we don’t have client side error reporting.

We will revisit in the future and see if we should get rid of it in favor of something else or improve it in some other way.

• Vvjjkkii renamed this task from mwext-MobileFrontend-npm-run-lint-modules-docker failing - node script running as php to b8aaaaaaaa.Jul 1 2018, 1:04 AM

• Vvjjkkii reopened this task as Open.

• Vvjjkkii removed Legoktm as the assignee of this task.

• Vvjjkkii added projects: CheckUser, Connected-Open-Heritage-Batch-uploads (RAÄ-KMB_1_2017-02), Tamil-Sites, Gamepress, Hashtags, Jade, KartoEditor, Language-2018-Apr-June, New-Editor-Experiences, Mail, TCB-Team (now WMDE-TechWish).

• Vvjjkkii updated the task description. (Show Details)

• Vvjjkkii removed subscribers: gerritbot, Aklapper.

CommunityTechBot renamed this task from b8aaaaaaaa to mwext-MobileFrontend-npm-run-lint-modules-docker failing - node script running as php.Jul 2 2018, 8:21 AM

CommunityTechBot closed this task as Resolved.

CommunityTechBot assigned this task to Legoktm.

CommunityTechBot updated the task description. (Show Details)

CommunityTechBot removed projects: TCB-Team (now WMDE-TechWish), Mail, New-Editor-Experiences, Language-2018-Apr-June, KartoEditor, Jade, Hashtags, Gamepress, Tamil-Sites, Connected-Open-Heritage-Batch-uploads (RAÄ-KMB_1_2017-02), CheckUser.

CommunityTechBot added subscribers: gerritbot, Aklapper.

Legoktm mentioned this in T224997: Update MobileFrontend-npm-run-lint-modules-docker to run node10.Jun 4 2019, 3:20 PM

In T196956#4278622, @Jhernandez wrote:

We will revisit in the future and see if we should get rid of it in favor of something else or improve it in some other way.

Did anything come of this? It'd be nice to get rid of this hack, and if the need is more general, implement something for all extensions.