1425399 - [mozprofile] Add support for Python 3

Status: RESOLVED FIXED

(Testing :: Mozbase, enhancement)

Description

[Vedant Chakravadhanula(:vedantc98)]

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

Comment 1

[Vedant Chakravadhanula(:vedantc98)]

Attachment: Bug 1425399 - Added python 3 support to mozprofile.

Part of moving mozbase to Python 3 while maintaining legacy
python support. Mozprofile is also a dependency of
mozregression, and moving it to py3 helps in moving
mozregression.

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

Comment 2

[Vedant Chakravadhanula(:vedantc98)]

William, I've made a few changes that should be good enough to move to Python 3, although Mozprofile depends on mozfile and mozlog, which are pending python 3 support.

I've run the changes through a linter and python-test.

Please let me know if there is anything else you suggest.
Thanks!

Comment 3

[William Lachance (:wlach)]

Comment on attachment 8937031 [details]
Bug 1425399 - Added python 3 support to mozprofile.

https://reviewboard.mozilla.org/r/207762/#review213808

setup.py also has a line that needs changing:

https://searchfox.org/mozilla-central/rev/3ec05888ca32b2d8a14d700474efb0c63411fca2/testing/mozbase/mozprofile/setup.py#14

Other than that this looks good to me? I'm running a try job to make sure nothing broke. I'll have a look around the time that finishes up.

Comment 4

[Vedant Chakravadhanula(:vedantc98)]

Comment on attachment 8937031 [details]
Bug 1425399 - Added python 3 support to mozprofile.

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

Comment 5

[Vedant Chakravadhanula(:vedantc98)]

Updated patch, please let me know if I need to change anything or if anything breaks on the try build.
Thanks!

Comment 6

[William Lachance (:wlach)]

Comment on attachment 8937031 [details]
Bug 1425399 - Added python 3 support to mozprofile.

https://reviewboard.mozilla.org/r/207762/#review213890

Just one last change (which I only noticed because of me looking at mozfile). This looks good in general, try run went ok too except for some oranges which seem to be the result of normal intermittents.

::: testing/mozbase/mozprofile/setup.py:22
(Diff revision 2)
>  
>  setup(name=PACKAGE_NAME,
>        version=PACKAGE_VERSION,
>        description="Library to create and modify Mozilla application profiles",
>        long_description="see https://firefox-source-docs.mozilla.org/mozbase/index.html",
>        classifiers=['Environment :: Console',

Could you modify the trove classifiers the same as you did with mozfile?

Comment 7

[William Lachance (:wlach)]

Hmm, actually maybe I spoke too soon. It looks like the X7 chunk of xpcshell is basically permafailing on linux64 opt with this enabled.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3cb2a7936b0179d2df019cec5bf5be952b196aa&selectedJob=151982322

I'll experiment with doing another try run, just to be sure.

Comment 8

[Vedant Chakravadhanula(:vedantc98)]

The failures seem to be because of bug 1365021 .
Is there anything we can do about this?

Comment 9

[William Lachance (:wlach)]

(In reply to Vedant Chakravadhanula(:vedantc98) from comment #8)
> The failures seem to be because of bug 1365021 .
> Is there anything we can do about this?

I suspect this might be a bad merge or something. Can you resubmit the patch on top of the latest mozilla-central (fixing the issues mentioned in the review) and I can try again?

Comment 10

[Vedant Chakravadhanula(:vedantc98)]

Comment on attachment 8937031 [details]
Bug 1425399 - Added python 3 support to mozprofile.

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

Comment 11

[Vedant Chakravadhanula(:vedantc98)]

I've made the required changes on top of the latest central pull.
In the meantime, should I start with another mozbase package?

Comment 12

[William Lachance (:wlach)]

(In reply to Vedant Chakravadhanula(:vedantc98) from comment #11)
> I've made the required changes on top of the latest central pull.
> In the meantime, should I start with another mozbase package?

Thank you! I will take a look tomorrow my time. And yes, please go ahead and port other mozbase packages (anything used by mozregression would be helpful to me).

Comment 13

[William Lachance (:wlach)]

Comment on attachment 8937031 [details]
Bug 1425399 - Added python 3 support to mozprofile.

https://reviewboard.mozilla.org/r/207762/#review214170

::: testing/mozbase/mozprofile/setup.py:7
(Diff revision 3)
>  # License, v. 2.0. If a copy of the MPL was not distributed with this file,
>  # You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  from __future__ import absolute_import
>  
>  import sys

flake8 says this line isn't used anymore, could you remove it. :)

To save time, try running flake8 beforehand. If your copy of Firefox is properly checked out, you should be able to do this with mach lint:

https://firefox-source-docs.mozilla.org/tools/lint/usage.html#using-the-command-line

Comment 14

[William Lachance (:wlach)]

Comment on attachment 8937031 [details]
Bug 1425399 - Added python 3 support to mozprofile.

https://reviewboard.mozilla.org/r/207762/#review214224

LGTM except for the flake8 errors. Try run worked well this time too, for whatever reason. I think xpcshell is just flakey :(

Comment 15

[Vedant Chakravadhanula(:vedantc98)]

Comment on attachment 8937031 [details]
Bug 1425399 - Added python 3 support to mozprofile.

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

Comment 16

[Vedant Chakravadhanula(:vedantc98)]

I've removed the unnecessary import line.
Sorry for wasting time like this, I should've run the linter after every commit.
Thanks!

Comment 17

[William Lachance (:wlach)]

Comment on attachment 8937031 [details]
Bug 1425399 - Added python 3 support to mozprofile.

https://reviewboard.mozilla.org/r/207762/#review214474

Looks good on try now! I'll merge.

Comment 18

[William Lachance (:wlach)]

Comment on attachment 8937031 [details]
Bug 1425399 - Added python 3 support to mozprofile.

https://reviewboard.mozilla.org/r/207762/#review214476

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2bc8e60fb356
Added python 3 support to mozprofile. r=wlach

Comment 20

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a2a8d7ccac40
Backed out changeset 2bc8e60fb356 for failing browser chrome tests r=backout on a CLOSED TREE

Comment 21

[William Lachance (:wlach)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a40c8ec3413

Comment 22

[William Lachance (:wlach)]

(In reply to Pulsebot from comment #20)
> Backout by [e-mail]:
> https://hg.mozilla.org/integration/autoland/rev/a2a8d7ccac40
> Backed out changeset 2bc8e60fb356 for failing browser chrome tests r=backout
> on a CLOSED TREE

Very odd, I didn't notice that in my earlier try runs. But it does indeed seem to be caused by the commit. So strange that the mozbase unit tests pass though. I suspect the problem is a subtle incompatibility between six and the default python libraries. Going to test that theory.

Comment 23

[William Lachance (:wlach)]

Comment on attachment 8937031 [details]
Bug 1425399 - Added python 3 support to mozprofile.

https://reviewboard.mozilla.org/r/207762/#review215014

Nope, the problem was the mass conversion of basestring -> str. This doesn't work in python2 (since unicode is not of type 'str').

Apparently best practice is to test against six.string_types:

https://pythonhosted.org/six/#six.string_types

Also, I noticed a usage of file() which we should replace with open():

https://hg.mozilla.org/mozilla-central/file/tip/testing/mozbase/mozprofile/mozprofile/prefs.py#l215

Vedant, could you fix these problems?

Comment 24

[Vedant Chakravadhanula(:vedantc98)]

Okay, thanks for the help William. 
I'll make the changes asap.

Comment 25

[Vedant Chakravadhanula(:vedantc98)]

Comment on attachment 8937031 [details]
Bug 1425399 - Added python 3 support to mozprofile.

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

Comment 26

[Vedant Chakravadhanula(:vedantc98)]

I've added the changes, please take a look. Thanks!

Comment 27

[William Lachance (:wlach)]

Comment on attachment 8937031 [details]
Bug 1425399 - Added python 3 support to mozprofile.

https://reviewboard.mozilla.org/r/207762/#review215140

Everything looks good now on try, including the tests that failed before. A learning experience for all of us. :) I'll go ahead and land this now.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/67104a1227e4
Added python 3 support to mozprofile. r=wlach

Comment 29

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/67104a1227e4

Comment 30

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8937031 [details]
Bug 1425399 - Added python 3 support to mozprofile.

https://reviewboard.mozilla.org/r/207762/#review215288

::: testing/mozbase/mozprofile/mozprofile/prefs.py:151
(Diff revision 5)
>          elif isinstance(prefs, dict):
>              values = prefs.values()
>          else:
>              raise PreferencesReadError("Malformed preferences: %s" % path)
> -        types = (bool, basestring, int)
> +        types = (bool, string_types, int)
>          if [i for i in values if not [isinstance(i, j) for j in types]]:

Can this pre-existing code ever work? I would expect `[isinstance(i, j) for j in types]` to always return a three-element array, which is truthy.

Comment 31

[William Lachance (:wlach)]

(In reply to :Ms2ger (⌚ UTC+1/+2) from comment #30)
> Comment on attachment 8937031 [details]
> Bug 1425399 - Added python 3 support to mozprofile.
> 
> https://reviewboard.mozilla.org/r/207762/#review215288
> 
> ::: testing/mozbase/mozprofile/mozprofile/prefs.py:151
> (Diff revision 5)
> >          elif isinstance(prefs, dict):
> >              values = prefs.values()
> >          else:
> >              raise PreferencesReadError("Malformed preferences: %s" % path)
> > -        types = (bool, basestring, int)
> > +        types = (bool, string_types, int)
> >          if [i for i in values if not [isinstance(i, j) for j in types]]:
> 
> Can this pre-existing code ever work? I would expect `[isinstance(i, j) for
> j in types]` to always return a three-element array, which is truthy.

Good catch! Vedant, would you like to file a followup bug and attach a patch?

Comment 32

[Vedant Chakravadhanula(:vedantc98)]

Thanks William, I'd like to file a bug if that hasn't been done already.
Also, should I continue with the mozbase porting?

Comment 33

[William Lachance (:wlach)]

(In reply to Vedant Chakravadhanula(:vedantc98) from comment #32)
> Thanks William, I'd like to file a bug if that hasn't been done already.
> Also, should I continue with the mozbase porting?

Please do file a bug. Feel free to continue porting mozbase, though I think it might be a good idea to work on some of the automated testing around python3 (in particular bug 1388013) so that your hard work doesn't wind up breaking later. :ahal ([e-mail]) can help mentor with that part, if you're not sure how to proceed.

Comment 34

[Vedant Chakravadhanula(:vedantc98)]

Filed Bug 1431024 for the above.