Closed Bug 1473498 Opened 6 years ago Closed 5 years ago

[mach] Add support for Python 3

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: davehunt, Assigned: ahal)

References

Details

User Story

To work on this bug you will need to install and configure Mercurial, which will enable you to download the Firefox source code. It will also be used to commit your changes locally and prepare a patch for review. See the installation guide at https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/installing.html for help getting Mercurial on your system. Once installed, there are some extensions we recommend installing, details of these can be found here: https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/extensions.html

To clone the Firefox source code we recommend using the unified repository. Details of this and how to create a bookmark for your work can be found here: https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/unifiedrepo.html

As this bug relates to Python 3, you will need to have this installed on your system. Our continuous integration is currently using Python 3.5, so for best results we recommend using the same version locally. There are a number of ways to install Python, and they vary depending on your environment. We suggest reading over the guides at http://docs.python-guide.org/en/latest/starting/installation/#python-3-installation-guides to find the best method for you. Note that we also need to maintain support for Python 2, so you'll also need to have Python 2.7 installed.

Whilst we're moving towards adding support for Python 3, we've disabled any tests that fail against this version. This means that in order to work on this bug you will need to enable the tests. This can be done by removing "skip-if = python == 3" from the manifest file in `python/mach/mach/test/python.ini`

To run the tests against Python 3, execute the following command:

```
mach python-test --python=3.5 python/mach
```

Work through the test failures, and update the tests and source code for the package to simultaneously support Python 2.7 and Python 3.5. You can always check that your changes have not inadvertently broken support for Python 2 by using `--python=2.7` on the command line. 

If you're new to Python 3, the guide at https://docs.python.org/3/howto/pyporting.html may help you to get started with understanding the differences. To assist with supporting both Python 2 and 3, we have vendored the "six" package. You can read more about this and how to use it at https://pythonhosted.org/six/

Once the package supports Python 3 and the tests pass, bump the version number in `python/mach/setup.py` to "1.0.0", and update the classifiers so they reflect the versions of Python that we now support.

When your patch(es) are ready, you will need to push them for review. We are use MozReview for this, and instructions for how to prepare your machine and how to structure your commit messages can be found in our guide: https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html

There may be some review issues to address, but once your contribution has been approved and subsequently landed, we will release the new version of the package!

Attachments

(17 files, 5 obsolete files)

Bug 1473498 - [mach] Pull tests out of make check
47 bytes, text/x-phabricator-request
Details | Review
Bug 1473498 - [mach] Enable test_dispatcher with python 3
47 bytes, text/x-phabricator-request
Details | Review
Bug 1473498 - [mach] Enable test_entry_points with Python 3
47 bytes, text/x-phabricator-request
Details | Review
Bug 1473498 - [mach] Enable test_config with Python 3
47 bytes, text/x-phabricator-request
Details | Review
Bug 1473498 - [mach] Enable test_logger with Python 3
47 bytes, text/x-phabricator-request
Details | Review
Bug 1473498 - [mach] Support running mach commands with python 3
47 bytes, text/x-phabricator-request
Details | Review
Bug 1473498 - [mach] Fix Python 3 decorator compatibility
47 bytes, text/x-phabricator-request
Details | Review
Bug 1473498 - Fixing Py3 compatibility errors reachable from testing/mach_commands.py
47 bytes, text/x-phabricator-request
Details | Review
Bug 1473498 - Don't add backport libraries to the Py3 environment
47 bytes, text/x-phabricator-request
Details | Review
Bug 1473498 - Fix Py3 compatibility issue in python/mozbuild/mozbuild/testing.py
47 bytes, text/x-phabricator-request
Details | Review
Bug 1473498 - [mach] Fix Py3 compatibility issues in telemetry.py
47 bytes, text/x-phabricator-request
Details | Review
Bug 1473498 - Fix Python 3 environment variables with subprocess
47 bytes, text/x-phabricator-request
Details | Review
Bug 1473498 - More support for py3.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1473498 - [mozbuild.mozconfig] Improve error message in MozconfigLoadException
47 bytes, text/x-phabricator-request
Details | Review
Bug 1473498 - [mozbuild.util] Don't use 'patch_main' hack with Python 3.4+
47 bytes, text/x-phabricator-request
Details | Review
Bug 1473498 - [mozbuild.base] Fix Python 3 incompatibilities in mozconfig error handling, r?glandium
47 bytes, text/x-phabricator-request
Details | Review
Bug 1473498 - [configure] Fix ImportError in util.configure under Python 3, r?glandium
47 bytes, text/x-phabricator-request
Details | Review
Without dropping support for legacy Python, we need to add support for Python 3 to mach.
Keywords: in-triage
Keywords: in-triage
Out of curiosity: Why “without dropping support for legacy python”? It’s much easier to port to Python 3 directly, and legacy only has 1½ years left to live.(†2020-01-01)
Blocks: 1555762

The code base is too large to convert all at once, so we need to do it piece by piece. Mach is a library that almost all Python in-tree depends on, so it will need to support both until the migration has finished.

This test is working with minimal effort. Let's get it running to prevent
future regressions.

Depends on D36097

I'll assign to myself for now (at least to get most of the above landed), but please reach out if anyone wants to help push on this.. we can coordinate.

Assignee: nobody → ahal
Status: NEW → ASSIGNED

I've run into some walls.. since mach_commands.py depend on MachCommandBase which uses a bunch of other stuff in mozbuild, I'm now hitting bytes vs str errors in init.configure. This is quickly getting into territory I'm not comfortable with and might need a build peer to either fix or figure out how we can not depend on it.

I'll still try to get the attached patches landed though (except the last two).

Keywords: leave-open
Pushed by ahalberstadt@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2bd4fed10ef7 [mach] Pull tests out of make check r=firefox-build-system-reviewers,mshal https://hg.mozilla.org/integration/autoland/rev/54ea59c8c99d [mach] Enable test_dispatcher with python 3 r=firefox-build-system-reviewers,chmanchester https://hg.mozilla.org/integration/autoland/rev/152598dd91a8 [mach] Enable test_entry_points with Python 3 r=firefox-build-system-reviewers,chmanchester https://hg.mozilla.org/integration/autoland/rev/7123c3146d81 [mach] Enable test_config with Python 3 r=firefox-build-system-reviewers,chmanchester https://hg.mozilla.org/integration/autoland/rev/3416303a9b4a [mach] Enable test_logger with Python 3 r=firefox-build-system-reviewers,chmanchester
Depends on: 1563797

Comment on attachment 9075823 [details]
Bug 1473498 - Vendor backports.shutil_which

Revision D36838 was moved to bug 1563797. Setting attachment 9075823 [details] to obsolete.

Attachment #9075823 - Attachment is obsolete: true
Attachment #9074348 - Attachment description: Bug 1473498 - Support both python 2 and 3 in mach → Bug 1473498 - [mach] Fix Python 3 decorator compatibility
Pushed by ahalberstadt@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0c04c1b1f297 Fixing Py3 compatibility errors reachable from testing/mach_commands.py r=gbrown https://hg.mozilla.org/integration/autoland/rev/a5a6ea416ec3 Don't add backport libraries to the Py3 environment r=Callek https://hg.mozilla.org/integration/autoland/rev/a75643abc92c Fix Py3 compatibility issue in python/mozbuild/mozbuild/testing.py r=nalexander
Attachment #9074348 - Attachment description: Bug 1473498 - [mach] Fix Python 3 decorator compatibility → Bug 1473498 - Fix Python 3 environment variables with subprocess
Attachment #9074348 - Attachment description: Bug 1473498 - Fix Python 3 environment variables with subprocess → Bug 1473498 - [mach] Fix Python 3 decorator compatibility

On Windows in Python 2, the subprocess module requires the use of bytes with
the 'env' argument. For that reason, we would sometimes use byte strings with
'os.environ' like so:

os.environ[b"FOO"] = b"bar"

However, this is a failure with Python 3 as 'os.environ' must only be used with
the text type. This patch removes the usage of byte literals in 'os.environ'
and instead encodes 'env' to byte strings at the subprocess call site.

Pushed by ahalberstadt@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c4a97f2e3303 [mach] Fix Python 3 decorator compatibility r=firefox-build-system-reviewers,chmanchester https://hg.mozilla.org/integration/autoland/rev/dd1bb21a2271 [mach] Fix Py3 compatibility issues in telemetry.py r=firefox-build-system-reviewers,chmanchester
Blocks: 1567642
Attachment #9075698 - Attachment is obsolete: true
Pushed by ahalberstadt@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/05c004d597d2 Fix Python 3 environment variables with subprocess r=glandium

This patch makes BuildEnvironmentNotFoundException a subclass of AttributeError as well, because hasattr in
python3 no longer catches all tracebacks but only AttributeError, and we use both hasattr and
BuildEnvironmentNotFoundException to guard against a handful of buildconfig variables in a few places
where it is OK to not have a buildenvironment.

We also use universal_newlines in real_host in init.configure (since I found
that fix while working on the AttributeError one) so that we get the right string type back from the process call

Lastly this patch also uses BytesIO for calling into a ReducedConfigureSandbox as its stdout and stderr pipes,
This ensures that related code handling the sandbox doesn't complain about getbuffer() missing in StringIO in py3.

Pushed by jwood@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6ff6e514f596 More support for py3. r=firefox-build-system-reviewers,mshal
Attachment #9080031 - Attachment description: Bug 1473498 - Convert config.status back to unicode in Python 3 → Bug 1473498 - Fix byte string errors with Python 3 in configenvironment.py, r?glandium
Attachment #9080031 - Attachment description: Bug 1473498 - Fix byte string errors with Python 3 in configenvironment.py, r?glandium → Bug 1473498 - Fix byte string errors with Python 3 in configenvironment.py

Ok, I've confused myself a bunch of times and have too many revisions going.. so commenting here to clarify the current state of affairs.

First, fixing this comment in D40504 causes errors in configure (see comment below that one). So there are three options here:

  1. Leave that encode at the end there despite not encoding config.status, i.e ignore this issue (sounds like this might be a no-go.. but figured I'd list it).
  2. Fix the underlying build problems. This is probably the most correct approach, but I don't feel qualified to tackle it (nor do I have that much time). This will likely delay the migration by weeks or even months. Maybe a build peer would be able to solve these more quickly than I think.
  3. Keep enforcing bytes in config.status and fix the encoding issues on the other side (e.g in configenvironment.py and whatever mach commands use it).

If we decide to do option #3, then we have two further options:

3A. Assume anytime we interface with a value from config.status, it's going to be bytes and update our code accordingly. D36912 started going down this route, but I started hitting issues in mach_commands.py files themselves. I'm not sure how deep the error stack goes, maybe it won't be too bad. Long term I think this approach will end up being annoying, but it might also be the cleanest for now.

3B. Recursively decode everything that comes out of config.status in configenvironment.py right away. This is basically what my initial patch did way back when.

Mike, which path would you like to take here?

Flags: needinfo?(mh+mozilla)

Looks like 3A also breaks configure:

 0:03.57   File "/home/ahal/dev/mozilla-central/python/mozbuild/mozbuild/frontend/reader.py", line 1057, in read_mozbuild
 0:03.57     raise bre
 0:03.57 mozbuild.frontend.reader.BuildReaderError:
 0:03.57 ==============================
 0:03.57 FATAL ERROR PROCESSING MOZBUILD FILE
 0:03.57 ==============================
 0:03.57 
 0:03.57 The error occurred while processing the following file:
 0:03.57 
 0:03.57     /home/ahal/dev/mozilla-central/build/templates.mozbuild
 0:03.57 
 0:03.57 This file was included as part of processing:
 0:03.57 
 0:03.57     /home/ahal/dev/mozilla-central/ipc/app/moz.build
 0:03.57 
 0:03.57 The error was triggered on line 20 of this file:
 0:03.57 
 0:03.57     PROGRAM = name
 0:03.57 
 0:03.57 The underlying problem is an attempt to write an illegal value to a special variable.
 0:03.57 
 0:03.57 The variable whose value was rejected is:
 0:03.57 
 0:03.58     PROGRAM
 0:03.58 The value being written to it was of the following type:
 0:03.58 
 0:03.58     str
 0:03.58 
 0:03.58 This variable expects the following type(s):
 0:03.58 
 0:03.58     unicode
 0:03.58 
 0:03.58 Change the file to write a value of the appropriate type and try again.
 0:03.58 
 0:03.63 *** Fix above errors and then restart with\
 0:03.63                "./mach build"
 0:03.63 gmake: *** [client.mk:115: configure] Error 1

(In reply to Andrew Halberstadt [:ahal] from comment #34)

  1. Leave that encode at the end there despite not encoding config.status, i.e ignore this issue (sounds like this might be a no-go.. but figured I'd list it).

config.status does a subset of what configure does, avoiding to rerun all tests when you only need to adapt to e.g. changes to moz.build files. That means config.status and configure are meant to do the exact same thing. So that's not a option.

  1. Fix the underlying build problems. This is probably the most correct approach, but I don't feel qualified to tackle it (nor do I have that much time). This will likely delay the migration by weeks or even months. Maybe a build peer would be able to solve these more quickly than I think.

Not sure what you mean by "underlying build problems". If you mean the fact that some things are bytes before ending up in config.status, that sounds sensible.

  1. Keep enforcing bytes in config.status and fix the encoding issues on the other side (e.g in configenvironment.py and whatever mach commands use it).

That sounds awful. As in, this implies multiple places to get things subtly wrong.

Flags: needinfo?(mh+mozilla)

(In reply to Mike Hommey [:glandium] from comment #36)

Not sure what you mean by "underlying build problems". If you mean the fact that some things are bytes before ending up in config.status, that sounds sensible.

Yes, this is the ideal solution. By underlying problems I mostly mean this error:
https://phabricator.services.mozilla.com/D40504#1246752

I tried to debug that but have no idea what's going on. I'm pretty sure when that gets fixed we'll hit more errors, see this comment:
https://searchfox.org/mozilla-central/source/configure.py#126

That sounds awful. As in, this implies multiple places to get things subtly wrong.

Actually, this might not be quite as bad as I initially thought.. might have something that works well enough for non-build related commands to get unblocked. Will post back if it ends up being successful.

Without the output, this exception wasn't being very helpful.

Attachment #9080031 - Attachment description: Bug 1473498 - Fix byte string errors with Python 3 in configenvironment.py → Bug 1473498 - [mozbuild.backend.configenvironment] Fix byte string errors with Python 3
Attachment #9074347 - Attachment description: Bug 1473498 - Support running mach commands with python 3 → Bug 1473498 - [mach] Support running mach commands with python 3
Attachment #9082760 - Attachment is obsolete: true
Attachment #9085551 - Attachment description: Bug 1473498 - [mozbuild.util] Fix different forking path with Python 3.4+ → Bug 1473498 - [mozbuild.util] Don't use 'patch_main' hack with Python 3.4+
Depends on: 1575135

Use of 'BaseException.message` was deprecated in Python 2.6 (and removed in
Python 3). We should rely on the exception's 'str' instead.

We also need to ensure the mozconfig subprocess' output is text when formatting
it into the message with Py3.

Attachment #9080031 - Attachment is obsolete: true

This also fixes an import error in Python 3 (builtins.unicode).

Attachment #9087049 - Attachment description: Bug 1473498 - [configure] Don't attempt to decode text strings in util.configure or mozconfig.py, r?glandium → Bug 1473498 - [configure] Fix ImportError in util.configure under Python 3, r?glandium
Depends on: 1575804
Attachment #9088160 - Attachment is obsolete: true
Pushed by ahalberstadt@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b51268c0040b [mozbuild.mozconfig] Improve error message in MozconfigLoadException r=glandium https://hg.mozilla.org/integration/autoland/rev/983a86c020c9 [mozbuild.base] Fix Python 3 incompatibilities in mozconfig error handling, r=glandium https://hg.mozilla.org/integration/autoland/rev/baf6c4adbf2b [configure] Fix ImportError in util.configure under Python 3, r=glandium https://hg.mozilla.org/integration/autoland/rev/fa5e23975029 [mozbuild.util] Don't use 'patch_main' hack with Python 3.4+ r=firefox-build-system-reviewers,mshal https://hg.mozilla.org/integration/autoland/rev/d5ef7a5e02a4 [mach] Support running mach commands with python 3 r=glandium,mars
Regressions: 1577517
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Regressions: 1577908
Regressions: 1578198
No longer regressions: 1578198
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: