1695312 - Make mach command modules be loaded selectively

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement, P3)

Description

[Mitchell Hentges [:mhentges] 🦀]

Currently, to register mach commands, we import all necessary mach_commands.py files, interpret them and their dependents, and run all @CommandProvider decorators.
This causes two downsides:

• It slows the running of mach, because all modules are loaded, even those for commands who are unused. On my machine, mach takes 420ms to start up, and roughly half of that (~225ms!) is spent interpreting all these unnecessary modules
• We attempt to import modules before activating a command's virtualenv.

Additionally, there's the general issue of mach commands being registered simply by having their file imported. There's some action-at-a-distance implicit work happening there which is tough to follow. By registering commands in a pythonic way, we improve tooling support and usability.

Comment 1

[Mitchell Hentges [:mhentges] 🦀]

Alex L has done some good work centralizing our MACH_COMMANDS, but there's still a fair amount of work to go to get lazy-loading working:

1. First, refactor cases that depend on all mach modules being loaded at once:
   • conditions may effect whether or not commands are shown as disabled in ./mach --help. We should ensure that, instead, the conditions option only affects whether the current command to-be-run is enabled or not.
   • settings: move these to a global spot (probably to mach_initialize.py) so that they're always available for all Mach commands to use.
2. Instead of importing all commands' modules, instead only import the one associated with the current command-to-be-run.
3. Finally add some tests to verify correctness of the MACH_COMMANDS "command-to-module" reference list:
   • All commands must point to a file containing their associated @Command function
   • All modules (MACH_COMMANDS.values()) must have all @Command functions associated with an entry in MACH_COMMANDS.

Comment 2

[Alex Hochheiden [:ahochheiden]]

Attachment: Bug 1695312 - Activate the virtualenv associated with a mach command much earlier r?#build

This activated virtualenv for a command is managed
CommandSiteManager and it is passed down to where it was activated
before to prevent a second, redundant, activation.

Since this activation is happening before we can determine where the
objdir will be, we have to store the virtualenvs elsewhere. They will
now live in the same directory as the mach virtualenv has always
lived.

Comment 3

[Alex Hochheiden [:ahochheiden]]

Attachment: Bug 1695312 - Selectively load only the mach command modules needed for the command about to be run r?#build

This makes loading almost all commands faster, since only one module
filed is loaded rather than all of them. There is one main exception,
dealing with 'help'. Running ./mach help (or -h or --help) requires
the description text for every command, so every module file is still
loaded.

We could expand this improvement here to consolidate all commands and
their parameters in this MACH_COMMANDS dict, but the only two benefits
are improving help, and not having two places where the commands are
specified (their file, and this dict).

There's a lot of extra work needed to do that, especially for handling
sub commands, and it did not seem worth the cost for the benefit at this
time.

Depends on D180499

Comment 4

[Alex Hochheiden [:ahochheiden]]

Attachment: Bug 1695312 - Add a virtualenv site for all `try` commands r?#build

This isn't strictly necessary for this work, but I know it's necessary
for upcoming work, so I may as well take care of it now, since it's
definitely related.

Depends on D180501

Comment 5

[Alex Hochheiden [:ahochheiden]]

Attachment: Bug 1695312 - Updated the `requirements.in` file so that we only vendor `mach` and `build` dependencies r?#build

The build must work without having to download anything, as such,
everything needed by the build site (and by extension the mach site)
must be vendored. To simplify dependency conflicts between other sites,
everything else should not be vendored. If a new dependency is needed
for say lint, it should be specified as a pypi:<X>==<Y> in the
lint.txt file.

There may be vendored dependencies specified in there. Those can be
modified to PyPi dependencies if need be. They are only pointing to the
vendored versions because they are currently compatible, and not having
to download from PyPi is slightly faster on first-time site creation.

Depends on D180502

Comment 6

[Alex Hochheiden [:ahochheiden]]

Attachment: Bug 1695312 - Move the location of site/command virtualenvs out of the objdir and into the state dir r?#build

This is necessary for activating the site/command virtualenvs earlier,
since we can't determine what the objdir will be very early on in the
mach process intialization. We do know where the state dir is, and how
to get to the state dir for a specific topsrcdir, so we can use that
instead. This is already where the mach virtualenv lives anyway.

Comment 7

[Alex Hochheiden [:ahochheiden]]

Attachment: Bug 1695312 - Make `mach vendor` and `mach vendor rust` use the `vendor` virtualenv r?#build

Depends on D181029

Comment 8

[Alex Hochheiden [:ahochheiden]]

Attachment: Bug 1695312 - Sort the `MACH_COMMANDS` dict alphabetically r?#build

Depends on D181030

Comment 9

[Alex Hochheiden [:ahochheiden]]

Attachment: Bug 1695312 - Add missing commands to the `MACH_COMMANDS` dict r?#build

Also add the associated virtualenv names to each command that has one in
preparation for loading the virtualenvs earlier (next patch).

Depends on D181031

Comment 10

[Phabricator Automation]

Comment on attachment 9339214 [details]
Bug 1695312 - Move the location of site/command virtualenvs out of the objdir and into the state dir r?#build

Revision D181029 was moved to bug 1838763. Setting attachment 9339214 [details] to obsolete.

Comment 11

[Alex Hochheiden [:ahochheiden]]

Attachment: Bug 1695312 - Vendor `fluent.runtime` at version `0.4.0` r?#build

This is needed so that we can remove the virtualenv_name parameter
from the @Command decorators because there's a subcommand for a
command that specifically needs this module. The easiest workaround
is to vendor this and use it in the common venv and remove the
subcommand's unique virtualenv alltogether.

Depends on D180501

Comment 12

[Alex Hochheiden [:ahochheiden]]

Attachment: Bug 1695312 - Remove the `virtualenv_name` parameter from all `@Command` decorators r?#build

Depends on D182279

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d2a2dd4790b
Sort the `MACH_COMMANDS` dict alphabetically r=firefox-build-system-reviewers,glandium
https://hg.mozilla.org/integration/autoland/rev/a35da3fce458
Add missing commands to the `MACH_COMMANDS` dict r=firefox-build-system-reviewers,glandium
https://hg.mozilla.org/integration/autoland/rev/eab5cdaf4bc9
Activate the virtualenv associated with a mach command much earlier r=firefox-build-system-reviewers,glandium
https://hg.mozilla.org/integration/autoland/rev/2839c2d35459
Selectively load only the mach command modules needed for the command about to be run r=firefox-build-system-reviewers,glandium

Comment 14

[Sandor Molnar[:smolnar]]

Backed out for causing lint failures

Backout link: https://hg.mozilla.org/integration/autoland/rev/3ae2f464a14e6453ce1d7402edfe00be45d2c781

Push with failures

Failure log -> Exception: Cannot use MACH_BUILD_PYTHON_NATIVE_PACKAGE_SOURCE="system" for any sites other than ('mach', 'build', 'common'). The current attempted site is "vendor"

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56f3d3fc806f
Sort the `MACH_COMMANDS` dict alphabetically r=firefox-build-system-reviewers,glandium
https://hg.mozilla.org/integration/autoland/rev/06aefe3418e1
Add missing commands to the `MACH_COMMANDS` dict r=firefox-build-system-reviewers,glandium
https://hg.mozilla.org/integration/autoland/rev/6123c201f979
Activate the virtualenv associated with a mach command much earlier r=firefox-build-system-reviewers,glandium
https://hg.mozilla.org/integration/autoland/rev/e5687aa10124
Selectively load only the mach command modules needed for the command about to be run r=firefox-build-system-reviewers,glandium

Comment 16

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/56f3d3fc806f
https://hg.mozilla.org/mozilla-central/rev/06aefe3418e1
https://hg.mozilla.org/mozilla-central/rev/6123c201f979
https://hg.mozilla.org/mozilla-central/rev/e5687aa10124

Comment 17

[Natalia Csoregi [:nataliaCs]]

Backed out as per request on Element, #sheriffs.

• backout: https://hg.mozilla.org/mozilla-central/rev/dbc796697941b3200deec64bdb3bd331743cb0fe

Comment 18

[Alex Hochheiden [:ahochheiden]]

Attachment: Bug 1695312 - Add the ability for `dispatch` to ad-hoc load command modules that aren't already loaded r?#build

This is really just shuffling a bunch of things around. None of the
'load_*' member functions of the Mach class actually needed to be
member functions. They can all be static so that they can be used
anywhere. That combined with moving all the other 'mach_command' logic
to a different file, allows us to load the module for any command so
that we can successfully dispatch it.

Depends on D180500

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/21f435391279
Sort the `MACH_COMMANDS` dict alphabetically r=firefox-build-system-reviewers,glandium
https://hg.mozilla.org/integration/autoland/rev/8b4be21eacef
Add missing commands to the `MACH_COMMANDS` dict r=firefox-build-system-reviewers,glandium
https://hg.mozilla.org/integration/autoland/rev/1932f8f581f7
Activate the virtualenv associated with a mach command much earlier r=firefox-build-system-reviewers,glandium
https://hg.mozilla.org/integration/autoland/rev/c36eec0834f4
Selectively load only the mach command modules needed for the command about to be run r=firefox-build-system-reviewers,glandium
https://hg.mozilla.org/integration/autoland/rev/ef16e8135fe1
Add the ability for `dispatch` to ad-hoc load command modules that aren't already loaded r=firefox-build-system-reviewers,glandium

Comment 20

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/21f435391279
https://hg.mozilla.org/mozilla-central/rev/8b4be21eacef
https://hg.mozilla.org/mozilla-central/rev/1932f8f581f7
https://hg.mozilla.org/mozilla-central/rev/c36eec0834f4
https://hg.mozilla.org/mozilla-central/rev/ef16e8135fe1

Comment 21

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/21f435391279
https://hg.mozilla.org/mozilla-central/rev/8b4be21eacef
https://hg.mozilla.org/mozilla-central/rev/1932f8f581f7
https://hg.mozilla.org/mozilla-central/rev/c36eec0834f4
https://hg.mozilla.org/mozilla-central/rev/ef16e8135fe1