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

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

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.

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

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

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.

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

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?

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

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 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?

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.

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.

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.

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

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?

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.

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

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?

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

Please review this. Thank you.

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.

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

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.

It can wait, thanks for the response! :)

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

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.

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

I've removed the classifiers. Thanks!

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'] ```

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

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!

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

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.

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

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

> [stealing this from davehunt, hope that's ok] Absolutely, sorry for dropping the ball, and thank you for picking it up!