Closed Bug 1388019 Opened 7 years ago Closed 6 years ago

[mozlog] Add support for Python 3

Categories

(Testing :: Mozbase, enhancement)

Product:
Testing
Component:
Mozbase
Version:
Version 3
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: emorley, Assigned: vedantc98)

References

Details

Attachments

(1 file)

Bug 1388019 - Adding Python 3 support for mozlog.
59 bytes, text/x-review-board-request
wlach
: review+
Details 
Without dropping support for legacy Python, we need to add support for Python 3 to mozlog. Since mozlog depends on mozfile, we likely need to fix bug 1388018 first too.

Amongst other things, this will help Treeherder switch to Python 3 (mozlog is the only mozbase package it uses).
Blocks: 1330474
Hey, I've added a patch with a few initial changes that are required to add py3 support.
Please let me know what else is required.

I'm unable to use python-test to test the module because it gives me an error as follows :
OSError: [Errno 62] Too many levels of symbolic links: '/Users/vedantc98/src/firefox/obj-x86_64-apple-darwin16.7.0/_virtualenv/bin/python'

I'm running macOS.
Thanks.
Flags: needinfo?(emorley)
Please see bug 1416924 which might help you to get the error fixed.
I've fixed the problem with python-test, and so I've fixed the error.
Further, I've run the code through a py2 and py3 linter using `mach lint -l py2 -l py3 testing/mozbase/mozlog/ `.

Since the updated review request returns no errors on the linter as well as python-test, mozlog should be ready for python 3 after Bug 1388018 is resolved.
Comment on attachment 8936759 [details]
Bug 1388019 - Adding Python 3 support for mozlog.

Hi! Thank you for the patch. I trust Henrik to review this more than me :-)
Flags: needinfo?(emorley)
Attachment #8936759 - Flags: review?(emorley) → review?(hskupin)
Assignee: nobody → vedantc98
Blocks: 1425399
I'm sorry about the review flag, please could you set it back to Henrik? This patch was pushed by accident..
Comment on attachment 8936759 [details]
Bug 1388019 - Adding Python 3 support for mozlog.

No problem - restoring now :-)
Attachment #8936759 - Flags: review?(emorley) → review?(hskupin)
Attachment #8936759 - Flags: review?(hskupin) → review?(wlachance)
I'm barely around until early next year and William helped you already with other patches. So he is better suited to review this one. Thanks for working on it Verdant!
Status: NEW → ASSIGNED
Comment on attachment 8936759 [details]
Bug 1388019 - Adding Python 3 support for mozlog.

https://reviewboard.mozilla.org/r/207480/#review214478

::: testing/mozbase/mozlog/mozlog/unstructured/loglistener.py
(Diff revision 3)
>  # You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  from __future__ import absolute_import
>  
> -import SocketServer
> +from six.moves import socketserver
> -import socket

I think we still need 'socket' here, since we still catch an exception from it below.

::: testing/mozbase/mozlog/setup.py:27
(Diff revision 3)
>        author_email='tools@lists.mozilla.org',
>        url='https://wiki.mozilla.org/Auto-tools/Projects/Mozbase',
>        license='MPL 1.1/GPL 2.0/LGPL 2.1',
>        packages=find_packages(),
>        zip_safe=False,
> -      install_requires=["blessings>=1.3"],
> +      install_requires=deps,

I don't see a need to break deps into a variable here, I think it's more clear just to define them inline.

::: testing/mozbase/mozlog/setup.py
(Diff revision 3)
>                     'Intended Audience :: Developers',
>                     'License :: OSI Approved :: Mozilla Public License 1.1 (MPL 1.1)',
>                     'Operating System :: OS Independent',
> -                   'Topic :: Software Development :: Libraries :: Python Modules',
> +                   'Topic :: Software Development :: Libraries :: Python Modules'],
> -                   'Programming Language :: Python :: 2.7',
> -                   'Programming Language :: Python :: 2 :: Only'],

Could you set the version classifiers as we did for mozprofile and mozfile?
Attachment #8936759 - Flags: review?(wlachance) → review-
Added the changes, plus a couple that i had missed earlier.
Thanks!
Comment on attachment 8936759 [details]
Bug 1388019 - Adding Python 3 support for mozlog.

Reassigning since Will did the previous review round :-)
Attachment #8936759 - Flags: review?(emorley) → review?(wlachance)
Comment on attachment 8936759 [details]
Bug 1388019 - Adding Python 3 support for mozlog.

https://reviewboard.mozilla.org/r/207480/#review215138

Looks pretty close here too! I'll trigger another try run.

::: testing/mozbase/mozlog/setup.py:25
(Diff revision 4)
>        packages=find_packages(),
>        zip_safe=False,
> -      install_requires=["blessings>=1.3"],
> -      tests_require=['mozfile'],
> +      install_requires=['blessings >= 1.3',
> +                        'six >= 1.10.0'],
> +      tests_require=['mozfile',
> +                     'six>=1.10.0'

Since you specified six in install_requires, you shouldn't need to specify it again in tests_require.
Attachment #8936759 - Flags: review?(wlachance) → review-
I've made the changes and pushed them.
Comment on attachment 8936759 [details]
Bug 1388019 - Adding Python 3 support for mozlog.

https://reviewboard.mozilla.org/r/207480/#review215542

looks good now, thank you!
Attachment #8936759 - Flags: review?(wlachance) → review+
Pushed by wlachance@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ba4c8c662479
Adding Python 3 support for mozlog. r=wlach
https://hg.mozilla.org/mozilla-central/rev/ba4c8c662479
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1447735
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: