697462 - Add VirtualenvMixin setup.py support to mozharness

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect, P3)

Description

[Andrew Halberstadt [:ahal]]

Currently VirtualenvMixin only lets you install packages via "pip". This assumes that the packages on pip are up to date and that we have internet access. There should also be support for installing a package via "python setup.py install". This way scripts can do things like pull a repo to make sure that they are running the exact version of the package that they need.

Comment 1

[Aki Sasaki (not active)]

Agreed, though as shown by https://github.com/escapewindow/mozharness/blob/templatescripts/examples/venv.py we can point at any tarball anywhere with a setup.py, and install via current VirtualenvMixin.

Maybe we should be able to point VirtualenvMixin at a directory and be able to run setup.py for when we clone from source?

Comment 2

[Jeff Hammel]

It is also worth noting that for mozbase, since there are several repositories in subdirectories in a github repo, we cannot install "HEAD" from there since github doesn't generate tarballs for these subdirectories.  There are several ways around this probably not worth discussing here, but this would provide one way forward: clone the repo and point at a subdirectory

Comment 3

[Aki Sasaki (not active)]

Attachment: wip

I think this will work.
I was debating whether to require virtualenv_modules to be a dict, or allow for lists and only require a dict if you want to use setup.py.

I'm going to write some tests and port peptest to use virtualenv_modules for its mozbase packages, but thought I'd pass my current patch to you for feedback.

Comment 4

[Jeff Hammel]

Comment on attachment 612727 [details] [diff] [review]
wip

Why would you want more than one pypi url specified per package?

Comment 5

[Aki Sasaki (not active)]

(In reply to Jeff Hammel [:jhammel] from comment #4)
> Comment on attachment 612727 [details] [diff] [review]
> wip
> 
> Why would you want more than one pypi url specified per package?

Hm, I'm going to guess you're referring to

> +                self._pip_install_module(self.config.get('%s_url' % module, module_config.get('pypi_url', module)))

For instance, for talos, this would look for self.config['talos_url'], then fall back to self.config['virtualenv_modules']['talos']['pypi_url'], then fall back to 'talos'.

We can get rid of the 'talos_url' check, but this has a couple advantages:

1) it's easier to override --talos-url URL via the commandline than it is to generate the entire dict that way.
2) also easier to override just the url in a config file.
3) it's backwards compatible, though that's not a huge concern right now since we have a limited set of scripts to update to use the new syntax.

So not a huge deal, I can get rid of it.  I can think of a usage case where we'd want to keep the exact same settings but override which talos we install (to verify a new package f/e), but I might be complicating things a bit too much without enough justification.

Comment 6

[Aki Sasaki (not active)]

Attachment: current wip

This patch:
* adds --download-cache support to pip install, since we may be downloading a ton of python packages from a ton of testers 24/7.  We'll need a bit of testing if we want to turn this on across the board, especially around .
* changes virtualenv_modules from a list or dict to a list of strings and/or dicts.  Preserving order was important in case some packages depend on others.

Comment 7

[Aki Sasaki (not active)]

(In reply to Aki Sasaki [:aki] from comment #6)
> We'll need a bit of testing if we want to turn this on across the board,
> especially around .

especially around updating packages that are already in the download cache, but are pointing to a new url/version.

Comment 8

[Aki Sasaki (not active)]

(In reply to Aki Sasaki [:aki] from comment #5)
> > +                self._pip_install_module(self.config.get('%s_url' % module, module_config.get('pypi_url', module)))
> 
> For instance, for talos, this would look for self.config['talos_url'], then
> fall back to self.config['virtualenv_modules']['talos']['pypi_url'], then
> fall back to 'talos'.
> 
> We can get rid of the 'talos_url' check, but this has a couple advantages:

For instance, in attachment 613015 [details] [diff] [review], the virtualenv_modules looks like:

+    virtualenv_modules = [
+        'simplejson',
+        {'module': 'mozlog', 'setup_path': 'tests/mozbase/mozlog'},
+        {'module': 'mozinfo', 'setup_path': 'tests/mozbase/mozinfo'},
...

A person running from the commandline with no simplejson_url set will download simplejson from the [default] pypi server.  However, in the production config files, we can set simplejson_url to a specific url without redefining everything else in virtualenv_modules at the same time.

Comment 9

[Jeff Hammel]

I like the new form of the patch better, though I'm not sure if this is accurate any longer:

+        if module_config.get('setup_path'):
+            if 'pypi_url' in module_config:
+                self.fatal("%s has both setup_path and pypi_url specified in virtualenv_modules!\nPlease specify one, at most." % module)

or the comment here:

+            virtualenv_modules = ['module1', {
+                'module': 'module2',
+                'pypi_url': 'http://url/to/package'
+            },{

since with the current patch --pypi-url only comes from self.config


A couple more things to think about:

* pip can install from a directory, so doing
{'module': 'mozlog', 'setup_path': 'tests/mozbase/mozlog'}
and
'tests/base/mozlog' should do the same thing (ignoring the --mozlog-url part)

* --pypi-url *could* be applicable to installing from a path if the setup.py of that path has dependencies.  If we used pip for this instead of `python setup.py install`, ABICT this does work

I'm a little confused by a few other things here.

+            self._pip_install_module(self.config.get('%s_url' % module, module_config.get('pypi_url', module)))

ABICT These aren't equivalent.  

Ignoring the overrides available (i.e. --mozlog-url) from the command line, it would seem that you could get away with
* an object-global --pypi-url for specifiying which pypi to use for the package and dependencies
* a single string describing the package, either a name ('mozlog'), a url ('http://example.com/mozlog.tgz'), or a path ('/foo/mozlog'), as pip handles all of these cases

Comment 10

[Aki Sasaki (not active)]

(In reply to Jeff Hammel [:jhammel] from comment #9)
> I like the new form of the patch better, though I'm not sure if this is
> accurate any longer:
>
> +        if module_config.get('setup_path'):
> +            if 'pypi_url' in module_config:
> +                self.fatal("%s has both setup_path and pypi_url specified
> in virtualenv_modules!\nPlease specify one, at most." % module)
>
> or the comment here:
>
> +            virtualenv_modules = ['module1', {
> +                'module': 'module2',
> +                'pypi_url': 'http://url/to/package'
> +            },{
>
> since with the current patch --pypi-url only comes from self.config

This refers to the pypi_url *for a specific module*, not the pypi url passed into pip install for all modules.

Therefore it's accurate, though possibly confusing.

> A couple more things to think about:
>
> * pip can install from a directory, so doing
> {'module': 'mozlog', 'setup_path': 'tests/mozbase/mozlog'}
> and
> 'tests/base/mozlog' should do the same thing (ignoring the --mozlog-url part)
>
> * --pypi-url *could* be applicable to installing from a path if the setup.py
> of that path has dependencies.  If we used pip for this instead of `python
> setup.py install`, ABICT this does work

Ok.  I can see how that would be advantageous to reduce the amount of code here (everything must be installed via pip).  Are there any cases where pip might not be available or usable for a setup.py?  If so, we have this code to call setup.py directly and might as well support it.

> I'm a little confused by a few other things here.
>
> +            self._pip_install_module(self.config.get('%s_url' % module,
> module_config.get('pypi_url', module)))
>
> ABICT These aren't equivalent.

They're not supposed to be equivalent.
It's a search path.

1. if MODULE_url is in config, use that
2. if pypi_url is in the virtualenv_modules dict, use that
3. use module otherwise.

> Ignoring the overrides available (i.e. --mozlog-url) from the command line,
> it would seem that you could get away with
> * an object-global --pypi-url for specifiying which pypi to use for the
> package and dependencies
> * a single string describing the package, either a name ('mozlog'), a url
> ('http://example.com/mozlog.tgz'), or a path ('/foo/mozlog'), as pip handles
> all of these cases

If you specify http://example.com/mozlog.tgz, you can't easily specify a different url for that module for production than for users, because it's hardcoded.  That's the main reason for having this separation.

Comment 11

[Jeff Hammel]

ah, i see.  maybe just 'url' vs 'pypi_url' would be less confusing?  e.g. 
{'module': 'talos', 'url': 'http://hg.mozilla.org/build/talos/archive/tip.tar.gz'} # not actually on pypi

I don't know of any case where `pip install PATH` wouldn't work but that's not to say that there aren't any. ianbicking would be the person to ask

Comment 12

[Aki Sasaki (not active)]

(In reply to Jeff Hammel [:jhammel] from comment #11)
> ah, i see.  maybe just 'url' vs 'pypi_url' would be less confusing?  e.g. 
> {'module': 'talos', 'url':
> 'http://hg.mozilla.org/build/talos/archive/tip.tar.gz'} # not actually on
> pypi

Sure, updated my patch to do so.  pypi_url now only refers to the value passed to pip install --pypi-url.

> I don't know of any case where `pip install PATH` wouldn't work but that's
> not to say that there aren't any. ianbicking would be the person to ask

Ok.  If we go that route, I can change

{module: MODULE, setup_path/url: PATH/URL} to {MODULE: PATH/URL} pretty easily.
Right now the only things I can think of are:

1. pip doesn't work for some reason
2. a package has named its setup.py something other than setup.py.

Not entirely sure if this is worth bothering to support.  Easy enough to tear out if wanted.

Comment 13

[Jeff Hammel]

(In reply to Aki Sasaki [:aki] from comment #12)
> (In reply to Jeff Hammel [:jhammel] from comment #11)
> > ah, i see.  maybe just 'url' vs 'pypi_url' would be less confusing?  e.g. 
> > {'module': 'talos', 'url':
> > 'http://hg.mozilla.org/build/talos/archive/tip.tar.gz'} # not actually on
> > pypi
> 
> Sure, updated my patch to do so.  pypi_url now only refers to the value
> passed to pip install --pypi-url.

Coolz.

> > I don't know of any case where `pip install PATH` wouldn't work but that's
> > not to say that there aren't any. ianbicking would be the person to ask
> 
> Ok.  If we go that route, I can change
> 
> {module: MODULE, setup_path/url: PATH/URL} to {MODULE: PATH/URL} pretty
> easily.
> Right now the only things I can think of are:
> 
> 1. pip doesn't work for some reason
> 2. a package has named its setup.py something other than setup.py.
> 
> Not entirely sure if this is worth bothering to support.  Easy enough to
> tear out if wanted.

I talked to Ian, and indeed if `pip install PATH` doesn't work it is a bug (and he was unaware of any cases where it wouldn't).  

If the package has named its setup.py script something other than setup.py, then many tools won't work ;)  Still, it could have an alternate installation method so its possible (though I can't think of any place I've seen this in the wild).

Comment 14

[Aki Sasaki (not active)]

Attachment: install all python packages with pip

This simplifies the code paths quite a bit.

I also renamed VirtualenvMixin.query_package() to VirtualenvMixin.query_python_package() to head off potential naming collisions.  We're actually not using this anymore once I remove the get-latest-tinderbox functionality in bug 720436, but assuming we want to keep that functionality, renaming is preferable.

I've tested this along with a modified bug 720436 patch and it works.

Comment 15

[Jeff Hammel]

Comment on attachment 613379 [details] [diff] [review]
install all python packages with pip

Looks a lot cleaner! :)

Comment 16

[Aki Sasaki (not active)]

Attachment: Same, with expanded install_module docstring

I think this is ready for review.

Comment 17

[Jeff Hammel]

Comment on attachment 613395 [details] [diff] [review]
Same, with expanded install_module docstring

         packages = output.split()
         return [package for package in packages
                 if package.lower().find(package_name.lower()) != -1]

Not part of this patch but this could give false positives

+            if isinstance(module, dict):
+                (module, module_url) = module.items()[0]

And if the dict is more than one item long?
Not really sure what to do here except checking the length.  You could also use a 2-tuple, though this doesn't get around the need to check the length.

Other than that, looks good!  The name change may break e.g. mozharness.mozilla.talos and perhaps more but we can figure that out going forward.

Comment 18

[Aki Sasaki (not active)]

(In reply to Jeff Hammel [:jhammel] from comment #17)
> Comment on attachment 613395 [details] [diff] [review]
> Same, with expanded install_module docstring
> 
>          packages = output.split()
>          return [package for package in packages
>                  if package.lower().find(package_name.lower()) != -1]
> 
> Not part of this patch but this could give false positives

Hm.  Well, it'll be unused soon, so if it's not good we can yank it.
Sounds like we need a MEGAREGEX! ;-)

> +            if isinstance(module, dict):
> +                (module, module_url) = module.items()[0]
> 
> And if the dict is more than one item long?
> Not really sure what to do here except checking the length.  You could also
> use a 2-tuple, though this doesn't get around the need to check the length.

=\

How about

if isinstance(module, dict):
    if len(module) != 1:
        self.fatal("Bad virtualenv_modules definition %s!" % str(module))
    (module, module_url) = module.items()[0]

> Other than that, looks good!  The name change may break e.g.
> mozharness.mozilla.talos and perhaps more but we can figure that out going
> forward.

Nothing currently checked in uses query_package that I haven't changed:

(bb08)deathduck:~/src/firefox-fetch/mozharness [15:02:33] (default)
670$ grep -r query_package mozharness scripts configs
(bb08)deathduck:~/src/firefox-fetch/mozharness [15:02:35] (default)
671$ 

I can't currently check this in til I fix the peptest stuff in bug 720436.
Do you think I should r? that and then move it to mozharness.mozilla.* or wait til it's all moved?

Comment 19

[Jeff Hammel]

(In reply to Aki Sasaki [:aki] from comment #18)
> (In reply to Jeff Hammel [:jhammel] from comment #17)
> > Comment on attachment 613395 [details] [diff] [review]
> > Same, with expanded install_module docstring
> > 
> >          packages = output.split()
> >          return [package for package in packages
> >                  if package.lower().find(package_name.lower()) != -1]
> > 
> > Not part of this patch but this could give false positives
> 
> Hm.  Well, it'll be unused soon, so if it's not good we can yank it.
> Sounds like we need a MEGAREGEX! ;-)

Again, somewhat OT for this bug, but casual inspection seems to indicate that if you
* ignore lines starting with -e (maybe a few other flags, maybe 'if not line.startswith('-')'
* split on '==', '<=', '>=' and take the first part you should be able to be pretty robust.
But there might be some cases I don't know about.  In any case, it would be better than what we have.

> > +            if isinstance(module, dict):
> > +                (module, module_url) = module.items()[0]
> > 
> > And if the dict is more than one item long?
> > Not really sure what to do here except checking the length.  You could also
> > use a 2-tuple, though this doesn't get around the need to check the length.
> 
> =\
> 
> How about
> 
> if isinstance(module, dict):
>     if len(module) != 1:
>         self.fatal("Bad virtualenv_modules definition %s!" % str(module))
>     (module, module_url) = module.items()[0]

Works for me.

> > Other than that, looks good!  The name change may break e.g.
> > mozharness.mozilla.talos and perhaps more but we can figure that out going
> > forward.
> 
> Nothing currently checked in uses query_package that I haven't changed:
> 
> (bb08)deathduck:~/src/firefox-fetch/mozharness [15:02:33] (default)
> 670$ grep -r query_package mozharness scripts configs
> (bb08)deathduck:~/src/firefox-fetch/mozharness [15:02:35] (default)
> 671$ 

Coolz.
 
> I can't currently check this in til I fix the peptest stuff in bug 720436.
> Do you think I should r? that and then move it to mozharness.mozilla.* or
> wait til it's all moved?

I don't know if I have a strong preference.  This isn't blocking me right now, though it will eventually.  So, in the immediate term, whichever works better for you.

Comment 20

[Aki Sasaki (not active)]

Comment on attachment 613395 [details] [diff] [review]
Same, with expanded install_module docstring

http://hg.mozilla.org/build/mozharness/rev/d8d1cc4f1126

Filed bug 743856 to either fix or remove query_python_package().

Comment 21

[Aki Sasaki (not active)]

Comment on attachment 613395 [details] [diff] [review]
Same, with expanded install_module docstring

Backed out due to windows bustage... need to re-support setup.py.

Comment 22

[Aki Sasaki (not active)]

For posterity:

    16:26:45     INFO - Installing mozlog into virtualenv c:/talos-slave/test/build/venv
    16:26:45     INFO - Running command: ['c:\\talos-slave\\test\\build\\venv\\Scripts\\pip', 'install', 'tests/mozbase/mozlog'] in c:\talos-slave\test\build
    16:26:45     INFO -  Exception:
    16:26:45    ERROR -  Traceback (most recent call last):
    16:26:45     INFO -    File "c:\talos-slave\test\build\venv\lib\site-packages\pip-1.0.2-py2.5.egg\pip\basecommand.py", line 126, in main
    16:26:45     INFO -      self.run(options, args)
    16:26:45     INFO -    File "c:\talos-slave\test\build\venv\lib\site-packages\pip-1.0.2-py2.5.egg\pip\commands\install.py", line 195, in run
    16:26:45     INFO -      InstallRequirement.from_line(name, None))
    16:26:45     INFO -    File "c:\talos-slave\test\build\venv\lib\site-packages\pip-1.0.2-py2.5.egg\pip\req.py", line 104, in from_line
    16:26:45     INFO -      return cls(req, comes_from, url=url)
    16:26:45     INFO -    File "c:\talos-slave\test\build\venv\lib\site-packages\pip-1.0.2-py2.5.egg\pip\req.py", line 38, in __init__
    16:26:45     INFO -      req = pkg_resources.Requirement.parse(req)
    16:26:45     INFO -    File "c:\talos-slave\test\build\venv\lib\site-packages\distribute-0.6.14-py2.5.egg\pkg_resources.py", line 2547, in parse
    16:26:45     INFO -      reqs = list(parse_requirements(s))
    16:26:45     INFO -    File "c:\talos-slave\test\build\venv\lib\site-packages\distribute-0.6.14-py2.5.egg\pkg_resources.py", line 2472, in parse_requirements
    16:26:45     INFO -      line, p, specs = scan_list(VERSION,LINE_END,line,p,(1,2),"version spec")
    16:26:45     INFO -    File "c:\talos-slave\test\build\venv\lib\site-packages\distribute-0.6.14-py2.5.egg\pkg_resources.py", line 2440, in scan_list
    16:26:45     INFO -      raise ValueError("Expected "+item_name+" in",line,"at",line[p:])
    16:26:45     INFO -  ValueError: ('Expected version spec in', 'tests/mozbase/mozlog', 'at', '/mozbase/mozlog')
    16:26:45     INFO -  Storing complete log in C:\Users\cltbld\AppData\Roaming\pip\pip.log
    16:26:45    ERROR - Return code: 2
    16:26:45    FATAL - Halting on failure while running ['c:\\talos-slave\\test\\build\\venv\\Scripts\\pip', 'install', 'tests/mozbase/mozlog']
    16:26:45    FATAL - Exiting 2

Comment 23

[Aki Sasaki (not active)]

https://bugzilla.mozilla.org/attachment.cgi?id=613013 and the corresponding patch from bug 720436 work on both linux and win32.

I'm going to clean those up tomorrow and ask for review.

Comment 24

[Jeff Hammel]

We should probably 

A. Figure out if this is a pip bug or not; talking to :ianb, if this doesn't work its probably a bug unless we're doing something wrong.
B. Ticket said bug if it exists
C. Fix said bug (again, if it exists)

Comment 25

[Aki Sasaki (not active)]

I'd be happy for that to happen, but I've already spent a decent amount of time here and haven't actually started working on what I need to for Q2, which is revamping how we run talos on devices.

As we have a working solution, I see comment 24 as non-urgent.

Comment 26

[Carl Meyer (:carljm)]

My first guess (not having tried to reproduce) would be that pip on Windows isn't recognizing "tests/mozbase/mozlog" as a path, because it's looking for "os.path.sep" which on Windows is backslash. Thus it's trying to treat it as a requirement in the "FooPackage==1.0" style, and failing.

If my hypothesis is correct, the simplest workaround would be for whatever script is constructing this command to build that path using os.path.join rather than hardcoding in the slashes.

Comment 27

[Aki Sasaki (not active)]

(In reply to Carl Meyer from comment #26)
> My first guess (not having tried to reproduce) would be that pip on Windows
> isn't recognizing "tests/mozbase/mozlog" as a path, because it's looking for
> "os.path.sep" which on Windows is backslash. Thus it's trying to treat it as
> a requirement in the "FooPackage==1.0" style, and failing.
> 
> If my hypothesis is correct, the simplest workaround would be for whatever
> script is constructing this command to build that path using os.path.join
> rather than hardcoding in the slashes.

Your hypothesis appears to be correct, thanks!  New patch(es?) coming for jhammel to r?

Comment 28

[Aki Sasaki (not active)]

Attachment: same, but with os.path.join in docstring

Interdiff:

(bb08)deathduck:~/src/firefox-fetch/mozharness [13:52:58] (default)
763$ diff ~/Desktop/setuppy{4,5}.diff
2c2
< # Parent e1d89b6b19018771ec30776386ea55b8c4ebefe5
---
> # Parent a83b93f3dff1bf46e7c8b07ab1a7fb0bc6fa9cc0
87c87
< +                {'module3': '/path/to/setup_py/dir/'},
---
> +                {'module3': os.path.join('path', 'to', 'setup_py', 'dir')},

Comment 29

[Jeff Hammel]

Comment on attachment 613752 [details] [diff] [review]
same, but with os.path.join in docstring

wfm

Comment 30

[Aki Sasaki (not active)]

Comment on attachment 613752 [details] [diff] [review]
same, but with os.path.join in docstring

http://hg.mozilla.org/build/mozharness/rev/d5f54551be49

Watching the results of a peptest run before closing.

Comment 31

[Aki Sasaki (not active)]

Green XP run.  -> RESO FIXED