1428706 - [mozcrash] Add support for Python 3

Status: RESOLVED FIXED

(Testing :: Mozbase, enhancement, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 1

[(not currently active) Ted Mielczarek]

Running mozcrash through a quick pylint check, I think using `from __future__ import unicode_literals` will fix most of the compat issues. We'll probably also need to wrap something around urllib2 to handle that. We've already got `six` vendored in the tree, it looks like that has support for this by importing `six.moves.urllib`, which provides the Python 3 urllib API:
https://pythonhosted.org/six/#module-six.moves

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> We'll probably also need to wrap something around urllib2 to handle that.

Yes, that is what we already do in mozfile.

Comment 3

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

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 `testing/mozbase/mozcrash/tests/manifest.ini`

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

```
mach python-test --python=3.5 testing/mozbase/mozcrash
```

Work through the test failures, and update the tests and source code for the package to simultaneuously support Python 2.7 and Python 3.5. You can always check that your changes have not inadvertantly 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/

Note that this package depends on others that may not yet support Python 3. If these dependencies are preventing tests from passing, then we can block this bug on getting support in those packages. If there are circular dependencies then we may need to coordinate landing a sequence of patches between bugs,

Once the package supports Python 3 and the tests pass, we will also need to prepare for a new release. Bump the version number in `testing/mozbase/mozcrash/setup.py` to "2.0.0", and update the classifiers so they reflect the versions of Python that we now support.

Finally, to prepare a universal distribution, create a `testing/mozbase/mozcrash/setup.cfg` file with the following contents:

```
[bdist_wheel]
universal=1
```

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!

Comment 4

[A. Wilcox [:awilfox]]

I have a patch that allows all the tests pass on Python 3, but the fix for Bug 1441287 doesn't seem to have been thoroughly tested.

mozcrash fails its tests even on Python 2.7 without any changes from me, if I change LANG and LC_ALL to a non-UTF-8 locale.  This is on a computer running Debian sid.  What's worse, none of the logging stuff in mach appears ready to handle the Unicode characters, so the test failure causes the logger to raise.  I was able to hack-and-slash mach to make the log file contain the failure:

[1538782849.0347, "python-test", {"line": "\u001b[1m\u001b[31mE       UnicodeEncodeError: 'latin-1' codec can't encode character u'\\U0001f36a' in position 58: ordinal not in range(256)\u001b[0m"}]

I assume this is Windows specific, and that nobody actually uses non-UTF-8 terminals on Unix.  I do not own a Windows computer, so I can't be sure that the tests still pass there.

Please advise the next step, as I'm not sure how to proceed.

Comment 5

[A. Wilcox [:awilfox]]

Attachment: Add support for Python 3 to mozcrash

This patch adds support in mozcrash for Python 3.

Note that per my comment in Bug 1428706, UTF-8 terminal encoding seems required to pass the tests.  I am therefore unsure if this will work correctly on Windows, but I have no Windows computer to test this on.

All tests pass under 2.7, 3.5, 3.6 locally under Debian sid.

Comment 6

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

Thanks for the patch :awilfox. :ted can you comment on the mozcrash support for Windows?

Comment 7

[(not currently active) Ted Mielczarek]

(In reply to A. Wilcox [:awilfox] from comment #4)
> mozcrash fails its tests even on Python 2.7 without any changes from me, if I change LANG and LC_ALL to a non-UTF-8 locale. 

We effectively don't support non-UTF-8 locales on Linux.

> [1538782849.0347, "python-test", {"line": "\u001b[1m\u001b[31mE      
> UnicodeEncodeError: 'latin-1' codec can't encode character u'\\U0001f36a' in
> position 58: ordinal not in range(256)\u001b[0m"}]

I don't know where this output is originating, but those are terminal escapes. (Specifically that is "set the foreground color to red" followed by a literal "E".)

> I assume this is Windows specific, and that nobody actually uses non-UTF-8
> terminals on Unix.  I do not own a Windows computer, so I can't be sure that
> the tests still pass there.

Yes. Handling Unicode output on Windows is a pain, but it should be tractable in this case because the output of minidump_stackwalk is generally just ASCII (unless there are non-ASCII paths in filenames in the module list, which is extremely rare and shouldn't happen in any of the cases we care about) and the rest of the output here is also ASCII.

Given that there's no sensible default on Windows, outputting UTF-8 here is the least-bad option for piping to other processes. We could basically do:
if sys.stdout.encoding is None:
    o = codecs.getwriter('utf-8')(sys.stdout)
else:
    o = sys.stdout
print('...', file=o)

And that ought to work.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 9014986 [details]
Add support for Python 3 to mozcrash

Phabricator didn't copy the r+ over here.

Comment 9

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

:awilfox I'm unable to land this because "Diff does not have proper author information in Phabricator." You may need to set an author in your .hgrc file. See this Lando FAQ for more details.

Finally, could you prepare for a new release by bumping the version number in testing/mozbase/mozcrash/setup.py to "2.0.0", and updating the classifiers so they reflect the versions of Python that we now support. Then, to prepare a universal distribution, create a testing/mozbase/mozcrash/setup.cfg file with the following contents:

[bdist_wheel]
universal=1

Thanks, and sorry for the delay on responding to your patch!

Comment 10

[A. Wilcox [:awilfox]]

The revision was created via the Phabricator Web UI or via an unsupported client.

That's what actually happened. moz-phab doesn't seem to work properly on my machine and Arcanist kept throwing errors. I can try again, but I don't know.

I can definitely bump the setup.py and .cfg information; would you like me to create a new single patch with the current changes and that bump, or would you like a separate patch for the setup.* files?

Comment 11

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

(In reply to A. Wilcox [:awilfox] from comment #10)

The revision was created via the Phabricator Web UI or via an unsupported client.

That's what actually happened. moz-phab doesn't seem to work properly on my machine and Arcanist kept throwing errors. I can try again, but I don't know.

Please try again, and if you get errors provide details so we can help you to resolve them.

I can definitely bump the setup.py and .cfg information; would you like me to create a new single patch with the current changes and that bump, or would you like a separate patch for the setup.* files?

Updating the existing patch would be great, thanks!

Comment 12

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

Attachment: Bug 1428706 - [mozcrash] Add support for Python 3; r=ted

Comment 13

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

Attachment: Bug 1428706 - [mozcrash] Bump version number to 1.2.0 and prepare for release with Python 3 support; r?ahal

Depends on D19132

Comment 14

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

The author's bugzilla account is disabled. Fortunately I was able to find previous commits from this author and so was able to attribute this patch to them. I've also added a follow-up patch to prepare us for a new release of mozcrash.

ted: Could you carry your review over to the new patch? There are no changes, but as I'm now the "author" of the patch (not the commit) I can't do the review.

ahal: Mind reviewing the small follow-up?

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/184745d744bf
[mozcrash] Add support for Python 3; r=ted

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/184745d744bf

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c4a149e6f90
[mozcrash] Bump version number to 1.2.0 and prepare for release with Python 3 support; r=ahal

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/3c4a149e6f90

Comment 19

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

mozcrash 1.1.0 has been released to PyPI: https://pypi.org/project/mozcrash/1.1.0/