Page MenuHomePhabricator

resource-modules linting is failing and blocking merging patches
Closed, ResolvedPublic3 Story Points

Description

Example failure log: https://integration.wikimedia.org/ci/job/mwext-MobileFrontend-npm-run-lint-modules-docker/734/console

It seems like some files changed in mediawiki core and as such the linter is confused about mw.msg, which it finds defined in resources/src/mediawiki/mediawiki.base.js but that is not included in the default files list of the linter.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 13 2018, 10:41 AM
Niedzielski added a subscriber: ovasileva.

@ovasileva, @Jdlrobson, this is blocking our code merges so I'm pulling it into the sprint.

ovasileva triaged this task as High priority.Jun 13 2018, 2:37 PM
Jdlrobson set the point value for this task to 3.Jun 13 2018, 5:20 PM

Change 440171 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Fix: upgrade resource-modules linter

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

Niedzielski removed Niedzielski as the assignee of this task.Jun 13 2018, 5:51 PM

Change 440171 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Fix: upgrade resource-modules linter

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

Thanks all. Any follow up work necessary, here?

@Jdlrobson, transition work tracked in T197251.

Jdlrobson closed this task as Resolved.Jun 14 2018, 5:27 PM
Jdlrobson claimed this task.

Thanks!

Jdlrobson reopened this task as Open.EditedJun 25 2018, 9:41 PM

This is back again.
A new issue showed up today in resource-modules blocking merges.

Error: Dependency problems in file: resources/mobile.talk.overlays/TalkSectionOverlay.js:
21:10:58   Required mw.msg defined in file resources/src/mediawiki.base/mediawiki.base.js not found in any dependencies

Given the recent change to the library, I figured we had muted core warnings, so looks like some more work here is needed.

Jdlrobson removed Jdlrobson as the assignee of this task.Jun 25 2018, 9:41 PM

@Niedzielski PR merged and resource-modules@2.1.0 released.

Given the recent change to the library, I figured we had muted core warnings, so looks like some more work here is needed.

That is not possible. In short, the linter has this feature:

  • Global mw namespaces/usages
    • Tracks global definitions of mw.X = { ... }, mw.X.Y = ... and $.extend( mw.X, { Y: ... } ), from the extension and core.
    • Tracks usages of mw.X in sources
    • Complains when used mw.X globals are not in previously defined scripts in ResourceModules

Which means, for example, that if you use mw.RegExp in your frontend sources, but
the resource module mediawiki.RegExp is not found in the dependency tree of resource modules in extension.json, then you get an error.

That error is the one you write up there, it is not an error in core. It is an error in your dependencies because you didn't specify the mediawiki.RegExp dependency on your extension.json, which is defined in a file in core.

Now, mediawiki has a set of files/modules that are added unconditionally from the source, and the linter needs to know about them. Since they are not marked in Resources.php in any way, and we don't have a good way to get the default files from the linter, we maintain a list of default mediawiki files in the code: https://github.com/joakin/resource-modules/blob/master/src/lint/global-dependencies.ts#L10-L19

Since in the last month default files have been moved around in core and changed folders and renamed, the default files list doesn't contain them any more and the linter can't find the file in Resources.php so it complains about them.

There is no way to avoid this right now without disabling the global variables dependency checking feature, which is important and has saved us from untrackable JS exceptions in the past because of race conditions in the dependency tree.

The proper solution to make the linter immune to changes to file names and paths in core is to find a way to get the list of default files from core, instead of specifying them manually in the sources. That is basically impossible right now sadly. We could probably move to getting the files from the jquery, mediawiki and mediawiki.base from the core configuration (because they are added by getStartupModules in php), and add startup.js to them (because it is added manually). The thing is there are then files that are always there because they get added by PHP code all over, like mediawiki.template and others, so it is impossible to get a list of default files because they are generated at runtime. In the end there will need to be a list to maintain of default modules at the very least.

I'd recommend you and the rest go read the list of features that this tool provides. If it is found not useful or worthy of the cost, then remove it from the extension and deprecate the CI job.

For now I'll be adding @Niedzielski as a maintainer and owner of the npm package so that he can perform the fixes he's been doing and unblock the team without me. I'll be around for consulting too of course.

I see that T197251: Transition mw-node-qunit and resource-modules to wikimedia/ is around, which is good. I've subscribed there to help with anything you need.

Jdlrobson closed this task as Resolved.Jun 28 2018, 3:01 PM

All good!

Jdlrobson reopened this task as Open.Jun 28 2018, 9:25 PM

and... back again.
https://integration.wikimedia.org/ci/job/mwext-MobileFrontend-npm-run-lint-modules-docker/829/console

Error: Dependency problems in file: resources/mobile.init/BetaOptinPanel.js:
20:58:07   Required mw.user defined in file resources/src/startup/mediawiki.js not found in any dependencies
Vvjjkkii renamed this task from resource-modules linting is failing and blocking merging patches to i4aaaaaaaa.Jul 1 2018, 1:04 AM
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed the point value for this task.
Vvjjkkii removed subscribers: gerritbot, Aklapper.
CommunityTechBot renamed this task from i4aaaaaaaa to resource-modules linting is failing and blocking merging patches.Jul 2 2018, 8:01 AM
CommunityTechBot set the point value for this task to 3.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added subscribers: gerritbot, Aklapper.
Jdlrobson closed this task as Resolved.Jul 3 2018, 5:09 PM