Closed Bug 985141 Opened 7 years ago Closed 10 months ago

Native support for virtualenvs in mach

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox-esr78 fixed)

RESOLVED FIXED
Tracking Status
firefox-esr78 --- fixed

People

(Reporter: gps, Assigned: rstewart)

References

Details

Attachments

(3 files, 1 obsolete file)

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?
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.
(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."
See Also: → 1200293
Depends on: 1437593
Product: Core → Firefox Build System

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

Component: Mach Core → General

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

Assignee: nobody → ahal
Status: NEW → ASSIGNED
Pushed by ahalberstadt@mozilla.com:
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
Keywords: leave-open
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1747dd37d2c9
[mozperftest] Fix missed instances of _activate_virtualenv in mozperftest framework, r=perftest-reviewers,sparky
Attachment #9166398 - Attachment description: Bug 985141 - [mozbuild] Add ability for mach commands to create arbitrary virtualenvs → Bug 985141 - [mozbuild] Add ability for mach commands to create arbitrary virtualenvs, r?#firefox-build-system-reviewers

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

Attachment #9166398 - Attachment is obsolete: true
Pushed by rstewart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/330b617b585c
Allow mach commands to specify what virtualenv they should use. r=mhentges,dmajor

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

Flags: needinfo?(ahal)

Fixed and re-landed.

Assignee: ahal → rstewart
Flags: needinfo?(ahal)
Pushed by rstewart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d2149ac954ff
Allow mach commands to specify what virtualenv they should use. r=mhentges,dmajor
Duplicate of this bug: 1659020
Regressions: 1659113

Fixing as bug 1659113.

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

Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Flags: needinfo?(rstewart)
Resolution: --- → FIXED
Regressions: 1659154

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:
Attachment #9168582 - Flags: approval-mozilla-esr78?
Attachment #9166396 - Flags: approval-mozilla-esr78?
Attachment #9166673 - Flags: approval-mozilla-esr78?

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.

Attachment #9166396 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+
Attachment #9166673 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+
Attachment #9168582 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+
You need to log in before you can comment on or make changes to this bug.