1428362 - Py3 support for python/mozterm

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Andrew Halberstadt [:ahal]]

We recently added py3 support to mozlog, but then I added a mozterm as a dependency and it broke again. So we'll also need to support py3 in mozterm.

Bug 13881013 tracks running our unittests in python3 via |mach python-test|. We might want to block on that before spending too much time converting stuff over.

Comment 2

[Ed Morley [:emorley]]

There are possibly more instances, but the one Python 3 incompatibility we've seen so far is:

../python/lib/python3.6/site-packages/_pytest/assertion/rewrite.py:213: in load_module
    py.builtin.exec_(co, mod.__dict__)
../python/lib/python3.6/site-packages/mozlog/formatters/machformatter.py:10: in <module>
    from mozterm import Terminal
../python/lib/python3.6/site-packages/mozterm/__init__.py:6: in <module>
    from .terminal import Terminal, NullTerminal  # noqa
../python/lib/python3.6/site-packages/mozterm/terminal.py:11: in <module>
    class NullTerminal(object):
../python/lib/python3.6/site-packages/mozterm/terminal.py:24: in NullTerminal
    class NullCallableString(unicode):
E   NameError: name 'unicode' is not defined

Comment 3

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

Attachment: Bug 1428362 - Vendor blessings via |mach vendor python|;

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

Comment 4

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

Attachment: Bug 1428362 - Add Python 3 support to mozterm;

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

Comment 5

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

Attachment: Bug 1428362 - Run modernize against mozlog to support Python 3;

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

Comment 6

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

Attachment: Bug 1428362 - Run mozterm tests against Python 3;

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

Comment 7

[Andrew Halberstadt [:ahal]]

Comment on attachment 8984524 [details]
Bug 1428362 - Add Python 3 support to mozterm;

https://reviewboard.mozilla.org/r/250394/#review256710

Comment 8

[Andrew Halberstadt [:ahal]]

Comment on attachment 8984523 [details]
Bug 1428362 - Vendor blessings via |mach vendor python|;

https://reviewboard.mozilla.org/r/250392/#review256712

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8984525 [details]
Bug 1428362 - Run modernize against mozlog to support Python 3;

https://reviewboard.mozilla.org/r/250396/#review256716

This looks good to me. But I feel like using `six.iteritems/itervalues` is likely overkill. The benefit of using an iterator in most of these cases is likely very tiny, and it might be cleaner to just use `items()/values()`. That said, this will minimize the risk of regressing performance, so it's probably best to just leave it as is for now.

Comment 10

[Andrew Halberstadt [:ahal]]

Comment on attachment 8984526 [details]
Bug 1428362 - Run mozterm tests against Python 3;

https://reviewboard.mozilla.org/r/250398/#review256724

::: taskcluster/ci/source-test/python.yml:133
(Diff revision 1)
>          mach: python-test --subsuite mozterm
>      when:
>          files-changed:
>              - 'python/mozterm/**'
>  
> +mozterm-py3:

Instead of duplicating task definitions, you should create a new transform function in this file:
https://searchfox.org/mozilla-central/source/taskcluster/taskgraph/transforms/source_test.py

A transform function can output more jobs than were input. So I'd invent some sort of "python" key in the task config, e.g python: [2, 3]

Then the transform function would take that value and create duplicate jobs for each version of python it can run against (also modifying the treeherder symbol and mach command appropriately).

I'm happy to help with this if you like, it can be a bit tricky to get right.

Comment 11

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

(In reply to Andrew Halberstadt [:ahal] from comment #10)
> Comment on attachment 8984526 [details]
> Bug 1428362 - Run mozterm tests against Python 3;
> 
> https://reviewboard.mozilla.org/r/250398/#review256724
> 
> ::: taskcluster/ci/source-test/python.yml:133
> (Diff revision 1)
> >          mach: python-test --subsuite mozterm
> >      when:
> >          files-changed:
> >              - 'python/mozterm/**'
> >  
> > +mozterm-py3:
> 
> Instead of duplicating task definitions, you should create a new transform
> function in this file:
> https://searchfox.org/mozilla-central/source/taskcluster/taskgraph/
> transforms/source_test.py
> 
> A transform function can output more jobs than were input. So I'd invent
> some sort of "python" key in the task config, e.g python: [2, 3]
> 
> Then the transform function would take that value and create duplicate jobs
> for each version of python it can run against (also modifying the treeherder
> symbol and mach command appropriately).
> 
> I'm happy to help with this if you like, it can be a bit tricky to get right.

In that case I can take care of this in bug 1388016. If you have any examples or pointers, please could you add a comment over there?

Comment 12

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

Comment on attachment 8984523 [details]
Bug 1428362 - Vendor blessings via |mach vendor python|;

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

Comment 13

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

Comment on attachment 8984524 [details]
Bug 1428362 - Add Python 3 support to mozterm;

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

Comment 14

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

Comment on attachment 8984525 [details]
Bug 1428362 - Run modernize against mozlog to support Python 3;

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

Comment 15

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

Comment on attachment 8984523 [details]
Bug 1428362 - Vendor blessings via |mach vendor python|;

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

Comment 16

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

Comment on attachment 8984524 [details]
Bug 1428362 - Add Python 3 support to mozterm;

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

Comment 17

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

Comment on attachment 8984525 [details]
Bug 1428362 - Run modernize against mozlog to support Python 3;

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64b4279078fa
Vendor blessings via |mach vendor python|; r=ahal
https://hg.mozilla.org/integration/autoland/rev/a40bfde2ac69
Add Python 3 support to mozterm; r=ahal
https://hg.mozilla.org/integration/autoland/rev/9cf851623091
Run modernize against mozlog to support Python 3; r=ahal

Comment 19

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/64b4279078fa
https://hg.mozilla.org/mozilla-central/rev/a40bfde2ac69
https://hg.mozilla.org/mozilla-central/rev/9cf851623091

Comment 20

[Ed Morley [:emorley]]

Would it be possible to publish a new release of mozterm at some point? :-)

Comment 21

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

Comment on attachment 8984525 [details]
Bug 1428362 - Run modernize against mozlog to support Python 3;

https://reviewboard.mozilla.org/r/250396/#review259570

::: testing/mozbase/mozlog/mozlog/commandline.py:267
(Diff revision 3)
>                  continue
>              if len(parts) == 2:
>                  _, formatter = parts
>                  for value in values:
>                      found = True
> -                    if isinstance(value, basestring):
> +                    if isinstance(value, six.string_types):

Note that this is true for bytes in py2, but not py3.

::: testing/mozbase/mozlog/mozlog/formatters/machformatter.py:65
(Diff revision 3)
>          if isinstance(test_id, list):
>              test_id = tuple(test_id)
>          return test_id
>  
>      def _get_file_name(self, test_id):
> -        if isinstance(test_id, (str, unicode)):
> +        if isinstance(test_id, (str, six.text_type)):

That's `str` twice in py3.

::: testing/mozbase/mozlog/mozlog/formatters/tbplformatter.py:261
(Diff revision 3)
>          time = int((data["time"] - start_time) / 1000)
>  
>          return "SUITE-END | took %is\n" % time
>  
>      def test_id(self, test_id):
> -        if isinstance(test_id, (str, unicode)):
> +        if isinstance(test_id, (str, six.text_type)):

Ditto.

::: testing/mozbase/mozlog/mozlog/formatters/xunit.py:13
(Diff revision 3)
>  
>  
>  def format_test_id(test_id):
>      """Take a test id and return something that looks a bit like
>      a class path"""
> -    if type(test_id) not in types.StringTypes:
> +    if not isinstance(test_id, six.string_types):

Ditto

::: testing/mozbase/mozlog/mozlog/handlers/base.py:103
(Diff revision 3)
>          if not formatted:
>              return
>          with self._lock:
> -            if isinstance(formatted, unicode):
> +            if isinstance(formatted, six.text_type):
>                  self.stream.write(formatted.encode("utf-8", "replace"))
>              elif isinstance(formatted, str):

Should be `six.binary_type`

::: testing/mozbase/mozlog/mozlog/logtypes.py:163
(Diff revision 3)
>  class Unicode(DataType):
>  
>      def convert(self, data):
> -        if isinstance(data, unicode):
> +        if isinstance(data, six.text_type):
>              return data
>          if isinstance(data, str):

Ditto

::: testing/mozbase/mozlog/mozlog/logtypes.py:173
(Diff revision 3)
>  class TestId(DataType):
>  
>      def convert(self, data):
> -        if isinstance(data, unicode):
> +        if isinstance(data, six.text_type):
>              return data
>          elif isinstance(data, bytes):

I didn't even know `bytes` worked in py2.

::: testing/mozbase/mozlog/mozlog/logtypes.py:220
(Diff revision 3)
>  class List(ContainerType):
>  
>      def convert(self, data):
>          # while dicts and strings _can_ be cast to lists,
>          # doing so is likely not intentional behaviour
> -        if isinstance(data, (basestring, dict)):
> +        if isinstance(data, (six.string_types, dict)):

Ditto

::: testing/mozbase/mozlog/mozlog/pytest_mozlog/plugin.py:93
(Diff revision 3)
>              status = 'FAIL' if report.when == 'call' else 'ERROR'
>          if report.skipped:
>              status = 'SKIP' if not hasattr(report, 'wasxfail') else 'FAIL'
>          if report.longrepr is not None:
>              longrepr = report.longrepr
> -            if isinstance(longrepr, basestring):
> +            if isinstance(longrepr, six.string_types):

Ditto

::: testing/mozbase/mozlog/mozlog/scripts/unstable.py:19
(Diff revision 3)
>          self.run_info = None
>          self.statuses = defaultdict(lambda: defaultdict(
>              lambda: defaultdict(lambda: defaultdict(int))))
>  
>      def test_id(self, test):
> -        if type(test) in (str, unicode):
> +        if type(test) in (str, six.text_type):

Ditto

Comment 22

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

Comment on attachment 8984525 [details]
Bug 1428362 - Run modernize against mozlog to support Python 3;

https://reviewboard.mozilla.org/r/250396/#review259570

> Note that this is true for bytes in py2, but not py3.

This was just running modernizr over mozlog to unblock us from running the tests against Python 3... Support for Python 3 will be added to mozlog soon, now that we can run the tests in CI.

Comment 23

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

> Would it be possible to publish a new release of mozterm at some point? :-)

Yes, once bug 1388016 is resolved.

Comment 24

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

(In reply to Dave Hunt [:davehunt] ⌚️UTC+0 from comment #22)
> Comment on attachment 8984525 [details]
> Bug 1428362 - Run modernize against mozlog to support Python 3;
> 
> https://reviewboard.mozilla.org/r/250396/#review259570
> 
> > Note that this is true for bytes in py2, but not py3.
> 
> This was just running modernizr over mozlog to unblock us from running the
> tests against Python 3... Support for Python 3 will be added to mozlog soon,
> now that we can run the tests in CI.

Once bug 1388016 is resolved I will be going through all the mozbase packages that have tests failing against Python 3 (all of them) and either raising bugs to ge the tests passing against Python 3, or updating existing bugs with details of how to run the tests, etc. I will transplant the issues from comment 21 for the mozlog bug to ensure these do not get missed. I also intend to publish a blog post with the intent to engage the community to help us with getting mozbase updated to support modern Python.