Support running |mach python-test| against Python 3

ASSIGNED
Assigned to

Status

()

Core
Build Config
ASSIGNED
4 months ago
3 months ago

People

(Reporter: davehunt, Assigned: davehunt, Mentored)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

4 months ago
Add a --python3 command line option to |mach python-test| to run the tests using Python 3. By default, tests should not run unless python3=true is included in the manifest file.

The Python path is currently used in python/mach_commands.py here: https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/python/mach_commands.py#211. We need to include logic to switch to a Python 3 path when the command line option is present and the manifest file indicates that the test supports Python 3.
(Assignee)

Updated

4 months ago
Blocks: 1388016
(Assignee)

Comment 1

4 months ago
Swapping out the self.virtualenv_manager.python_path for a path to a Python 3 binary fails because the necessary dependencies such as mozunit are missing. Should we create an additional virtual environment manager based on Python 3 for use by python-tests when the new option is specified?
Flags: needinfo?(ahalberstadt)
(Assignee)

Comment 2

4 months ago
gps answered on IRC, we'll need another virtual environment, and I think I've made some progress with this locally.
Flags: needinfo?(ahalberstadt)

Updated

4 months ago
Blocks: 1388447
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8897866 - Flags: feedback?(gps)
(Assignee)

Updated

4 months ago
Attachment #8897867 - Flags: feedback?(gps)
(Assignee)

Updated

4 months ago
Attachment #8897868 - Flags: feedback?(gps)
(Assignee)

Comment 6

4 months ago
Looking for some feedback on the attached MozReview patches. Currently I am experiencing an issue with pyc files. This can be resolved by setting PYTHONDONTWRITEBYTECODE [1] or by cleaning these up using |find . -name '*.pyc' -delete|.

I'm also looking for advice on how to take advantage of bug 1385381 to avoid hard-coding the path to Python 3, and general feedback on whether I'm along the right lines with these patches.

[1]: https://docs.python.org/3/using/cmdline.html#envvar-PYTHONDONTWRITEBYTECODE
(Assignee)

Updated

4 months ago
Assignee: nobody → dave.hunt
Status: NEW → ASSIGNED

Comment 7

4 months ago
mozreview-review
Comment on attachment 8897866 [details]
Bug 1388013 - Add support for Python 3 to mozunit

https://reviewboard.mozilla.org/r/169156/#review174730

This looks mostly good.

::: config/mozunit.py:13
(Diff revision 1)
> -from StringIO import StringIO
>  import os
>  import sys
>  
> +import six
> +from six import StringIO

Nit: `StringIO = six.StringIO`.

(Importing the same module twice feels weird and engages import machinery, which is more expensive than just assigning a variable from an attribute.)

Also, StringIO is type sensitive in Python 3. By the time you fix callers to pass consistent types, it is better to use `io.BytesIO` or `io.StringIO`. Unless the code is performance sensitive: in that case I think Python 2's `cStringIO.StringIO` is a bit faster than `io.BytesIO` or `io.StringIO`. I could be wrong about that though.

::: config/mozunit.py:148
(Diff revision 1)
>          f.write('foo')
>      self.assertRaises(Exception,f.open('foo', 'r'))
>      '''
>      def __init__(self, files = {}):
>          self.files = {}
> -        for name, content in files.iteritems():
> +        for name, content in six.iteritems(files):

Meh. Just use `files.items()`. `iteritems()` is only useful for perf reasons if the item is large. This being test code for a mock, we don't care.

Comment 8

4 months ago
mozreview-review
Comment on attachment 8897867 [details]
Bug 1388013 - Add support for Python 3 to mozbuild

https://reviewboard.mozilla.org/r/169158/#review174736

::: python/mozbuild/mozbuild/virtualenv.py:26
(Diff revision 1)
>  IS_CYGWIN = (sys.platform == 'cygwin')
>  
>  # Minimum version of Python required to build.
> -MINIMUM_PYTHON_VERSION = LooseVersion('2.7.3')
> -MINIMUM_PYTHON_MAJOR = 2
> +MINIMUM_PYTHON_VERSION = {
> +    2: LooseVersion('2.7.3'),
> +    3: LooseVersion('3.6.0')}

We set this as 3.5.0 in python.py in the same directory. Let's not require 3.6 just yet.

::: python/mozbuild/mozbuild/virtualenv.py:535
(Diff revision 1)
> -    if major != MINIMUM_PYTHON_MAJOR or our < MINIMUM_PYTHON_VERSION:
> -        log_handle.write('Python %s or greater (but not Python 3) is '
> -            'required to build. ' % MINIMUM_PYTHON_VERSION)
> +    if major not in MINIMUM_PYTHON_VERSION or \
> +            our < MINIMUM_PYTHON_VERSION[major]:
> +        log_handle.write('Python %s or greater is required to build. ' %
> +                         MINIMUM_PYTHON_VERSION.get(
> +                            major, MINIMUM_PYTHON_VERSION[2]))

This function is called in two places:

1) configure
2) this file as part of creating a virtualenv

For configure, we require Python 2 but Python 3 is optional. So the new error message is a bit misleading.

Honestly, it feels like this function should be moved into build/moz.configure/init.configure. I don't think there are many callers of this file as a standalone module (`python -m mozbuild.virtualenv`). The call to this function in this file could likely be removed and the entirety of the code moved to init.configure. moz.configure is a bit overwhelming for newcomers. So if you file a bug to move the code and needinfo me, I can do that for you.

Comment 9

4 months ago
mozreview-review
Comment on attachment 8897868 [details]
Bug 1388013 - Support running |mach python-test| against Python 3

https://reviewboard.mozilla.org/r/169154/#review174738

::: python/mach_commands.py:94
(Diff revision 1)
>                           verbose=False,
>                           stop=False,
> -                         jobs=1):
> +                         jobs=1,
> +                         py3=False):
> +        if py3:
> +            _py3_path = '/Users/dhunt/.pyenv/versions/3.6.2/bin/python'

This should use `self.python`.

::: python/mach_commands.py:97
(Diff revision 1)
> +                         py3=False):
> +        if py3:
> +            _py3_path = '/Users/dhunt/.pyenv/versions/3.6.2/bin/python'
> +            self.virtualenv_manager.virtualenv_root += '_py3'
> +            self.virtualenv_manager.ensure(python=_py3_path)
> +            self.virtualenv_manager.activate()

I doubt this line does what you think it does. The above line should create a virtualenv. Whether it populates the virtualenv properly, I'm not sure. But this line almost certainly doesn't do the right thing. (It is meant to update Python variables so the current process "inherits" the virtualenv. But it won't swap out the running executable.)

Comment 10

4 months ago
Comment on attachment 8897866 [details]
Bug 1388013 - Add support for Python 3 to mozunit

Feedback doesn't work in MozReview. So just r? next time and leave a comment in the commit message that the patch is incomplete and I'll cancel or r- it.
Attachment #8897866 - Flags: feedback?(gps)

Updated

4 months ago
Attachment #8897867 - Flags: feedback?(gps)

Comment 11

4 months ago
Comment on attachment 8897868 [details]
Bug 1388013 - Support running |mach python-test| against Python 3

This one is a bit wonkier. Not sure how we'll invoke Python 3 here. mach commands are ideally written as minimal dispatch methods. So one way to handle this would be to extract all the Python for invoking the test harness to a module and then run `python3 -m <module> <args>` to invoke it using Python 3.
Attachment #8897868 - Flags: feedback?(gps)
(Assignee)

Updated

3 months ago
Depends on: 1396582
(Assignee)

Comment 12

3 months ago
(In reply to Gregory Szorc [:gps] from comment #11)
> Comment on attachment 8897868 [details]
> Bug 1388013 - Support running |mach python-test| against Python 3
> 
> This one is a bit wonkier. Not sure how we'll invoke Python 3 here. mach
> commands are ideally written as minimal dispatch methods. So one way to
> handle this would be to extract all the Python for invoking the test harness
> to a module and then run `python3 -m <module> <args>` to invoke it using
> Python 3.

Do you have an example of another mach command that works in this way? Does this mean we wouldn't invoke the tests using mach at all? I got the impression that the virtual environment would use Python 2.7 even when Python 3 is available, so assumed I would need to create a Python 3 virtual environment in order to run tests against Python 3.
(Assignee)

Updated

3 months ago
Flags: needinfo?(gps)

Comment 13

3 months ago
AFAIK we don't yet have a mach command that executes Python 3. But it would look something like:

def command(self):
    py3_bin, py3_version = self.python3
    if not py3_bin:
        print('cannot find Python 3!')
        return 1

    env = dict(os.environ)
    env['PYTHONPATH'] = ':'.join([
        # paths of Python packages
    ])
    return self.run_process([py3_bin, '-u', '-m', 'mypackage.mymodule'] + args,
                            env=env)


You could also write out a temporary JSON file with argument values or something.

Let's get something landed first. We can add magic to make this all more turnkey for multiple mach commands later.
Flags: needinfo?(gps)

Comment 14

3 months ago
mozreview-review
Comment on attachment 8897868 [details]
Bug 1388013 - Support running |mach python-test| against Python 3

https://reviewboard.mozilla.org/r/169154/#review181766

::: python/mach_commands.py:145
(Diff revision 1)
> +        def _py3(tests, values):
> +            for test in tests:
> +                if 'py3' in test:
> +                    yield test
> +
>          filters = []
> +        if py3:
> +            filters.append(_py3)

Instead of creating a new filter, let's re-use skip-if:

    mozinfo.info['py3'] = True
    tests = mp.active_tests(..., **mozinfo.info)

Then in python.ini:

    [test_foo.py]
    skip-if = py3

Comment 15

3 months ago
mozreview-review
Comment on attachment 8897866 [details]
Bug 1388013 - Add support for Python 3 to mozunit

https://reviewboard.mozilla.org/r/169156/#review181770

::: commit-message-0fbd5:1
(Diff revision 1)
> +Bug 1388013 - Add support for Python 3 to mozunit

Alternatively, if we fix bug 1395630 all this code can be deleted.

Updated

3 months ago
status-firefox57: --- → wontfix
You need to log in before you can comment on or make changes to this bug.