Page MenuHomePhabricator

Maintenance script designed for run.php <class> syntax cannot be executed in Wikimedia production
Open, Needs TriagePublic

Description

Maintenance script designed for run.php <class> syntax cannot be executed in Wikimedia production, as we had discovered today following T317375#8856530. mwscript insists on treating its parameter as a file name.

Example script: https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/DiscussionTools/+/2321e645f479b120527de91910a8ecec775d0a55/maintenance/NewTopicOptOutActiveUsers.php

I expected the following command to work: mwscript MediaWiki.Extension.DiscussionTools.Maintenance.NewTopicOptOutActiveUsers --wiki=fiwiki --dry-run

This script can't be executed by file name, lacking the various wrappers that made it possible. I thought that this is the future, but I guess it's not. Do I need to add the legacy wrappers?

Event Timeline

@matmarex : mwscript should be able to find the script by both class name and path (relative from $IP) so mwscript extensions/DiscussionTools/maintenance/NewTopicOptOutActiveUsers.php --wiki=Foo should work. Does it work?

It should also work with class name but maybe I messed up something there.

You don't need to add the legacy wrapper, it wouldn't make a difference. mwscript uses run.php internally.

@Ladsgroup: is run.php your work?

Yes and I got email for this because of T326800#8857359 so I'm aware.

Tried it:

ladsgroup@mwmaint1002:~$ mwscript extensions/DiscussionTools/maintenance/NewTopicOptOutActiveUsers.php --wiki=fiwiki --dry-run
ERROR: The script file '/srv/mediawiki/php-1.41.0-wmf.8/extensions/DiscussionTools/maintenance/NewTopicOptOutActiveUsers.php' cannot be executed using MaintenanceRunner.
It does not set $maintClass and does not return a class name.

You need to add $maintClass still (but not the rest). That's not legacy AFAIK.

Yeah, mwscript tries to prepend path when it sees a class name. I fix that but without the $maintClass it's going to throw the same error. Maybe @daniel can confirm that's still needed (he did the run.php in core).

You need to add $maintClass still (but not the rest). That's not legacy AFAIK.

It is needed in order to run scripts by file name. It is not needed to run scipts by class name.

As far as I can tell, the problem is that mwscript expects the script to always be given as a file name, it doesn't support class names...

By the way, in run.php, it's not necesasary to give the full namespace of an extension script, if it follows conventions. Instead of MediaWiki.Extension.DiscussionTools.Maintenance.NewTopicOptOutActiveUsers you can use DiscussionTools:NewTopicOptOutActiveUsers.

Perhaps mwscript could look for the ":" and avoid the path prefix if that is given?

Fixing the class name should be fine, it should stop appending path if there is no / in the argument. It's fine and easily fixable.

You need to add $maintClass still (but not the rest). That's not legacy AFAIK.

It is needed in order to run scripts by file name. It is not needed to run scipts by class name.

I don't like inconsistency, specially since running scripts happen by sysadmins (wikimedia or third party) and writing the script happens by a developer. Making it take the main class in the file if $maint doesn't exist shouldn't be hard (famous last words?)

Looking at the code to fix it, I realized I actually added support to avoid prepending maint/ but that's only on ":". In other words, I just tried this and it worked:

ladsgroup@mwmaint1002:~$ mwscript DiscussionTools:NewTopicOptOutActiveUsers --wiki=fiwiki --dry-run
Dry run:
Found 490 active users with enough edit
....

I can also make sure to avoid it if there are two dots or more in the value.

I thought that the : version did not work yesterday either, I vaguely recall someone on IRC trying it and saying it also had problems. Maybe we were too confused and it actually would have worked?

I thought that the : version did not work yesterday either, I vaguely recall someone on IRC trying it and saying it also had problems. Maybe we were too confused and it actually would have worked?

The MaintenanceRunner adds MediaWiki\Extension\Maintenance to it so you must only provide name of the class and nothing more. Maybe you added DiscussionTools:Maintenance\NewTopicOptOutActiveUsers and that didn't work (that's actually what I did first and errored and I saw it tried MediaWiki\DiscussionTools\Maintenance\Maintenance\NewTopicOptOutActiveUsers.

Change 920788 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[operations/mediawiki-config@master] mwscript: Avoid prepending maintenance/ if >= 2 dots in argument

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

I don't like inconsistency, specially since running scripts happen by sysadmins (wikimedia or third party) and writing the script happens by a developer. Making it take the main class in the file if $maint doesn't exist shouldn't be hard (famous last words?)

Make a patch and I'll merge it :)

Change 920788 merged by jenkins-bot:

[operations/mediawiki-config@master] mwscript: Avoid prepending maintenance/ if >= 2 dots in argument

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

Mentioned in SAL (#wikimedia-operations) [2023-05-31T10:25:18Z] <ladsgroup@deploy1002> Started scap: Backport for [[gerrit:920788|mwscript: Avoid prepending maintenance/ if >= 2 dots in argument (T336819)]]

Mentioned in SAL (#wikimedia-operations) [2023-05-31T10:27:11Z] <ladsgroup@deploy1002> ladsgroup: Backport for [[gerrit:920788|mwscript: Avoid prepending maintenance/ if >= 2 dots in argument (T336819)]] synced to the testservers: mwdebug2002.codfw.wmnet, mwdebug1002.eqiad.wmnet, mwdebug1001.eqiad.wmnet, mwdebug2001.codfw.wmnet

Mentioned in SAL (#wikimedia-operations) [2023-05-31T10:34:09Z] <ladsgroup@deploy1002> Finished scap: Backport for [[gerrit:920788|mwscript: Avoid prepending maintenance/ if >= 2 dots in argument (T336819)]] (duration: 08m 50s)

A couple more clean ups can be done here. I'll take care of them soonTM