1388018 - [mozfile] Add support for Python 3

Status: RESOLVED FIXED

(Testing :: Mozbase, enhancement)

Description

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

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

Comment 1

[Rupanshu Verma]

Hi

I am new to the open source community and I would like to work on this bug. Any kind of guidance would be highly appreciated.
Thanks

Comment 2

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

Hey Rupanshu! Thank you for your interest in working on this bug. There are a few dependencies, but these are mostly for ensuring we're able to run the necessary tests against Python 3 in our continuous integration environment and will help to prevent any regressions. If you're interested in working on this please go ahead, and we can land any patches you contribute once the dependencies are resolved.

To get started, you will need to install Mercurial:
http://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/installing.html

Once you have Mercurial, you should be able to clone the repository using the following command:
hg clone https://hg.mozilla.org/mozilla-central/

You can run the existing mozfile tests using:
./mach python-test testing/mozbase/mozfile

Note that the above will run the tests using Python 2. This will be addressed in bug 1388013, which you're also welcome to work on.

Comment 3

[Aditya Addepalli]

I don't know how to bring this to anyone's notice, so I'll just put it here:
https://github.com/mozilla/gecko-dev/pull/128

Comment 4

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Hey, is anyone interested in getting this across the line? I'm checking so I don't step on any toes, but am thinking of taking a look at this and want to make sure I'm not going to interfere with work in progress.

Comment 5

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

(In reply to dyex719 from comment #3)
> I don't know how to bring this to anyone's notice, so I'll just put it here:
> https://github.com/mozilla/gecko-dev/pull/128

Hey Dyex719, thank you for the contribution! So usually we work with patches attached to Bugzilla given that gecko-dev is a read-only repository and the PR could not be merged. Given that Dave is the mentor here I will ni? him, so he can follow-up with you.

Comment 6

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

Thank you for the patch dyex719, and for bringing it to my attention whimboo. As pointed out, this would need to be attached as a patch in Bugzilla in order for us to review it. Whilst bug 1388013 is required to ensure we don't later regress Python 3 support, I don't see a reason not to merge a patch that makes the package provisionally supported by Python 3. Of course anyone using mozfile with Python 3 before bug 1388013 is resolved would risk any intermediate releases causing breakage.

Please attach your patch so we can review it and ensure that there are no regressions introduced by it before landing. Once it's landed, we can release a new version of mozfile.

Comment 7

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

I should add that since bug 1388340 we've vendored in six, which may be useful for simultaneously supporting both Python 2 and 3.

Comment 8

[Aditya Addepalli]

Attachment: First-Attempt

As this bug depends on bug #1388013 (https://bugzilla.mozilla.org/show_bug.cgi?id=1388013) wouldn't we have to wait for that to get resolved before we start testing this?

Comment 9

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Hey, I think I can offer some help here to aid with Dave's review queue. I hope this is cool.

The patch looks good for Python 3 support. Since we're still supporting Python 2.7 six can be used to aid in compatibility with 2:

>-    import urlparse
>+    import urllib.parse
> 
>-    parsed = urlparse.urlparse(thing)
>+    parsed = urllib.parse.urlparse(thing)

For functionality which is now under urllib, using six's six.moves module (https://pythonhosted.org/six/#module-six.moves) allows for compatibility with both versions of Python.

>-    import urllib2
>+    import urllib.request, urllib.error, urllib.parse

>-    return urllib2.urlopen(resource)
>+    return urllib.request.urlopen(resource)

As above.

>-        self.assertTrue(isinstance(path, basestring))
>+        self.assertTrue(isinstance(path, str))

six.string_types (https://pythonhosted.org/six/#constants) can be used here to provide compatibility.

There's a couple of tests in the tests which can be refactored in a similar way. Notably I believe `test_extract.py` has an old style print ad `test_tempfile.py` has an instance check using basestring.

Comment 10

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

Comment on attachment 8910296 [details] [diff] [review]
First-Attempt

In addition to the items raising in the review from :SingingTree (thank you), you will also want to ensure that the existing tests are still passing. You can run these using the following command:

$ mach python-test testing/mozbase/mozfile/

Currently with your patch they are failing with:

> ImportError: No module named request.

As mentioned in comment 6, I have no objection to landing support for Python 3 before bug 1388013 is resolved. I just wouldn't want to advertise support for Python 3 as there wouldn't be any protection in place from regressing support until that's done. Anyone using the package with Python 3 before then would be doing so at some risk, although they could mitigate this somewhat by pinning to a specific version of the package.

Comment 11

[Aditya Addepalli]

Attachment: Using the six module

Sorry! I somehow missed this!
Thanks for your help :SingingTree. I tried to make the changes that you said.
Please tell me if I am missing something.

Comment 12

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

Comment on attachment 8918629 [details] [diff] [review]
Using the six module

Review of attachment 8918629 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Dyex! The tests are passing in Python 2 and the Python 3 compat linter looks good, however there are a couple of issues I've highlighted that need to be addressed. I also noticed that this didn't import cleanly for me because the patch appears to be from testing/mozbase/mozfile. I can still apply it by specifying a --prefix, but if you could fix this it would make it easier to land.

::: mozfile/mozfile.py
@@ +420,4 @@
>      Return True if thing looks like a URL.
>      """
>  
> +    from six.moves import urllib

As we're using six, we need to add it as a dependency. Could you add it to install_requires in testing/mozbase/mozfile/setup.py? This doesn't matter for running the tests as we vendor in six, but does matter for the package once it's published on PyPI.

@@ +446,2 @@
>          # if no scheme is given, it is a file path
>          return file(resource)

'file' is not a built-in function in Python 3, so this is failing. You should be able to safely switch over to 'open' for this and the occurrence in test_extract.py.

::: tests/test_tempfile.py
@@ +68,5 @@
>          # make a deleteable file; ensure it gets cleaned up
>          path = None
>          with mozfile.NamedTemporaryFile(delete=True) as tf:
>              path = tf.name
> +        import six

Could you move this import to the top, after the mozunit import?

Comment 13

[Aditya Addepalli]

Attachment: Adding Minor changes

Please look at this.
I find it difficult working with mercurial. Can you help me tackle these questions?
1. How do I update my local copy of the firefox repo with the master branch? 
2. How can I go to a previous commit without it changing my files?
3. How can I make "hg export > Bugxyz.patch" export the changes in more than one commit? For Eg: The last commit and the one before that.

Thank you.

Comment 14

[Andrew Halberstadt [:ahal]]

Just a note: six isn't installed in any of our test machines in automation, so making mozbase depend on it will break all the mochitest/reftest/etc jobs until bug 1407769. So make sure to wait for that bug to be finished before attempting to land.

Comment 15

[Andrew Halberstadt [:ahal]]

Ignore me. Turns out six is already on the internal pypi, so as long as it's defined in setup.py it should just work. Definitely test out a mochitest/reftest job or two just to be safe though.

Comment 16

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

(In reply to dyex719 from comment #13)
> Created attachment 8920107 [details] [diff] [review]
> Adding Minor changes
> 
> Please look at this.
> I find it difficult working with mercurial. Can you help me tackle these
> questions?
> 1. How do I update my local copy of the firefox repo with the master branch? 
> 2. How can I go to a previous commit without it changing my files?
> 3. How can I make "hg export > Bugxyz.patch" export the changes in more than
> one commit? For Eg: The last commit and the one before that.
> 
> Thank you.

I would suggest reading through http://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/index.html, which may answer some of your questions. I tend to use the unified repository with bookmarks for my development, so I'd typically have a bookmark such as 'bug1388018', which I can rebase from the 'central' bookmark periodically. You can read more about the unified repository here: http://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/unifiedrepo.html#unified-repository-on-hg-mozilla-org and about using bookmarks here: http://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/bookmarks.html#

When you're ready to submit a patch for review, you can push to our MozReview tool, which saves you from needing to export diffs for upload to Bugzilla. After a review you can simple add more commits and push those to MozReview for the next round of reviews. You can read more about MozReview and how to configure it here: http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html#mozreview-user

Comment 17

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

Comment on attachment 8920107 [details] [diff] [review]
Adding Minor changes

Review of attachment 8920107 [details] [diff] [review]:
-----------------------------------------------------------------

Note that if bug 1397849 lands first then we'd need to rebase this as there are some shared changes.

::: testing/mozbase/mozfile/setup.py
@@ +23,4 @@
>        packages=['mozfile'],
>        include_package_data=True,
>        zip_safe=False,
> +      install_requires=['six'],

We should depend on 1.10.0 or later, as that's what we have vendored into the tree.

Comment 18

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

Bug 1397849 has been resolved, which means this patch will need to be rebased. As a result it should also be much smaller. Are you still available to finish this up, Dyex?

Comment 19

[Aditya Addepalli]

There seems to be some updates to that bug after you commented. I'll keep checking on the status of that bug and rebase this once Bug 1397849 is confirmed to be done.

Comment 20

[Ed Morley [:emorley]]

The followup to bug 1397849 has now landed, so hopefully we should be good to proceed here :-)

Comment 21

[Aditya Addepalli]

I'm guessing I need to do something like,
$ hg up C
$ hg rebase --dest E 
(from https://www.mercurial-scm.org/wiki/RebaseExtension)
What is 'E' here?

Comment 22

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

Yes, you should be able to use the rebase extension. As the documentation shows, you'll need to provide a source changeset and a destination changeset. The source is the first changeset of your changes, and the destination is where you want your changes to be rebased against. This will be the latest changeset from an update, and needs to be after the changes from bug 1397849.

A successful rebase will remove your changes, and reapply them to the destination. You can use |hg log| to see details of changesets. There's some additional suggestions, including |hg wip| to show work in progress here: http://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/workflows.html#refining-what-changesets-are-shown

Comment 23

[Aditya Addepalli]

Attachment: Rebased Patch

Please review this. Thank you.

Comment 24

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

Comment on attachment 8924060 [details] [diff] [review]
Rebased Patch

Review of attachment 8924060 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Dyex! I was able to get the tests running locally against Python 3 with a few hacks, and found some issues.

* testing/mozbase/mozfile/tests/stubs.py:30 is using file instead of open.
* testing/mozbase/mozfile/tests/test_extract.py:112 throws TypeError: a bytes-like object is required, not 'str'
* testing/mozbase/mozfile/tests/test_tempfile.py:46 throws TypeError: a bytes-like object is required, not 'str'
* testing/mozbase/mozfile/tests/test_tempfile.py:32 throws TypeError: a bytes-like object is required, not 'str'
* testing/mozbase/mozfile/tests/test_load.py fails to import mozhttpd because it is not currently compatible with Python 3
* testing/mozbase/mozfile/tests/test_move_remove.py fails to import mozinfo because it is also not currently compatible with Python 3

I would say that the last two are out of scope for this bug as we'd need to add support for Python 3 to mozhttpd and mozinfo. That said, without these tests passing we may not be able to claim mozfile supports Python 3. I wouldn't block on this as we're not ready to run the tests against Python 3 yet, and once we've landed this patch we could move onto either one of those packages next.

::: testing/mozbase/mozfile/mozfile/mozfile.py
@@ +162,5 @@
>              retry_count += 1
>  
> +            print('%s() failed for "%s". Reason: %s (%s). Retrying...' % \
> +                (func.__name__, args, e.strerror, e.errno))
> +

This change is not needed.

@@ +438,3 @@
>      """
>  
> +    from six.moves import urllib

We're importing this in two places. Could you move the import to the top so we only have to import it once for the module?

::: testing/mozbase/mozfile/tests/test_tempfile.py
@@ +63,4 @@
>          self.assertEqual(lines, notes)
>  
>      def test_delete(self):
> +

Please remove this extra whitespace.

@@ +69,5 @@
>          # make a deleteable file; ensure it gets cleaned up
>          path = None
>          with mozfile.NamedTemporaryFile(delete=True) as tf:
>              path = tf.name
> +

Also remove the extra whitespace here.

Comment 25

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

dyex719: do you have time to address my latest comments?

Comment 26

[Aditya Addepalli]

Sorry, I am a bit busy till Monday. I can only work on this after that. If it is urgent, feel free to finish it up.

Comment 27

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

It can wait, thanks for the response! :)

Comment 28

[Vedant Chakravadhanula(:vedantc98)]

If there hasn't been work ongoing in a while, I'd like to take up this bug ( since it blocks Bug 1388019 ). 
Please let me know if you're actively working on this, dyex719

Comment 29

[Aditya Addepalli]

Really sorry about this. Sure, you can finish this. Refer to Comment #24. There are a few minor changes to be made and errors to be solved. Sorry for my negligence.

Comment 30

[Vedant Chakravadhanula(:vedantc98)]

No problems, I'll add the minor changes needed on top of your previous patches. 
Thanks!

Dave, would it be okay if I imported the previous attachments as patches and then added the new required changes and pushed them for review as a single commit?

Comment 31

[Vedant Chakravadhanula(:vedantc98)]

Attachment: Bug 1388018 - [mozfile] Add support for python 3.

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

Comment 32

[William Lachance (:wlach)]

Comment on attachment 8935658 [details]
Bug 1388018 - [mozfile] Add support for python 3.

https://reviewboard.mozilla.org/r/206558/#review213810

[stealing this from davehunt, hope that's ok]

This looks good except for some minor issues with setup.py. Could you fix those and resubmit? I'll run a small number of unit tests on try to be sure this looks ok too.

::: testing/mozbase/mozfile/setup.py:17
(Diff revision 1)
>  setup(name=PACKAGE_NAME,
>        version=PACKAGE_VERSION,
>        description="Library of file utilities for use in Mozilla testing",
>        long_description="see https://firefox-source-docs.mozilla.org/mozbase/index.html",
>        classifiers=['Programming Language :: Python :: 2.7',
>                     'Programming Language :: Python :: 2 :: Only'],

You should be able to remove these classifiers.

Comment 33

[Vedant Chakravadhanula(:vedantc98)]

Comment on attachment 8935658 [details]
Bug 1388018 - [mozfile] Add support for python 3.

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

Comment 34

[Vedant Chakravadhanula(:vedantc98)]

I've removed the classifiers.
Thanks!

Comment 35

[William Lachance (:wlach)]

Comment on attachment 8935658 [details]
Bug 1388018 - [mozfile] Add support for python 3.

https://reviewboard.mozilla.org/r/206558/#review213886

There are some flake8 errors with this patch which need to be fixed before landing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2eca3030a2be&selectedJob=151982018

Close though :)

::: testing/mozbase/mozfile/setup.py
(Diff revision 2)
>        version=PACKAGE_VERSION,
>        description="Library of file utilities for use in Mozilla testing",
>        long_description="see https://firefox-source-docs.mozilla.org/mozbase/index.html",
> -      classifiers=['Programming Language :: Python :: 2.7',
> -                   'Programming Language :: Python :: 2 :: Only'],
> -      # Get strings from http://pypi.python.org/pypi?%3Aaction=list_classifiers

Sorry to be a pain, but could you actually add trove classifiers that make it clear that python 3 is supported? I.e.:

```py
classifiers = ['Programming Language :: Python :: 2.7',
 'Programming Language :: Python :: 3']
```

Comment 36

[Vedant Chakravadhanula(:vedantc98)]

Comment on attachment 8935658 [details]
Bug 1388018 - [mozfile] Add support for python 3.

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

Comment 37

[Vedant Chakravadhanula(:vedantc98)]

No problem, thanks for the guidance William!
I've updated the PR after sorting out the flake8 errors and adding the classifiers.
Thanks!

Comment 38

[William Lachance (:wlach)]

Comment on attachment 8935658 [details]
Bug 1388018 - [mozfile] Add support for python 3.

https://reviewboard.mozilla.org/r/206558/#review213914

Ok, I think we're good here now!

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca4c587c313b
[mozfile] Add support for python 3. r=wlach

Comment 40

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/ca4c587c313b

Comment 41

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

Do we want to already push the new version to PyPI, or wait until all packages have been updated? I would appreciate to get a Py3 compatible version as early as possible.

Comment 42

[William Lachance (:wlach)]

(In reply to Henrik Skupin (:whimboo) [mostly away Dec 11th - Jan 3rd] from comment #41)
> Do we want to already push the new version to PyPI, or wait until all
> packages have been updated? I would appreciate to get a Py3 compatible
> version as early as possible.

My personal plan is to wait until we have a minimal set required by mozregression, but I'm also totally fine if people want to publish individual packages before then.

Comment 43

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

(In reply to William Lachance (:wlach) (use needinfo!) from comment #42)
> My personal plan is to wait until we have a minimal set required by
> mozregression, but I'm also totally fine if people want to publish
> individual packages before then.

So mozdownload actually also relies on mozinfo. So once both packages are py3 compatible new releases would be great. I only fear that we might miss/forget to release an already fixed packages.

Comment 44

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

> [stealing this from davehunt, hope that's ok]

Absolutely, sorry for dropping the ball, and thank you for picking it up!