1428709 - [mozhttpd] Add support for Python 3

Status: RESOLVED WONTFIX

(Testing :: Mozbase, enhancement, P3)

Description

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

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

Comment 1

[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/mozhttpd/tests/manifest.ini`

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

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

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/

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/mozhttpd/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/mozhttpd/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 2

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1428709 - Add py35 skip-if to individual mozhttpd tests;

Review commit: https://reviewboard.mozilla.org/r/254464/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254464/

Comment 3

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1428709 - Fix Python 3 imports of http.server in mozhttpd;

Review commit: https://reviewboard.mozilla.org/r/254466/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254466/

Comment 4

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1428709 - Add six for Python 3 compat in mozhttpd;

Review commit: https://reviewboard.mozilla.org/r/254468/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254468/

Comment 5

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

Comment on attachment 8989374 [details]
Bug 1428709 - Add py35 skip-if to individual mozhttpd tests;

https://reviewboard.mozilla.org/r/254464/#review261250

Comment 6

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

Comment on attachment 8989375 [details]
Bug 1428709 - Fix Python 3 imports of http.server in mozhttpd;

https://reviewboard.mozilla.org/r/254466/#review261252

Comment 7

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

Comment on attachment 8989376 [details]
Bug 1428709 - Add six for Python 3 compat in mozhttpd;

https://reviewboard.mozilla.org/r/254468/#review261254

::: testing/mozbase/mozhttpd/mozhttpd/mozhttpd.py:29
(Diff revision 1)
>  
>  if PY3:
>      # BaseHTTPServer and SimpleHTTPServer have been merged in Python 3
>      from http.server import HTTPServer, SimpleHTTPRequestHandler
> +
> +    # urlpase has been moved to urllib.parse in Python 3

Nit: typo in urlparse, did you mean urlsplit?

Comment 8

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

Comment on attachment 8989374 [details]
Bug 1428709 - Add py35 skip-if to individual mozhttpd tests;

https://reviewboard.mozilla.org/r/254462/#review261256

Do these patches allow us to enable any tests? I'd like to hold off landing anything unless it enables tests, to protect us from regressions.

Comment 9

[Raphael Aurich [:raphael] UTC+01:00]

Comment on attachment 8989374 [details]
Bug 1428709 - Add py35 skip-if to individual mozhttpd tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254464/diff/1-2/

Comment 10

[Raphael Aurich [:raphael] UTC+01:00]

Comment on attachment 8989376 [details]
Bug 1428709 - Add six for Python 3 compat in mozhttpd;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254468/diff/1-2/

Comment 11

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Bug 1428709 - Enable several mozhttpd tests for Python 3;

Review commit: https://reviewboard.mozilla.org/r/256602/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256602/

Comment 12

[Raphael Aurich [:raphael] UTC+01:00]

Comment on attachment 8989376 [details]
Bug 1428709 - Add six for Python 3 compat in mozhttpd;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254468/diff/2-3/

Comment 13

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

Comment on attachment 8991675 [details]
Bug 1428709 - Enable several mozhttpd tests for Python 3;

https://reviewboard.mozilla.org/r/256602/#review263678

Comment 14

[Raphael Aurich [:raphael] UTC+01:00]

Comment on attachment 8989376 [details]
Bug 1428709 - Add six for Python 3 compat in mozhttpd;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254468/diff/3-4/

Comment 15

[Raphael Aurich [:raphael] UTC+01:00]

Comment on attachment 8991675 [details]
Bug 1428709 - Enable several mozhttpd tests for Python 3;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256602/diff/1-2/

Comment 16

[Raphael Aurich [:raphael] UTC+01:00]

Try build https://treeherder.mozilla.org/#/jobs?repo=try&revision=c37a91715cc122b987e8f751ddc4b139885ab021

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63a0d1c2d2b9
Add py35 skip-if to individual mozhttpd tests;r=davehunt
https://hg.mozilla.org/integration/autoland/rev/b70032519c16
Add six for Python 3 compat in mozhttpd; r=davehunt
https://hg.mozilla.org/integration/autoland/rev/e4cf0bcb5b69
Enable several mozhttpd tests for Python 3; r=davehunt

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/63a0d1c2d2b9
https://hg.mozilla.org/mozilla-central/rev/b70032519c16
https://hg.mozilla.org/mozilla-central/rev/e4cf0bcb5b69

Comment 19

[Raphael Aurich [:raphael] UTC+01:00]

Hi :ahal! The tests in api.py and filelisting.py still fail under Python 3.5 due to the way strings are being handled. I don't know mozhttpd well enough to be certain about the expected behavior. Would you mind having a look?

Comment 20

[Andrew Halberstadt [:ahal]]

For filelisting.py, it looks like we're importing urllib2 which doesn't exist anymore:
http://python-future.org/compatible_idioms.html#urllib-module

But I'm having trouble even running those tests with python 3 (they both seem to hang). I don't really have time to debug this at the moment, could you please file a follow-up bug? Thanks!

Comment 21

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

(In reply to Andrew Halberstadt [:ahal] from comment #20)
> For filelisting.py, it looks like we're importing urllib2 which doesn't
> exist anymore:
> http://python-future.org/compatible_idioms.html#urllib-module
> 
> But I'm having trouble even running those tests with python 3 (they both
> seem to hang). I don't really have time to debug this at the moment, could
> you please file a follow-up bug? Thanks!

The tests appear to be hanging because the mozhttpd service is never stopped. I was able to resolve this in `filelisting.py` by converting it to pytest test with a fixture for the mozhttpd service, which yields the MozHttpd object and stops it as part of the teardown. The test was still failing, but I was then able to fix it by filtering the list of lines returned down to just those containing paths, and comparing this to the file system. I'll push this patch for review shortly.

For `api.py` I think we should do the same, and convert the test to pytest with a fixture for the MozHttpd object. As this is a larger test file, with different arguments for the MozHttpd object I suggest splitting out the conversion to pytest into a separate bug. I'll raise this as a blocker.

Comment 22

[Raphael Aurich [:raphael] UTC+01:00]

Attachment: Add support for Python 3 to mozhttpd (Bug 1428709)

This patch resolves Python 3 compatiblity issues for mozhttpd's API and filelisting tests.

Comment 23

[Raphael Aurich [:raphael] UTC+01:00]

Hi folks,

The ``RequestHandler`` in mozhttpd [1] inherits from Python's ``SimpleHTTPRequestHandler``[2]. The output stream (``wfile``) for responses [3] requires a bytes-like object under Python 3.5 and will raise a TypeError for str objects.

I submitted a patch to encode response bodies to utf-8: https://phabricator.services.mozilla.com/D3981#inline-23080.

However this might cause errors in projects, that use mozhttpd, under both Python 2 and Python 3 if they don't expect utf-8 encoded bytes and I would like to hear your thoughts on how to resolve this issue.

[1]: https://searchfox.org/mozilla-central/source/testing/mozbase/mozhttpd/mozhttpd/mozhttpd.py#72
[2]: https://docs.python.org/3.5/library/http.server.html#http.server.SimpleHTTPRequestHandler
[3]: https://docs.python.org/3.5/library/http.server.html#http.server.BaseHTTPRequestHandler.wfile

Comment 24

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

It's not really my area of knowledge right now, so I believe Dave and Andrew are the better resources for information here.

Comment 25

[Andrew Halberstadt [:ahal]]

There isn't a whole lot of things that use mozhttpd these days (it's deprecated in favour of wptserve):
https://dxr.mozilla.org/mozilla-central/search?limit=100&redirect=false&q=mozhttpd%20-path%3Aobj-x86_64-pc-linux-gnu%2F%20-path%3Atesting%2Fmozbase%2F%20-path%3Atesting/mozharness%2F

I guess you could push your changes to try, make sure to run pgo builds, valgrind and talos at least. If things break, I guess you could hardcode different paths for python 2 vs python 3 (or better yet we port the remainder of those consumers over to wptserve).

Comment 26

[Raphael Aurich [:raphael] UTC+01:00]

Thanks for the info :ahal & :whimboo. There are a few projects outside of mozilla-central, which appear to use mozhttpd:

https://libraries.io/pypi/mozhttpd/usage

I thought about a Python 2 / Python 3 shim in mozhttpd, however this won't prevent problems for Python 2 consumers that send HTTP requests to a mozhttpd server running Python 3 (or consumers that don't expect utf-8 encoded response bodies for that matter).

Try run for https://phabricator.services.mozilla.com/D3981 and pgo, valgrind, talos, python2, python3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1c58e40f15294f30651e0d81173b27709a4abbe

Comment 27

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

(In reply to Raphael Pierzina [:raphael] UTC+01:00 from comment #26)
> Thanks for the info :ahal & :whimboo. There are a few projects outside of
> mozilla-central, which appear to use mozhttpd:
> 
> https://libraries.io/pypi/mozhttpd/usage

It looks like some of those are pinned to specific versions. I took a look at the few that aren't, and they all appear to be inactive. I like :ahal's suggestion of migrating consumers to wptserve. As we want to finish off bug 1471622, which is being blocked by a single test that uses mozhttpd, why don't we start with migrating mozfile's tests to depend on wptserve?

Comment 28

[Raphael Aurich [:raphael] UTC+01:00]

(In reply to Dave Hunt [:davehunt] ⌚️UTC+1 from comment #27)
> As we want to finish off bug 1471622, which is being blocked by a single test
> that uses mozhttpd, why don't we start with migrating mozfile's tests to
> depend on wptserve?

I created bug 1492401 for this and submitted a patch to migrate mozfile's tests to wptserve.

Comment 29

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:raphael, maybe it's time to close this bug?

Comment 30

[Raphael Aurich [:raphael] UTC+01:00]

I also like the suggestion to migrate mozhttpd consumers to wptserve. What does that mean for this bug?

Comment 31

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

I would suggest filing bugs for each of the in-tree consumers of mozhttpd to swtich to wptserve. We could close this as WONTFIX.

Comment 32

[Andrew Halberstadt [:ahal]]

Yeah, that sounds like a good plan to me.

Comment 33

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:raphael, maybe it's time to close this bug?

Comment 34

[Raphael Aurich [:raphael] UTC+01:00]

I created bug 1578237 for porting consumers over to wptserve.