985141 - Native support for virtualenvs in mach

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Gregory Szorc [:gps]]

For whatever reason when I woke up this morning my brain had derived a solution to the mach + virtualenv problem despite me not consciously thinking about it.

Rough draft is as follows.

We make it possible to define virtualenvs inside the mach core. Perhaps we have a @MachVirtualenv decorator or something. We end up with mach maintaining a map of virtualenv names to their settings (which packages to install, etc). We could have a "buildsystem" virtualenv used by the core build system. We could have a "versioncontrol" virtualenv that contains Mercurial, dulwich, code review packages, etc. We could have a "talos" virtualenv that can run Talos. etc.

Supporting multiple virtualenvs is important because things like Talos may rely on separate mozbase package versions than what we have in tree. And, we don't always want to pollute the in-tree virtualenv with other (potentially conflicting) packages. This allows mach commands to branch out and experiment without fear of interfering with other mach commands.

Individual mach commands can then specify which virtualenv (if any) to use. When the command is executed, the mach core will create and populate said virtualenv (if necessary) and then activate that virtualenv.

There is a special case where the virtualenv's python does not match the invoking mach process's (sys.executable). In that case, the mach core will spawn a new Python process using the virtualenv's python binary and will redirect the existing mach command and its arguments, pipes, etc to that process. Alternatively, we could do this for every command. But I don't like the new process overhead here, especially on Windows.

Where to store the virtualenvs is an interesting question. We may not want to store virtualenvs in the objdir because, well, knowing what the objdir is requires executing mozconfigs, looking for environment variables, etc and that code is in the mozbuild package - part of the virtualenv. Hello chicken and egg! I was thinking we could store the virtualenvs under MOZBUILD_STATE_PATH, keyed to the topsrcdir somehow. e.g. /home/gps/mozilla-central and /home/gps/mozilla-inbound would have their own virtualenv "namespaces" in the state dir. We could periodically scan the state dir and prune virtualenvs that belong to defunct source directories.

Anyway, the end state is that mach_bootstrap.py doesn't need to perform gross sys.path adjustment. We move everything to proper virtualenvs and have commands exec the activate_this.py script from within the appropriate virtualenv. This should hopefully eliminate the package compatibility problems we've seen.

I do see a few concerns. First, having commands invoke other mach commands could be problematic if those commands cross a virtualenv boundary. Solution to that is to not allow cross-virtualenv command calls. Another problem is module imports. A lot of mach_commands.py files import packages from the virtualenv. We could roll a demand importer so the actual imports are deferred until the virtualenv is activated. Mercurial has something like this (although it doesn't involve a virtualenv).

What do people think?

Comment 1

[Andrew Halberstadt [:ahal]]

This proposal makes sense to me. Would it play well with people who have their own custom virtualenvs activated while running mach commands?

(In reply to Gregory Szorc [:gps] from comment #0)
> There is a special case where the virtualenv's python does not match the
> invoking mach process's (sys.executable).

Don't we require python 2.7? Or is python 3 also allowed? Otherwise, I think this is very edge case-y and could be YAGNI, at least until we support python 3.

Comment 2

[Gregory Szorc [:gps]]

(In reply to Andrew Halberstadt [:ahal] from comment #1)
> This proposal makes sense to me. Would it play well with people who have
> their own custom virtualenvs activated while running mach commands?
> 
> (In reply to Gregory Szorc [:gps] from comment #0)
> > There is a special case where the virtualenv's python does not match the
> > invoking mach process's (sys.executable).
> 
> Don't we require python 2.7? Or is python 3 also allowed? Otherwise, I think
> this is very edge case-y and could be YAGNI, at least until we support
> python 3.

I think those two points are related :)

I think if you run mach with an activated virtualenv and that virtualenv isn't the one the command is requesting, mach should switch to the proper virtualenv. This seems safest and allows people to run mach commands in more scenarios.

I'm trying to think of scenarios where someone would want to run mach commands from a custom virtualenv. So far, they all end with "modify the virtualenv used by the mach command."

Comment 3

[Andrew Halberstadt [:ahal]]

Have a WIP patch that solves this iteratively on the current system in mozbuild.base rather than mach core.

Comment 4

[Andrew Halberstadt [:ahal]]

Attachment: Bug 985141 - [mozbuild] Remove leading underscore from MozbuildObject._activate_virtualenv, r?#firefox-build-system-reviewers

This function is used all across the tree and should be considered a public API.

Comment 5

[Andrew Halberstadt [:ahal]]

Attachment: Bug 985141 - [mozbuild] Add ability for mach commands to create arbitrary virtualenvs, r?#firefox-build-system-reviewers

Depends on D85045

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/162f07f42162
[mozbuild] Remove leading underscore from MozbuildObject._activate_virtualenv, r=firefox-build-system-reviewers,perftest-reviewers,andi,AlexandruIonescu,rstewart

Comment 7

[Andrew Halberstadt [:ahal]]

Attachment: Bug 985141 - [mozperftest] Fix missed instances of _activate_virtualenv in mozperftest framework, r=#perftest-reviewers

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1747dd37d2c9
[mozperftest] Fix missed instances of _activate_virtualenv in mozperftest framework, r=perftest-reviewers,sparky

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/162f07f42162

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1747dd37d2c9

Comment 11

[Ricky Stewart]

Attachment: Bug 985141 - Allow mach commands to specify what virtualenv they should use.

Now you can pass the virtualenv_name kwarg to the Command decorator which will configure the _virtualenv_manager accordingly.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/330b617b585c
Allow mach commands to specify what virtualenv they should use. r=mhentges,dmajor

Comment 13

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/330b617b585c

Comment 14

[Natalia Csoregi [:nataliaCs]]

Backed out for bustage and test info failures due to missing argument caused on Mozilla-Central:

backout: https://hg.mozilla.org/integration/autoland/rev/6330bdb55f34a780db5dcafc1a0b7f5ee995e98f

push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=f46205a42faecda0e5a73bdcde0a8e1caa0126d9&selectedTaskRun=FTgP5BS_Tq2pc3dGLGlLXQ.0&searchStr=Linux%2Cx64%2Cdebug%2Cstatic-analysis-autotest-linux64-st-autotest%2Fdebug%2CSa AND https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&selectedTaskRun=ArGazHqJR4-JCgAIRIg5FQ.0&revision=f46205a42faecda0e5a73bdcde0a8e1caa0126d9&searchStr=test-info

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=312961084&repo=mozilla-central&lineNumber=1705

[task 2020-08-13T21:54:57.061Z] Error running mach:
[task 2020-08-13T21:54:57.061Z]
[task 2020-08-13T21:54:57.061Z] ['--log-no-times', 'static-analysis', 'autotest', '--intree-tool']
[task 2020-08-13T21:54:57.061Z]
[task 2020-08-13T21:54:57.061Z] The error occurred in the implementation of the invoked mach command.
[task 2020-08-13T21:54:57.061Z]
[task 2020-08-13T21:54:57.062Z] This should never occur and is likely a bug in the implementation of that
[task 2020-08-13T21:54:57.062Z] command.
[task 2020-08-13T21:54:57.062Z] You can invoke |./mach busted| to check if this issue is already on file. If it
[task 2020-08-13T21:54:57.062Z] isn't, please use |./mach busted file static-analysis| to report it. If |./mach busted| is
[task 2020-08-13T21:54:57.062Z] misbehaving, you can also inspect the dependencies of bug 1543241.
[task 2020-08-13T21:54:57.062Z]
[task 2020-08-13T21:54:57.062Z] If filing a bug, please include the full output of mach, including this error
[task 2020-08-13T21:54:57.062Z] message.
[task 2020-08-13T21:54:57.062Z]
[task 2020-08-13T21:54:57.062Z] The details of the failure are as follows:
[task 2020-08-13T21:54:57.062Z]
[task 2020-08-13T21:54:57.062Z] TypeError: init() missing 1 required positional argument: 'virtualenv_name'
[task 2020-08-13T21:54:57.062Z]
[task 2020-08-13T21:54:57.062Z] File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/code_analysis/mach_commands.py", line 1423, in autotest
[task 2020-08-13T21:54:57.062Z] _, config, _ = self._get_config_environment()
[task 2020-08-13T21:54:57.062Z] File "/builds/worker/checkouts/gecko/python/mozbuild/mozbuild/code_analysis/mach_commands.py", line 2418, in _get_config_environment
[task 2020-08-13T21:54:57.062Z] builder = Build(self._mach_context)
[task 2020-08-13T21:54:57.096Z] 21:54:57 WARNING - setting return code to 2
[task 2020-08-13T21:54:57.096Z] 21:54:57 FATAL - 'mach static-analysis autotest --intree-tool' did not run successfully. Please check log for errors.
[task 2020-08-13T21:54:57.096Z] 21:54:57 FATAL - Running post_fatal callback...
[task 2020-08-13T21:54:57.097Z] 21:54:57 FATAL - Exiting -1

Comment 15

[Ricky Stewart]

Fixed and re-landed.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2149ac954ff
Allow mach commands to specify what virtualenv they should use. r=mhentges,dmajor

Comment 17

[Kagami Rosylight [:saschanaz] (they/them)]

The patch still breaks clang-format: https://searchfox.org/mozilla-central/rev/358cef5d1a87172f23b15e1a705d6f278db4cdad/python/mozbuild/mozbuild/code_analysis/mach_commands.py#2418

Ah okay, it just didn't reach mozilla-central yet.

Comment 18

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/d2149ac954ff

Comment 19

[Alexandru Michis [:malexandru]]

Ricky, this is still happening even after d2149ac954ff landed, could you please take a look?
https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&searchStr=test-info&revision=605c404fbd80c67e1127ac054b8bec6742bd748f

Comment 21

[Ricky Stewart]

Fixing as bug 1659113.

Closing this bug as fixed because it appears the patch has made it to central.

Comment 22

[Intermittent Failures Robot]

8 failures in 3601 pushes (0.002 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-central: 8

Platform breakdown:

• lint: 8

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=985141&startday=2020-08-10&endday=2020-08-16&tree=all

Comment 23

[Mitchell Hentges [:mhentges] 🦀]

Comment on attachment 9168582 [details]
Bug 985141 - Allow mach commands to specify what virtualenv they should use.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Needed to uplift https://phabricator.services.mozilla.com/D88296.
  In D88296, we use the "virtualenv_name" parameter (added in this patch) to specify which which virtualenv to use, instead of using pipenv.
  Associated patches are needed so this patch lands cleanly.
• User impact if declined: None
• Fix Landed on Version: 81
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): None
• String or UUID changes made by this patch:

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9166396 [details]
Bug 985141 - [mozbuild] Remove leading underscore from MozbuildObject._activate_virtualenv, r?#firefox-build-system-reviewers

Needed to better support running mach on newer macOS releases. Approved for 78.8esr.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/eb4d9f7b6378
https://hg.mozilla.org/releases/mozilla-esr78/rev/ce1a5d0f3458
https://hg.mozilla.org/releases/mozilla-esr78/rev/fb60f781ba8a