Closed Bug 1466211 Opened 5 years ago Closed 5 years ago

Use pipenv in |mach python-test| by default

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Version:
3 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: davehunt, Assigned: davehunt)

References

Details

Attachments

(7 files, 12 obsolete files)

Bug 1466211 - Vendor requests via |mach vendor python|;
59 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 1466211 - Vendor json-e via |mach vendor python|;
59 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 1466211 - Vendor voluptuous via |mach vendor python|;
59 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 1466211 - Sort dependencies in Pipfile;
59 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 1466211 - Switch all |mach python-test| tests to run using pipenv;
59 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 1466211 - Use --python for selecting target Python when using |mach python-test|;
59 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 1466211 - Detect if we are running in a virtual environment;
59 bytes, text/x-review-board-request
Details 
Currently the |mach python-test| command will only use pipenv when the --three command line option is passed. This is because various subsuites have issues running in pipenv due to the way the packages are laid out or the tests are written. I was hoping that we might be able to come back to these after getting some Python 3 tasks running, but having two ways of running the tests is causing issues in CI that would be resolved by having consistency.

Some of the changes are trivial, such as mozlint (adding pyyaml as a dependency, and adding mozlint.formatters to the packages in setup.py), but others will be more involved. I'll try to do a single series of patches associated with this bug, but if there's anything particularly big I'll split it out as a dependency.
Blocks: mozbase-py3, 1388016, 1428362, 1465887
Depends on: 1463834
No longer depends on: mozbase-py3, 1388016, 1428362, 1465887
Apologies for the long patch series. I'd like to do this in one go, to avoid needing to opt-in to using pipenv for |mach python-test|. I believe I have all the TaskCluster jobs passing now, except the build, which I would appreciate some assistance deciphering and reproducing the issues locally. If you think it's better to land what I have, then I could add a --two command line option to the mach command, which acts much like --three and opts into pipenv. Then we can tackle the build issues separately, and move on to adding Python 3 support to the tasks that work under pipenv.

Here's my latest try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df0edfeec4aa22dab317710fb6950a1870f365d5
Flags: needinfo?(ahal)
It appears that the build failure was caused by pip running from the wrong directory. By forcing this to use topsrcdir I now get test failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2b4be4b7a67bb8c705d38f3bd2c167c0bd2e0f2&selectedJob=182161830

Some of these failures should be easy to fix by adding packages much like I have for other tasks, however it appears that there are several modules that don't appear in a package, such as mozwebidlcodegen and printprereleasesuffix. Do you have any suggestions for handling these?
Comment on attachment 8983939 [details]
Bug 1466211 - Switch all |mach python-test| tests to run using pipenv;

https://reviewboard.mozilla.org/r/248988/#review256228

::: config/mozunit/mozunit/mozunit.py
(Diff revision 1)
>              args.append('--color=yes')
>  
>          module = __import__('__main__')
>          args.extend([
>              '-vv',
> -            '-p', 'mozlog.pytest_mozlog.plugin',

This will also break the `marionette_harness` unittests, just like bug 1417920. You can try passing it in from the test's `mozunit.main(...)` call (hopefully you'll have better luck than me).
Attachment #8983939 - Flags: review?(ahal) → review+
Comment on attachment 8983940 [details]
Bug 1466211 - Vendor blessings via |mach vendor python|;

https://reviewboard.mozilla.org/r/248990/#review256230

::: Pipfile:16
(Diff revision 1)
>  six = "==1.10.0"
>  attrs = "==18.1.0"
>  pytest = "==3.2.5"
>  jsmin = "==2.1.0"
>  python-hglib = "==2.4"
> +blessings = "==1.6.1"

Can we alphabetize this list? This issue isn't related to this change, so feel free to discard.
Attachment #8983940 - Flags: review?(ahal) → review+
Comment on attachment 8983941 [details]
Bug 1466211 - Add blessings to dependencies for |mach python-test|;

https://reviewboard.mozilla.org/r/248992/#review256232
Attachment #8983941 - Flags: review?(ahal) → review+
Comment on attachment 8983942 [details]
Bug 1466211 - Run mozlint tests using pipenv;

https://reviewboard.mozilla.org/r/248994/#review256234
Attachment #8983942 - Flags: review?(ahal) → review+
Comment on attachment 8983943 [details]
Bug 1466211 - Vendor requests via |mach vendor python|;

https://reviewboard.mozilla.org/r/248996/#review256236
Attachment #8983943 - Flags: review?(ahal) → review+
Comment on attachment 8983939 [details]
Bug 1466211 - Switch all |mach python-test| tests to run using pipenv;

https://reviewboard.mozilla.org/r/248988/#review256238

Also being a bit nitpicky, but this should be the last commit in the series to prevent breaking |mach python-test| on the other intermediate commits.
Comment on attachment 8983944 [details]
Bug 1466211 - Vendor browsermob-proxy via |mach vendor python|;

https://reviewboard.mozilla.org/r/248998/#review256240

::: Pipfile:18
(Diff revision 1)
>  pytest = "==3.2.5"
>  jsmin = "==2.1.0"
>  python-hglib = "==2.4"
>  blessings = "==1.6.1"
>  requests = "==2.9.1"
> +browsermob-proxy = "==0.8.0"

This is already vendored in here:
https://searchfox.org/mozilla-central/source/testing/marionette/harness/marionette_harness/runner/mixins/browsermob-proxy-py/browsermobproxy

How come we need to re-vendor when using it with pipenv? Can we remove it from the old location? If so please do so in the same commit and flag a marionette owner for review instead.
Comment on attachment 8983945 [details]
Bug 1466211 - Run marionette harness tests using pipenv;

https://reviewboard.mozilla.org/r/249000/#review256242
Attachment #8983945 - Flags: review?(ahal) → review+
Comment on attachment 8983946 [details]
Bug 1466211 - Run raptor tests using pipenv;

https://reviewboard.mozilla.org/r/249002/#review256244
Attachment #8983946 - Flags: review?(ahal) → review+
Comment on attachment 8983947 [details]
Bug 1466211 - Vendor json-e via |mach vendor python|;

https://reviewboard.mozilla.org/r/249004/#review256248
Attachment #8983947 - Flags: review?(ahal) → review+
Comment on attachment 8983948 [details]
Bug 1466211 - Vendor voluptuous via |mach vendor python|;

https://reviewboard.mozilla.org/r/249006/#review256250
Attachment #8983948 - Flags: review?(ahal) → review+
Comment on attachment 8983949 [details]
Bug 1466211 - Run taskgraph tests using pipenv;

https://reviewboard.mozilla.org/r/249008/#review256252

::: taskcluster/setup.py:21
(Diff revision 1)
> +        'taskgraph',
> +        'taskgraph.actions',
> +        'taskgraph.cron',
> +        'taskgraph.transforms',
> +        'taskgraph.transforms.job',
> +        'taskgraph.util',

Could we use `find_packages()` here? I think remembering to register new directories could get annoying.
Attachment #8983949 - Flags: review?(ahal) → review+
Comment on attachment 8983950 [details]
Bug 1466211 - Run tryselect tests using pipenv;

https://reviewboard.mozilla.org/r/249010/#review256258

::: tools/tryselect/setup.py:11
(Diff revision 1)
> +
> +from setuptools import setup
> +
> +setup(
> +    author='Mozilla Foundation',
> +    author_email='dev-builds@lists.mozilla.org',

I do think it's a bit of shame that everything will need a setup.py, but I guess the benefits are worth it.

Please use tools@lists.mozilla.org for the email here.

::: tools/tryselect/tryselect/vcs.py:1
(Diff revision 1)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.

Were these files moved with `hg mv`? It's hard to tell in mozreview. But if not, please redo it with that so we can follow blame and handle merge conflicts easier.
Comment on attachment 8983951 [details]
Bug 1466211 - Run mozversioncontrol tests using pipenv;

https://reviewboard.mozilla.org/r/249012/#review256260
Attachment #8983951 - Flags: review?(ahal) → review+
Comment on attachment 8983952 [details]
Bug 1466211 - Run mozterm tests using pipenv;

https://reviewboard.mozilla.org/r/249792/#review256262
Attachment #8983952 - Flags: review?(ahal) → review+
Comment on attachment 8983939 [details]
Bug 1466211 - Switch all |mach python-test| tests to run using pipenv;

https://reviewboard.mozilla.org/r/248988/#review256228

> This will also break the `marionette_harness` unittests, just like bug 1417920. You can try passing it in from the test's `mozunit.main(...)` call (hopefully you'll have better luck than me).

It shouldn't, as using pipenv means mozlog is installed via `setup.py` and therefore pytest-mozlog is registered without the need to do so as a command line option.
Comment on attachment 8983940 [details]
Bug 1466211 - Vendor blessings via |mach vendor python|;

https://reviewboard.mozilla.org/r/248990/#review256230

> Can we alphabetize this list? This issue isn't related to this change, so feel free to discard.

Sure, I'll take care of this in a separate commit.
Comment on attachment 8983939 [details]
Bug 1466211 - Switch all |mach python-test| tests to run using pipenv;

https://reviewboard.mozilla.org/r/248988/#review256238

Good point, I'll reorder the commits to put this on top.
(In reply to Dave Hunt [:davehunt] ⌚️UTC+0 from comment #16)
> It appears that the build failure was caused by pip running from the wrong
> directory. By forcing this to use topsrcdir I now get test failures:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b2b4be4b7a67bb8c705d38f3bd2c167c0bd2e0f2&selectedJob=1
> 82161830
> 
> Some of these failures should be easy to fix by adding packages much like I
> have for other tasks, however it appears that there are several modules that
> don't appear in a package, such as mozwebidlcodegen and
> printprereleasesuffix. Do you have any suggestions for handling these?

Hm, this is tricky, and tbh I'm also not entirely thrilled that things like taskgraph and tryselect need be turned into packages.

Currently, mach gets around this by adding files and packages directly to sys.path here:
https://searchfox.org/mozilla-central/source/build/mach_bootstrap.py#168

The 'search_path' function above that, reads virtualenv_requirements.txt:
https://searchfox.org/mozilla-central/source/build/virtualenv_packages.txt

What if we move (and rename) that 'search_path' function into mozbuild/virtualenv.py. Then after calling 'activate_pipenv', we could call this new function and add everything to sys.path the same way mach_bootstrap.py does. I realize this is kind of hacky, and likely works against the whole point of using pipenv in the first place :/

But it would:
A) work with the edge cases you mention
B) prevent us from needing everything to be a package
C) have these things defined in a single place (rather than the Pipfile and virtualenv_packages.txt)

I don't know if this is a good idea or not, but I don't have any other ideas.
Flags: needinfo?(ahal)
Comment on attachment 8983944 [details]
Bug 1466211 - Vendor browsermob-proxy via |mach vendor python|;

https://reviewboard.mozilla.org/r/248998/#review256240

> This is already vendored in here:
> https://searchfox.org/mozilla-central/source/testing/marionette/harness/marionette_harness/runner/mixins/browsermob-proxy-py/browsermobproxy
> 
> How come we need to re-vendor when using it with pipenv? Can we remove it from the old location? If so please do so in the same commit and flag a marionette owner for review instead.

Ah, I did wonder how it was working without this being vendored. I'd prefer a consistent approach to vendoring, and to include browsermob-proxy in third_party/python, but I could also just add the existing location to the Pipfile. I'll go ahead and see if removing it from the previous location causes any issues.
Or alternatively, can we just forward the sys.path from the current context onto pipenv's virutalenv? I think that would accomplish the same thing (and we could even delete the python/Pipfile)
Comment on attachment 8983949 [details]
Bug 1466211 - Run taskgraph tests using pipenv;

https://reviewboard.mozilla.org/r/249008/#review256252

> Could we use `find_packages()` here? I think remembering to register new directories could get annoying.

I had an issue with `find_packages` because it was including the tests, and pytest complains when it tries to run a test that exists in the package. I will revisit and see if I can use `find_packages` whilst excluding the tests.
Comment on attachment 8983944 [details]
Bug 1466211 - Vendor browsermob-proxy via |mach vendor python|;

https://reviewboard.mozilla.org/r/248998/#review256240

> Ah, I did wonder how it was working without this being vendored. I'd prefer a consistent approach to vendoring, and to include browsermob-proxy in third_party/python, but I could also just add the existing location to the Pipfile. I'll go ahead and see if removing it from the previous location causes any issues.

Actually I think this package is used out of tree a lot, so you probably can't remove it. I'd at least check with :ato before spending too much time on it.
Comment on attachment 8983950 [details]
Bug 1466211 - Run tryselect tests using pipenv;

https://reviewboard.mozilla.org/r/249010/#review256532

I'll r+ for now to get it out of my queue, but sounds like you might have this working such that we don't need to turn tryselect into a package. I'd prefer that approach if it is working well.
Attachment #8983950 - Flags: review?(ahal) → review+
Attachment #8983939 - Attachment is obsolete: true
Attachment #8983940 - Attachment is obsolete: true
Attachment #8983941 - Attachment is obsolete: true
Attachment #8983942 - Attachment is obsolete: true
Attachment #8983944 - Attachment is obsolete: true
Attachment #8983945 - Attachment is obsolete: true
Attachment #8983946 - Attachment is obsolete: true
Attachment #8983949 - Attachment is obsolete: true
Attachment #8983950 - Attachment is obsolete: true
Attachment #8983951 - Attachment is obsolete: true
Attachment #8983952 - Attachment is obsolete: true
Comment on attachment 8984518 [details]
Bug 1466211 - Sort dependencies in Pipfile;

https://reviewboard.mozilla.org/r/250382/#review256626
Attachment #8984518 - Flags: review?(ahal) → review+
Comment on attachment 8984519 [details]
Bug 1466211 - Switch all |mach python-test| tests to run using pipenv;

https://reviewboard.mozilla.org/r/250384/#review256630

Thanks, this looks good! I have a few minor issues/suggestions. Feel free to reflag me on bugzilla if the changes end up being complicated.

::: python/mozbuild/mozbuild/virtualenv.py:559
(Diff revision 1)
> -        subprocess.check_call(
> +            subprocess.check_call(
> -            [pipenv, 'install'] + args,
> +                [pipenv, 'install'] + args,
> -            stderr=subprocess.STDOUT,
> +                stderr=subprocess.STDOUT,
> -            env=env)
> +                env=env)

nit: just build the command/env inside the if statement and call `subprocess.check_call` outside of it.

::: python/mozbuild/mozbuild/virtualenv.py:575
(Diff revision 1)
>          self.virtualenv_root = subprocess.check_output(
>              [pipenv, '--venv'],
>              stderr=subprocess.STDOUT,
>              env=env).rstrip()
>  
> +        if not pipfile:

What if instead of using the existence of the `pipfile` argument, we added a `populate=False` kwarg to `activate_pipenv`?

I could imagine scenarios where you would want to both activate a Pipfile and install the in-tree packages at the same time.

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

Rather than remove the first clause of the if statement, let's change it to:

    if major not in MINIMUM_PYTHON_VERSIONS or ...
    
Will also need to change the error message to include all supported versions.
Attachment #8984519 - Flags: review?(ahal) → review+
Comment on attachment 8984779 [details]
Bug 1466211 - Remove broken call to initial virtualenv when using pipenv;

https://reviewboard.mozilla.org/r/250572/#review256872

::: build/moz.configure/init.configure
(Diff revision 1)
> -        log.info('Reexecuting in the virtualenv')
> -        if env_python:
> -            del os.environ['PYTHON']
> -        # One would prefer to use os.execl, but that's completely borked on
> -        # Windows.
> -        sys.exit(subprocess.call([python] + sys.argv))

This block was being entered because `python` is the pipenv virtual environment, and `sys.executable` is the initial virtual environment created by `mach`. I'm not entirely sure I understand what the reason for this block is, but I think perhaps it's unecessary. Interestingly, this is failing because the `subprocess` that's called doesn't have a `call` method, which suggests that this code path is never executed at present... https://searchfox.org/mozilla-central/rev/e6a0a192ea8691f7eb466506301da73fabe8fd23/python/mozbuild/mozbuild/test/configure/common.py#119-126
Comment on attachment 8984779 [details]
Bug 1466211 - Remove broken call to initial virtualenv when using pipenv;

https://reviewboard.mozilla.org/r/250572/#review256872

> This block was being entered because `python` is the pipenv virtual environment, and `sys.executable` is the initial virtual environment created by `mach`. I'm not entirely sure I understand what the reason for this block is, but I think perhaps it's unecessary. Interestingly, this is failing because the `subprocess` that's called doesn't have a `call` method, which suggests that this code path is never executed at present... https://searchfox.org/mozilla-central/rev/e6a0a192ea8691f7eb466506301da73fabe8fd23/python/mozbuild/mozbuild/test/configure/common.py#119-126

hmm.. it looks like removing this block has caused another issue, though I still don't understand how this code would have worked.
Comment on attachment 8984779 [details]
Bug 1466211 - Remove broken call to initial virtualenv when using pipenv;

https://reviewboard.mozilla.org/r/250572/#review256872

> hmm.. it looks like removing this block has caused another issue, though I still don't understand how this code would have worked.

https://treeherder.mozilla.org/logviewer.html#?job_id=182787366&repo=try&lineNumber=990
Attachment #8984779 - Flags: review?(ahal) → review?(core-build-config-reviews)
Think this one should go onto the build queue.
Comment on attachment 8984779 [details]
Bug 1466211 - Remove broken call to initial virtualenv when using pipenv;

Removing the review request for this patch, as I'm now convinced it's not the correct approach. I'll revisit and push something new when I've worked out what's going on.
Attachment #8984779 - Flags: review?(core-build-config-reviews)
Attachment #8984779 - Attachment is obsolete: true
Comment on attachment 8985936 [details]
Bug 1466211 - Detect if we are running in a virtual environment;

https://reviewboard.mozilla.org/r/251418/#review257932

Lgtm, but I'm not a build peer.
Attachment #8985936 - Flags: review?(ahal)
Attachment #8985936 - Flags: review?(core-build-config-reviews)
Comment on attachment 8985935 [details]
Bug 1466211 - Use --python for selecting target Python when using |mach python-test|;

https://reviewboard.mozilla.org/r/251416/#review257934

Thanks, looks good!

::: python/mozbuild/mozbuild/virtualenv.py:96
(Diff revision 1)
> +    def version_info(self):
> +        return eval(subprocess.check_output([
> +            self.python_path, '-c', 'import sys; print(sys.version_info[:])']))

Maybe we should just print the major version (and rename this property to `major_version` for now just to avoid needing to eval. Though the eval does look like it'll be safe, it's probably fine.
Attachment #8985935 - Flags: review?(ahal) → review+
Comment on attachment 8985936 [details]
Bug 1466211 - Detect if we are running in a virtual environment;

https://reviewboard.mozilla.org/r/251418/#review258670

::: build/moz.configure/init.configure:260
(Diff revision 1)
>          log.info('Creating Python environment')
>          manager.build(python)
>  
>      python = normsep(manager.python_path)
>  
> -    if python != normsep(sys.executable):
> +    if not normsep(sys.executable).startswith(virtualenvs_root):

Would `os.path.dirname(sys.executable) == os.path.join(virtualenvs_root, 'init', 'bin')` be a little more specific and about as effective?
Attachment #8985936 - Flags: review+
Attachment #8985936 - Flags: review?(core-build-config-reviews)
Comment on attachment 8985936 [details]
Bug 1466211 - Detect if we are running in a virtual environment;

https://reviewboard.mozilla.org/r/251418/#review258670

> Would `os.path.dirname(sys.executable) == os.path.join(virtualenvs_root, 'init', 'bin')` be a little more specific and about as effective?

The issue is that `sys.executable` is within a virtualenv, but not `init`. This is because pipenv is creating a new environment, which is populated in the same way as the `init` environment, but can be using a different version of Python. At this point we're satisfied if we're in any virtual environment, and not insisting on the `init` environment being active.
Attachment #8985936 - Flags: review?(ahal)
Pushed by dhunt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1695d4464c54
Vendor requests via |mach vendor python|; r=ahal
https://hg.mozilla.org/integration/autoland/rev/4b197754381a
Vendor json-e via |mach vendor python|; r=ahal
https://hg.mozilla.org/integration/autoland/rev/e04950c7c496
Vendor voluptuous via |mach vendor python|; r=ahal
https://hg.mozilla.org/integration/autoland/rev/fe821d61ecbf
Sort dependencies in Pipfile; r=ahal
https://hg.mozilla.org/integration/autoland/rev/23eb377e2e6d
Switch all |mach python-test| tests to run using pipenv; r=ahal
https://hg.mozilla.org/integration/autoland/rev/dfdf4ea83655
Use --python for selecting target Python when using |mach python-test|; r=ahal
https://hg.mozilla.org/integration/autoland/rev/6680ca0acc27
Detect if we are running in a virtual environment; r=chmanchester
Comment on attachment 8984519 [details]
Bug 1466211 - Switch all |mach python-test| tests to run using pipenv;

:ahal could you take another look at this patch. It was backed out for causing bustage on Windows. This was caused by some path issues and the environment containing unicode. I've fixed these and triggered a full try build via mozreview.
Flags: needinfo?(dave.hunt)
Attachment #8984519 - Flags: review+ → review?(ahal)
Comment on attachment 8984519 [details]
Bug 1466211 - Switch all |mach python-test| tests to run using pipenv;

https://reviewboard.mozilla.org/r/250384/#review259222

Thanks, this looks great!

::: python/mozbuild/mozbuild/virtualenv.py:598
(Diff revision 5)
>      major, minor, micro = sys.version_info[:3]
>  
>      our = LooseVersion('%d.%d.%d' % (major, minor, micro))
>  
> -    if major != MINIMUM_PYTHON_MAJOR or our < MINIMUM_PYTHON_VERSION:
> -        log_handle.write('Python %s or greater (but not Python 3) is '
> +    if major not in MINIMUM_PYTHON_VERSIONS or our < MINIMUM_PYTHON_VERSIONS[major]:
> +        log_handle.write('The following Python versions are required to build:\n')

nit: Let's say "One of the following...", as written it makes it sound like both are required.
Attachment #8984519 - Flags: review?(ahal) → review+
Pushed by dhunt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8a535e12ba48
Vendor requests via |mach vendor python|; r=ahal
https://hg.mozilla.org/integration/autoland/rev/cbdc1c8ef567
Vendor json-e via |mach vendor python|; r=ahal
https://hg.mozilla.org/integration/autoland/rev/980204366869
Vendor voluptuous via |mach vendor python|; r=ahal
https://hg.mozilla.org/integration/autoland/rev/f761d8aed904
Sort dependencies in Pipfile; r=ahal
https://hg.mozilla.org/integration/autoland/rev/8118894d2053
Switch all |mach python-test| tests to run using pipenv; r=ahal
https://hg.mozilla.org/integration/autoland/rev/9b0a35f0572a
Use --python for selecting target Python when using |mach python-test|; r=ahal
https://hg.mozilla.org/integration/autoland/rev/434d589d020d
Detect if we are running in a virtual environment; r=chmanchester
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.