698507 - Use default of `which firefox` for --executablePath

Status: RESOLVED WONTFIX

(Testing :: Talos, defect)

Description

[Jeff Hammel]

Currently Talos will die with a mysterious error if you don't specify
e.g. -e to find the binary.  Instead, use the python-equivalent of
`which firefox` to find the default executable.

Comment 1

[Jeff Hammel]

Attachment: better default value + sanity checking

Comment 2

[William Lachance (:wlach)]

Comment on attachment 570765 [details] [diff] [review]
better default value + sanity checking

Okay this patch looks mostly good to me but there are some minor issues.

 class Usage(Exception):
-    def __init__(self, msg):
-        self.msg = msg
+    """marker class for usage exceptions"""

Why are you removing this class's constructor?

+    # Sanity checking
+    if not options.exePath:
+        raise Usage("please specify --executablePath")
+    if not os.path.exists(options.exePath):
+        raise Usage("--executablePath not found: %s" % options.exePath)
+

1. This should be in PerfConfigurator's verifyCommandLine function, just above.
2. We could be helpful and handle the remote case. A good default is *probably* org.mozilla.fennec_$USER. You'll want to reset the default inside remoteTalosOptions __init__ method.
3. If we're going to use the Usage exception I'd probably use it elsewhere (especially in verifyCommandLine) and also present a meaningful error message on the exception in both PerfConfigurator.py and remotePerfConfigurator.py.

Comment 3

[Jeff Hammel]

(In reply to William Lachance (:wlach) from comment #2)
> Comment on attachment 570765 [details] [diff] [review] [diff] [details] [review]
> better default value + sanity checking
> 
> Okay this patch looks mostly good to me but there are some minor issues.
> 
>  class Usage(Exception):
> -    def __init__(self, msg):
> -        self.msg = msg
> +    """marker class for usage exceptions"""
> 
> Why are you removing this class's constructor?

A couple of reasons:
1. currently Usage (and Configuration) are not used at all.  
2. If you run it without the ctor provided you get:

(tmp)│python PerfConfigurator.py 
Traceback (most recent call last):
  File "PerfConfigurator.py", line 413, in <module>
    sys.exit(main())
  File "PerfConfigurator.py", line 392, in main
    raise Usage("please specify --executablePath")
__main__.Usage

vs. the much more sensible

(tmp)│python PerfConfigurator.py 
Traceback (most recent call last):
  File "PerfConfigurator.py", line 412, in <module>
    sys.exit(main())
  File "PerfConfigurator.py", line 391, in main
    raise Usage("please specify --executablePath")
__main__.Usage: please specify --executablePath

Comment 4

[William Lachance (:wlach)]

(In reply to Jeff Hammel [:jhammel] from comment #3)
> (In reply to William Lachance (:wlach) from comment #2)
> > Comment on attachment 570765 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > better default value + sanity checking
> > 
> > Okay this patch looks mostly good to me but there are some minor issues.
> > 
> >  class Usage(Exception):
> > -    def __init__(self, msg):
> > -        self.msg = msg
> > +    """marker class for usage exceptions"""
> > 
> > Why are you removing this class's constructor?
> 
> A couple of reasons:
> 1. currently Usage (and Configuration) are not used at all.  
> 2. If you run it without the ctor provided you get:

Configuration is definitely used. :) You are correct however that Usage currently isn't. 

If you look at the bottom of PerfConfigurator you'll see some code which attempts to catch exceptions and fail gracefully when they're found. I think we should try to plug your changes into that. If we refactor/remove/change the Configuration and Usage exceptions at the same time, so much the better.

Comment 5

[Jeff Hammel]

Attachment: more cleanup

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 571699 [details] [diff] [review]
more cleanup

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

can we ensure that the code paths for remotePerfConfigurator will exercise these changes properly?

too many questions for a r+

::: talos/PerfConfigurator.py
@@ +22,5 @@
>  import optparse
>  
>  defaultTitle = "qm-pxp01"
>  
> +class PerfConfigurator(object):

does this work in 2.4?

@@ +233,5 @@
>          self.msg = "ERROR: " + msg
>  
> +def findInPath(fileName, path=os.environ['PATH']):
> +    """python equivalent of which; should really be in the stdlib"""
> +    dirs = path.split(os.pathsep)

careful here with os.pathsep, does this work on windows in msys?

@@ +238,5 @@
> +    for dir in dirs:
> +        if os.path.isfile(os.path.join(dir, fileName)):
> +            return os.path.join(dir, fileName)
> +        if os.path.isfile(os.path.join(dir, fileName + ".exe")):
> +            return os.path.join(dir, fileName + ".exe")

what does this return if neither are found?

Comment 7

[Jeff Hammel]

(In reply to Joel Maher (:jmaher) from comment #6)
> Comment on attachment 571699 [details] [diff] [review] [diff] [details] [review]
> more cleanup
> 
> Review of attachment 571699 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> can we ensure that the code paths for remotePerfConfigurator will exercise
> these changes properly?
> 
> too many questions for a r+
> 
> ::: talos/PerfConfigurator.py
> @@ +22,5 @@
> >  import optparse
> >  
> >  defaultTitle = "qm-pxp01"
> >  
> > +class PerfConfigurator(object):
> 
> does this work in 2.4?

Sure does

> @@ +233,5 @@
> >          self.msg = "ERROR: " + msg
> >  
> > +def findInPath(fileName, path=os.environ['PATH']):
> > +    """python equivalent of which; should really be in the stdlib"""
> > +    dirs = path.split(os.pathsep)
> 
> careful here with os.pathsep, does this work on windows in msys?

Not sure, though we do use it here: https://github.com/mozilla/mozbase/blob/master/mozrunner/mozrunner/utils.py#L86 (As a side-kvetch, I really wish python had this `which` function built in.  I end up copy+pasting this all over the place which is highly annoying.  I have proposed including it in os or os.path but there was lots of bikesheddy resistence, big surprise).

> @@ +238,5 @@
> > +    for dir in dirs:
> > +        if os.path.isfile(os.path.join(dir, fileName)):
> > +            return os.path.join(dir, fileName)
> > +        if os.path.isfile(os.path.join(dir, fileName + ".exe")):
> > +            return os.path.join(dir, fileName + ".exe")
> 
> what does this return if neither are found?

None, as I would expect.

Comment 8

[Jeff Hammel]

Attachment: unbitrot and update patch

Please let me know any issues here

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 575027 [details] [diff] [review]
unbitrot and update patch

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

::: talos/PerfConfigurator.py
@@ +304,5 @@
> +    for dir in dirs:
> +        if os.path.isfile(os.path.join(dir, fileName)):
> +            return os.path.join(dir, fileName)
> +        if os.path.isfile(os.path.join(dir, fileName + ".exe")):
> +            return os.path.join(dir, fileName + ".exe")

can we conditionally do this if we are on win32?  likewise is 'firefox' the binary name on osx, I thought it was firefox-bin.

@@ +486,5 @@
> +        # ensure executable exists
> +        if not options.exePath:
> +            raise Configuration("please specify --executablePath")
> +        if not os.path.exists(options.exePath):
> +            raise Configuration("--executablePath not found: %s" % options.exePath)

we call verifyCommandLine from the remote scripts where we specify "-e org.mozilla.fennec"  So this will not work.

Comment 10

[Jeff Hammel]

Even if we don't want the findInPath change, which it sounds like we don't, we should take the other cleanup.  I'll ticket and put up a patch as soon as I can find a way to stage + test

Comment 11

[Jeff Hammel]

see https://bugzilla.mozilla.org/show_bug.cgi?id=704321

Comment 12

[Jeff Hammel]

Cleanup taken as bug 704321 . I'm not going to persist with this for the time being, but the patch is there if anyone wants to take it up

Comment 13

[Jeff Hammel]

Since it seems I'm the only person that cares about this and none of the other harnesses do this (sadly, IMHO) I'm going to close