880839 - [mozrunner] metrotestharness.exe is not getting packaged for dist builds

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

In bug 879386 I have implemented the support for Firefox Metro. After the release of mozrunner 5.17 which includes this support I have noticed, that we do not package the harness tool, so we fail when trying to execute Firefox Metro.

Also given that we have to change that I would like to change the application type to metrofirefox given that this is also the internal name.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Information in how to package additional resources can be found here:
http://docs.python.org/2/distutils/setupscript.html#distutils-installing-package-data

A patch will be ready in a minute.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1

This patch fixes our packaging for the harness launcher and also synchronizes the app option to the real underlying application name.

Comment 3

[Jeff Hammel]

Comment on attachment 759955 [details] [diff] [review]
Patch v1

I'm not at all sure if pkg_resources is guaranteed to be installed. I don't believe it is part of the standard lib.  OTOH, I suppose it is part of setuptools/distribute which we implicitly require, although I worry about any cases where we use non-installed (e.g. .pth, PYTHONPATH, etc) modules

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I'm not sure what you mean by 'pkg_resources is guaranteed to be installed'. Can you please explain? Do you mean if the module is available? If that is your concern please check the current source of mozbase which makes use of this method a couple of times. If we haven't had a problem until know, why should we get one now?

Comment 5

[Jeff Hammel]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> I'm not sure what you mean by 'pkg_resources is guaranteed to be installed'.
> Can you please explain? Do you mean if the module is available? 

pkg_resources is not part of the stdlib: http://docs.python.org/2/library/ (and note also the import output of a stdlib module, here suprocess, vs that of pkg_resources on this computer:
>>> import pkg_resources
>>> pkg_resources.__file__
'/usr/lib/python2.7/dist-packages/pkg_resources.pyc'
>>> import subprocess
>>> subprocess.__file__
'/usr/lib/python2.7/subprocess.pyc'
)

If you have setuptoools or distribute installed, you get pkg_resources along with it, but if you don't it is missing:

│virtualenv.py --no-setuptools tmp
New python executable in tmp/bin/python
│cd tmp/
(tmp)│python
Python 2.7.4 (default, Apr 19 2013, 18:28:01) 
[GCC 4.7.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import pkg_resources
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: No module named pkg_resources

See also http://pythonhosted.org/distribute/pkg_resources.html . If we do require setuptools/distribute, IMHO we should much more clearly note this and also consider this with e.g. how we are running on m-c.

> If that is
> your concern please check the current source of mozbase which makes use of
> this method a couple of times. 

We use this twice in mozbase code itself, in mozrunner.runner and mozrunner.utils (https://github.com/mozilla/mozbase/blob/master/mozrunner/mozrunner/runner.py#L360 and https://github.com/mozilla/mozbase/blob/master/mozrunner/mozrunner/utils.py#L20) which IMHO are redundant so filed bug 881489 for this.  Note that utils does not hard fail on pkg_resource import failure.  The other case, setup_development.py, is for core mozbase developers to run so I don't feel bad having it there.

Again, if we're going to require pkg_resources (setuptools, distutils, etc), I'm not necessarily opposed, but i do think it should be a thought-out decision and not as a side effect of this bug, such as at the mozbase meeting or via the tools mailing list. While we mostly run in virtualenvs in tree and in production these days, not having setuptools and using e.g. .pth files or PYTHONPATH or sys.path tricks, while not my general recommendation, is possible and I would tend to keep open vs. close off that path.

> If we haven't had a problem until know, why should we get one now?

We are running in a variety of environments.  Currently, the utils and CLI code paths are not hit, ABICT, in production; I am fine (given their consolidation and appropriate import guarding) with the way things are there.  However, I would want to fix bug 881489 ASAP (as soon as prioritized).

All this said, if we *do* already require distribute/setuptools (and yes, I mean outside of the setup.py scripts), then I take all of this back.  I can't really support that in all cases since we don't really need them, but I also won't argue.

That all said, I'd be fine with the following general approaches, my favorite to least:

* have a function that either fetches using pkg_resources if available or as relative to __file__ otherwise . Not sure if there are recipes for this.  This can go in mozrunner for now for expediency, though mozfile is, IMHO, a better fit.

* have e.g. MetroFirefoxRunner be entirely surrounded by `try: import pkg_resources ... except ImportError:` guards; in other words, MetroFirefoxRunner requires pkg_resources, but importing mozrunner.runner will not break if pkg_resources unavailable. (Bonus points for declaring extras in setup.py, but pointless other than as an alert to humans since if you can import setuptools you certainly have pkg_resources.)

* Some other solution I can't think of

...and in any case, it'd be nice if we noted one way or other more explicitly in our docs for posterity.

Comment 6

[Jeff Hammel]

Comment on attachment 759955 [details] [diff] [review]
Patch v1

Going to hold off on review until the concerns of comment 5 are addressed.  As is, I'd give an r- for reasons implied therein.

Comment 7

[Jeff Hammel]

Comment on attachment 759955 [details] [diff] [review]
Patch v1

going to r- for concerns in comment 5 . would love to hear what :ahal and :wlach think of this; giving to :wlach for feedback

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Jeff Hammel [:jhammel] from comment #5)
> * have a function that either fetches using pkg_resources if available or as
> relative to __file__ otherwise . Not sure if there are recipes for this. 
> This can go in mozrunner for now for expediency, though mozfile is, IMHO, a
> better fit.

I would prefer this option then. We only have to be sure to keep the harness launcher at the given location, or when it gets moved that appropriate paths will have to be updated. Given that this is urgent I will come up with a patch shortly.

> * have e.g. MetroFirefoxRunner be entirely surrounded by `try: import
> pkg_resources ... except ImportError:` guards; in other words,
> MetroFirefoxRunner requires pkg_resources, but importing mozrunner.runner
> will not break if pkg_resources unavailable. (Bonus points for declaring
> extras in setup.py, but pointless other than as an alert to humans since if
> you can import setuptools you certainly have pkg_resources.)

I wouldn't do this. When we are going to make mochitests dependent on mozbase, this would be a requirement. We already have metro tests written as mochitests.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hm, actually not sure. When I create a virtualenv without setuptools, we fail even running setup.py develop. How are we going to install those packages on mozilla-central?

$ python setup.py develop
Traceback (most recent call last):
  File "setup.py", line 6, in <module>
    from setuptools import setup
ImportError: No module named setuptools

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v2

This patch makes use of __file__ to not depend on the pkg_resource method.

Comment 11

[William Lachance (:wlach)]

(In reply to Jeff Hammel [:jhammel] from comment #7)
> Comment on attachment 759955 [details] [diff] [review]
> Patch v1
> 
> going to r- for concerns in comment 5 . would love to hear what :ahal and
> :wlach think of this; giving to :wlach for feedback

If we can't guarantee the presence of pkg_resources, __file__ sounds like the way to go. I'd probably just use that method all the time, as :whimboo does in his patch. Trying to support both methods seems to be complication for no gain, unless I'm missing something.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 761729 [details] [diff] [review]
Patch v2

Given that Jeff is not around, Clint suggested to flag you both for a review. So whoever comes first, I would appreciate a review. Thanks.

Comment 13

[Andrew Halberstadt [:ahal]]

Comment on attachment 761729 [details] [diff] [review]
Patch v2

Probably should flag both of us, otherwise we may do it at the same time. I'll do the review

Comment 14

[Andrew Halberstadt [:ahal]]

Comment on attachment 761729 [details] [diff] [review]
Patch v2

Review of attachment 761729 [details] [diff] [review]:
-----------------------------------------------------------------

Is this the wrong patch? If not it's still adding "import package_resources"

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v2.1

Well, only missed to remove that line. Everything else was adapted to what Jeff wanted to have.

Comment 16

[Andrew Halberstadt [:ahal]]

Comment on attachment 762207 [details] [diff] [review]
Patch v2.1

Review of attachment 762207 [details] [diff] [review]:
-----------------------------------------------------------------

Ah cool, I didn't see the other patch. Lgtm!

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed on master:
https://github.com/mozilla/mozbase/commit/1b9a730d48e5e1bc9d78ecc8c2f1b1b8616e1fc2