Page MenuHomePhabricator

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

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 Gerrit Patches:

Event Timeline

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.

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

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.

Jdlrobson triaged this task as High priority.Jun 12 2018, 4:05 PM
Jdlrobson added a subscriber: hashar.

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.

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

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

Legoktm closed this task as Resolved.Jun 13 2018, 1:41 AM
Legoktm claimed this task.

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.

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 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 added subscribers: gerritbot, Aklapper.

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.

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.

Potentially followed up in T224997