Closed Bug 1388013 Opened 7 years ago Closed 6 years ago

Support running |mach python-test| against Python 3

Categories

(Testing :: Python Test, enhancement)

Product:
Testing
Component:
Python Test
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox57 wontfix, firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox62 --- fixed

People

(Reporter: davehunt, Assigned: davehunt)

References

Details

Attachments

(3 files, 3 obsolete files)

Bug 1388013 - Vendor jsmin via |mach vendor python|;
59 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 1388013 - Support running |mach python-test| against Python 3 using pipenv;
59 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 1388013 - Remove restriction of Python 2 in mozrunner;
59 bytes, text/x-review-board-request
ahal
: review+
Details 
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.
Blocks: 1388016
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)
gps answered on IRC, we'll need another virtual environment, and I think I've made some progress with this locally.
Flags: needinfo?(ahalberstadt)
Attachment #8897866 - Flags: feedback?(gps)
Attachment #8897867 - Flags: feedback?(gps)
Attachment #8897868 - Flags: feedback?(gps)
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: nobody → dave.hunt
Status: NEW → ASSIGNED
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 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 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 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)
Attachment #8897867 - Flags: feedback?(gps)
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)
Depends on: 1396582
(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.
Flags: needinfo?(gps)
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 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 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.
I'm not actively working on this, so unassigning for now. If I find time to pick it up again, I'll update here.
Assignee: dave.hunt → nobody
Status: ASSIGNED → NEW
If there's nobody working on this bug, can I take it up?
I would need a little guidance, :ahal, could you help me out?
Flags: needinfo?(ahalberstadt)
Hi Vedant, I'd be happy to help. I've assigned you the bug.

You'll notice davehunt left a patch series attached in mozreview. So the first step would be to pull those patches into your local repository and get them rebased onto the latest central (the command to pull them into your repo is listed on mozreview). If you hit conflicts and need help with the merge, let me know.

Next you'll see gps and I left a series of review comments on the review. You should try and fix them, feel free to ask if you don't understand something.

Finally you'll need to test your changes (by running |mach python-test| with python3), and submit them to your own review request. But we can worry about this step later.

Thanks for your interest!
Assignee: nobody → vedantc98
Status: NEW → ASSIGNED
Flags: needinfo?(ahalberstadt)
(In reply to Vedant Chakravadhanula(:vedantc98) from comment #17)
> If there's nobody working on this bug, can I take it up?
> I would need a little guidance, :ahal, could you help me out?

I'm also happy to help review or test any patches. Thanks for picking this up Vedant, it's something I'm very keen to see!
Thanks ahal and davehunt for the guidance, I'll work on this and get back with a patch in a while.
After working on bug 1437593 I have been able to achieve running |mach python-test| against Python 3 without too much effort. Basically I created a Pipfile in /python which lists the dependencies by path, and then it's just a case of telling pipenv to use Python 3 when creating the virtual environment. I had to make mozunit pip installable, install pytest from PyPI because the vendored version does not include setup.py.
Depends on: 1437593
Depends on: 1438250
Haven't seen any activity, re-setting assignee. Plus I think Dave might have found a better way of implementing this with pipenv.
Assignee: vedantc98 → nobody
Status: ASSIGNED → NEW
Product: Core → Firefox Build System
Hey Dave, I was thinking maybe we should just land something along the lines of the original patch series here in the meantime while we wait to figure out what to do with pipenv.

I'd like to get tests running in python 3 to fix things like bug 1428362, even if it's just using something hacky. Thoughts?
With pipenv this becomes so much easier, as we just pass the Python version to pipenv, which takes care of everything. I've updated bug 1437593 in the hope we can make some progress there. If there doesn't seem to be a way forward with pipenv then we can fall back to the original approach.
Attachment #8897866 - Attachment is obsolete: true
Attachment #8897867 - Attachment is obsolete: true
Depends on: 1458812
Attachment #8897868 - Attachment is obsolete: true
Comment on attachment 8981950 [details]
Bug 1388013 - Support running |mach python-test| against Python 3 using pipenv;

https://reviewboard.mozilla.org/r/247956/#review254090

::: python/Pipfile:6
(Diff revision 1)
> +[packages]
> +"d5b4a14" = {path = "./mach"}
> +"8ddb376" = {path = "./mozbuild"}
> +"b3ddbcf" = {path = "./mozterm"}
> +"38a4a9a" = {path = "./mozversioncontrol"}

If we got the initial virtualenv working with Pipenv, could we eventually delete this Pipfile? It's a shame we have to redefine all this from `virtualenv_packages.txt`.

::: python/mozbuild/mozbuild/virtualenv.py:548
(Diff revision 1)
>              'WORKON_HOME': os.path.join(self.topobjdir, '_virtualenvs'),
>          })
>  
> +        args = args or []
>          subprocess.check_call(
> -            [pipenv, 'install', '--deploy'],
> +            [pipenv, 'install'] + args,

It looks like we lose `--deploy` here, is that intentional? Wouldn't that break whatever was using this function before?
Attachment #8981950 - Flags: review?(ahal) → review+
Comment on attachment 8981949 [details]
Bug 1388013 - Vendor jsmin via |mach vendor python|;

https://reviewboard.mozilla.org/r/247954/#review254092
Attachment #8981949 - Flags: review?(ahal) → review+
Comment on attachment 8981951 [details]
Bug 1388013 - Remove restriction of Python 2 in mozrunner;

https://reviewboard.mozilla.org/r/247958/#review254094
Attachment #8981951 - Flags: review?(ahal) → review+
No longer depends on: 1396582
Assignee: nobody → dave.hunt
Mentor: dave.hunt
Status: NEW → ASSIGNED
Comment on attachment 8981950 [details]
Bug 1388013 - Support running |mach python-test| against Python 3 using pipenv;

https://reviewboard.mozilla.org/r/247956/#review254090

> If we got the initial virtualenv working with Pipenv, could we eventually delete this Pipfile? It's a shame we have to redefine all this from `virtualenv_packages.txt`.

If we could skip the initial virtual environment and use pipenv from the outset, then we could theoretically have this all in a single Pipfile. That would require us to have pipenv installed, as we're currently using the initial virtualenv to install it. I was thinking a more likely scenario would be that mach commands define their own Pipfiles that only contain the required dependencies for the command, and this would ultimately mean the initial virtualenv wouldn't need so many packages installed, and perhaps even just enough to run pipenv.

> It looks like we lose `--deploy` here, is that intentional? Wouldn't that break whatever was using this function before?

This was intentional, and nothing is currently using pipenv, as bug 1458461 is still open. The `--deploy` causes pipenv to abort if the Pipfile.lock is out of date (the Pipfile hash is not valid). This makes it very difficult to update the `Pipfile`, so I've made it optional by allowing arguments to be passed to `activate_pipenv`.
Pushed by dhunt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3a163da2b21b
Vendor jsmin via |mach vendor python|; r=ahal
https://hg.mozilla.org/integration/autoland/rev/c55bfefbd4e1
Support running |mach python-test| against Python 3 using pipenv; r=ahal
https://hg.mozilla.org/integration/autoland/rev/eea857170a41
Remove restriction of Python 2 in mozrunner; r=ahal
It appears that our vendored version of jsmin was closer to v2.1.0 than v2.0.11.
Flags: needinfo?(dave.hunt)
Pushed by dhunt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eccc5f4ea53e
Vendor jsmin via |mach vendor python|; r=ahal
https://hg.mozilla.org/integration/autoland/rev/317c77708b22
Support running |mach python-test| against Python 3 using pipenv; r=ahal
https://hg.mozilla.org/integration/autoland/rev/930b30d0d0aa
Remove restriction of Python 2 in mozrunner; r=ahal
Component: General → Python Test
Product: Firefox Build System → Testing
Blocks: 1428362
https://hg.mozilla.org/mozilla-central/rev/eccc5f4ea53e
https://hg.mozilla.org/mozilla-central/rev/317c77708b22
https://hg.mozilla.org/mozilla-central/rev/930b30d0d0aa
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: