1458812 - Use pipenv to manage environment for mach python-test command

Status: RESOLVED DUPLICATE of bug 1388013

(Firefox Build System :: General, enhancement)

Description

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

Splitting this out as a blocker for bug 1388013. Here I'll aim to get the |mach python-test| command to use pipenv to manage the environment using Python 2.7, which we can then build on for optionally switching to Pyhton 3.

Comment 1

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

Attachment: Bug 1458812 - Vendor pytest 3.5.1;

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

Comment 2

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

Attachment: Bug 1458812 - Vendor py 1.5.3;

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

Comment 3

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

Attachment: Bug 1458812 - Vendor attrs 17.4.0;

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

Comment 4

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

Attachment: Bug 1458812 - Vendor funcsigs 1.0.2;

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

Comment 5

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

Attachment: Bug 1458812 - Vendor pluggy 0.6.0;

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

Comment 6

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

Attachment: Bug 1458812 - Vendor more-itertools 4.1.0;

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

Comment 7

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

Attachment: Bug 1458812 - Vendor jsmin 2.2.2;

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

Comment 8

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

Attachment: Bug 1458812 - Add mach.mixin to mach distribution;

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

Comment 9

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

Attachment: Bug 1458812 - Make mozversioncontrol pip installable;

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

Comment 10

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

Attachment: Bug 1458812 - Allow using pipenv to manage environment for mach python-test command;

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

Comment 11

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

Attachment: Bug 1458812 - Run mozbase python tests using pipenv;

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

Comment 12

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

Initially I wanted to make this switch |mach python-test| to pipenv, but there are several subsuites that will take additional work. For example, I hit issues running the mach tests due to some mach packages not being included in the distribution, leading to some potential refactoring so that these packages are included, but without including the tests themselves. I managed to get some working without too much trouble, such as taskgraph and marionette harness, but rather than fight my way through these I decided to make pipenv a command line opt-in for now. I was thinking that once all of the suites are capable of running through pipenv, then we can remove this option and make it the default behaviour. For now, I've focused on the mozbase tests.

Comment 13

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

Here's a try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e614cdd2fa1a92b1234942fe45c0a91707ef7bd2

Comment 14

[Andrew Halberstadt [:ahal]]

Comment on attachment 8974356 [details]
Bug 1458812 - Vendor pytest 3.5.1;

https://reviewboard.mozilla.org/r/242698/#review248540

::: third_party/python/pytest/.coveragerc:1
(Diff revision 1)
> +[run]
> +omit =
> +    # standlonetemplate is read dynamically and tested by test_genscript
> +    *standalonetemplate.py

Could you vendor the unzipped wheels instead of the source distributions? This way it'll exclude all of the extra repo and test files, saving a bit of space.

I normally run:

    $ cd third_party/python
    $ pip wheel pytest==3.5.1  # this also downloads dependency wheels
    
Then for each package:

    $ rm -rf pytest
    $ unzip pytest-3.5.1-py2.py3-none-any.whl -d pytest -x "*.dist-info/*"
    $ unzip pytest-3.5.1-py2.py3-none-any.whl -d pytest "*.dist-info/LICENSE.*"
    $ mv pytest/*.dist-info/* pytest
    $ hg addremove pytest

We should probably put this in a script somewhere, at least until we implement a proper `mach vendor python` that uses pipenv. I just added the LICENSE step, so for many of these packages, it'll be a new file being added.

p.s, I don't really care if these are split out into separate commits or not. So feel free to vendor pytest and all the dependencies in the same commit if that's easier for you.

Comment 15

[Andrew Halberstadt [:ahal]]

Comment on attachment 8974356 [details]
Bug 1458812 - Vendor pytest 3.5.1;

https://reviewboard.mozilla.org/r/242698/#review248540

> Could you vendor the unzipped wheels instead of the source distributions? This way it'll exclude all of the extra repo and test files, saving a bit of space.
> 
> I normally run:
> 
>     $ cd third_party/python
>     $ pip wheel pytest==3.5.1  # this also downloads dependency wheels
>     
> Then for each package:
> 
>     $ rm -rf pytest
>     $ unzip pytest-3.5.1-py2.py3-none-any.whl -d pytest -x "*.dist-info/*"
>     $ unzip pytest-3.5.1-py2.py3-none-any.whl -d pytest "*.dist-info/LICENSE.*"
>     $ mv pytest/*.dist-info/* pytest
>     $ hg addremove pytest
> 
> We should probably put this in a script somewhere, at least until we implement a proper `mach vendor python` that uses pipenv. I just added the LICENSE step, so for many of these packages, it'll be a new file being added.
> 
> p.s, I don't really care if these are split out into separate commits or not. So feel free to vendor pytest and all the dependencies in the same commit if that's easier for you.

Talked to davehunt offline. He mentioned two reasons for using source distributions over wheels:
1) Pipenv needs a setup.py to install from
2) Binary wheels are not always compatible with both python 2 and python 3

Given that, I think it's fine if these extra files are vendored in for now. Eventually I'd like if we could solve this problem in the future `mach vendor python` command (maybe by using setuptools programmatically). But for now it's not worth worrying about.

Comment 16

[Andrew Halberstadt [:ahal]]

Comment on attachment 8974365 [details]
Bug 1458812 - Allow using pipenv to manage environment for mach python-test command;

https://reviewboard.mozilla.org/r/242716/#review248546

::: python/Pipfile:7
(Diff revision 1)
> +url = "https://pypi.org/simple"
> +verify_ssl = true
> +name = "pypi"
> +
> +[packages]
> +"d5b4a14" = {path = "./mach"}

How did you generate these identifiers? Could you put the steps for adding a new package in the commit message?

::: python/mach_commands.py:70
(Diff revision 1)
> +    @CommandArgument('--pipenv',
> +                     default=False,
> +                     action='store_true',
> +                     help='Use pipenv to manage the virtual environment.')

Is there a reason this needs to be an option? I'd prefer to just make it the *only* way. Or will the non-pipenv path be removed in a follow-up bug?

Comment 17

[Andrew Halberstadt [:ahal]]

Comment on attachment 8974363 [details]
Bug 1458812 - Add mach.mixin to mach distribution;

https://reviewboard.mozilla.org/r/242712/#review248548

::: python/mach/setup.py:23
(Diff revision 1)
>      long_description=README,
>      license='MPL 2.0',
>      author='Gregory Szorc',
>      author_email='[e-mail]',
>      url='https://developer.mozilla.org/en-US/docs/Developer_Guide/mach',
> -    packages=['mach'],
> +    packages=['mach', 'mach.mixin'],

Why is this change needed? Shouldn't the mixin package already be accessible via `from mach import mixin`?

Please also add the motivation for this to the commit message.

Comment 18

[Andrew Halberstadt [:ahal]]

Comment on attachment 8974364 [details]
Bug 1458812 - Make mozversioncontrol pip installable;

https://reviewboard.mozilla.org/r/242714/#review248550

Comment 19

[Andrew Halberstadt [:ahal]]

Comment on attachment 8974366 [details]
Bug 1458812 - Run mozbase python tests using pipenv;

https://reviewboard.mozilla.org/r/242718/#review248552

Comment 20

[Andrew Halberstadt [:ahal]]

Comment on attachment 8974356 [details]
Bug 1458812 - Vendor pytest 3.5.1;

https://reviewboard.mozilla.org/r/242698/#review248608

::: third_party/python/pytest/doc/en/Makefile:1
(Diff revision 1)
> +# Makefile for Sphinx documentation

As discussed on irc I'd like if we could at least remove the doc and test dirs (misc top-level files are probably fine to keep).

I'm going to unflag myself from the other vendor commits for the time being to keep my review queue sane.

Comment 21

[Andrew Halberstadt [:ahal]]

Comment on attachment 8974365 [details]
Bug 1458812 - Allow using pipenv to manage environment for mach python-test command;

https://reviewboard.mozilla.org/r/242716/#review248610

Comment 22

[Andrew Halberstadt [:ahal]]

Comment on attachment 8974365 [details]
Bug 1458812 - Allow using pipenv to manage environment for mach python-test command;

https://reviewboard.mozilla.org/r/242716/#review248546

> Is there a reason this needs to be an option? I'd prefer to just make it the *only* way. Or will the non-pipenv path be removed in a follow-up bug?

Missed the explanation for this in the bug. Please resolve this issue by copying that explanation into the commit message.

Comment 23

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

There are some issues with running the existing tests using pipenv, which will need to be resolved when adding support for Python 3. For this reason, I don't think it's worthwhile trying to use pipenv for running against Python 2 at this time. Resolving as a duplicate of bug 1388013, which will use pipenv to run against Python 3.