1473498 - [mach] Add support for Python 3

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Without dropping support for legacy Python, we need to add support for Python 3 to mach.

Comment 1

[flying sheep]

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)

Comment 2

[Andrew Halberstadt [:ahal]]

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.

Comment 3

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1473498 - [mach] Pull tests out of make check

Comment 4

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1473498 - [mach] Enable test_dispatcher with python 3

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

Depends on D36097

Comment 5

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1473498 - [mach] Enable test_entry_points with Python 3

Depends on D36098

Comment 6

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1473498 - [mach] Enable test_config with Python 3

Depends on D36100

Comment 7

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1473498 - [mach] Enable test_logger with Python 3

Depends on D36101

Comment 8

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1473498 - [mach] Support running mach commands with python 3

Depends on D36102

Comment 9

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1473498 - [mach] Fix Python 3 decorator compatibility

Depends on D36103

Comment 10

[Andrew Halberstadt [:ahal]]

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.

Comment 11

[Andrew Halberstadt [:ahal]]

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).

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/2bd4fed10ef7
https://hg.mozilla.org/mozilla-central/rev/54ea59c8c99d
https://hg.mozilla.org/mozilla-central/rev/152598dd91a8
https://hg.mozilla.org/mozilla-central/rev/7123c3146d81
https://hg.mozilla.org/mozilla-central/rev/3416303a9b4a

Comment 14

[Māris Fogels [:mars] (please needinfo)]

Attachment: Bug 1473498 - make mach shell POSIX compliant

Comment 15

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1473498 - Vendor backports.shutil_which

Comment 16

[Phabricator Automation]

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

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

Comment 17

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1473498 - Fixing Py3 compatibility errors reachable from testing/mach_commands.py

Comment 18

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1473498 - Don't add backport libraries to the Py3 environment

Depends on D37762

Comment 19

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1473498 - Fix Py3 compatibility issue in python/mozbuild/mozbuild/testing.py

Depends on D37763

Comment 20

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1473498 - [mach] Fix Py3 compatibility issues in telemetry.py

Depends on D36104

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

Comment 22

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1473498 - Fix Python 3 environment variables with subprocess

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.

Comment 23

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/0c04c1b1f297
https://hg.mozilla.org/mozilla-central/rev/a5a6ea416ec3
https://hg.mozilla.org/mozilla-central/rev/a75643abc92c

Comment 24

[Pulsebot]

Pushed by [e-mail]:
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

Comment 25

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/c4a97f2e3303
https://hg.mozilla.org/mozilla-central/rev/dd1bb21a2271

Comment 26

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1473498 - [mozbuild.backend.configenvironment] Fix byte string errors with Python 3

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05c004d597d2
Fix Python 3 environment variables with subprocess r=glandium

Comment 28

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/05c004d597d2

Comment 29

[Justin Wood (:Callek)]

Attachment: Bug 1473498 - More support for py3.

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.

Comment 30

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1473498 - Don't encode config.status

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ff6e514f596
More support for py3. r=firefox-build-system-reviewers,mshal

Comment 32

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/6ff6e514f596

Comment 34

[Andrew Halberstadt [:ahal]]

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?

Comment 35

[Andrew Halberstadt [:ahal]]

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

Comment 36

[Mike Hommey [:glandium]]

(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.

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.

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.

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).

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

Comment 37

[Andrew Halberstadt [:ahal]]

(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.

Comment 38

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1473498 - [mozbuild.mozconfig] Improve error message in MozconfigLoadException

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

Comment 39

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1473498 - [mozbuild.util] Don't use 'patch_main' hack with Python 3.4+

Depends on D36843

Comment 40

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1473498 - [mozbuild.base] Fix Python 3 incompatibilities in mozconfig error handling, r?glandium

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.

Comment 41

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1473498 - [configure] Fix ImportError in util.configure under Python 3, r?glandium

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

Comment 42

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1473498 - [mozbuild] Don't attempt to decode text strings in mozconfig.py, r?glandium

Comment 43

[Pulsebot]

Pushed by [e-mail]:
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

Comment 44

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/b51268c0040b
https://hg.mozilla.org/mozilla-central/rev/983a86c020c9
https://hg.mozilla.org/mozilla-central/rev/baf6c4adbf2b
https://hg.mozilla.org/mozilla-central/rev/fa5e23975029
https://hg.mozilla.org/mozilla-central/rev/d5ef7a5e02a4