703646 - Decide what to do about MozRunner executable finding

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Jeff Hammel]

Mozrunner has a really long chain of what it does to find the binary:

https://github.com/mozilla/mozbase/blob/master/mozrunner/mozrunner/runner.py

This is good, in the sense that it provides convenience, but it is bad
in the sense of reproducibility and predictability.

I think we should:

1. Document what it does now (verbosely)
2. Decide what we really want it to do (verbosely)
3. Do it (probably the easiest step)

See also bug 649042

Comment 1

[cmtalbert]

Require full paths.  End of story.  This code is horrible, prone to failure in strange and exciting and terrible ways, and has been a thorn in the side of more people than it has helped.  It never worked in the beginning, it was made somewhat usable by mozmill 1.5.x but at this point, it still causes us issues, and I'm for just ripping it all out.

Additionally, it's what all the other test frameworks do.

Yes, that means on mac you require the foo.app/Contents/MacOS/foo-bin.  I could be persuaded to accept the full path to the foo.app on mac os because I know that many new users don't realize there is a Contents/MacOS underneath the .app (you can't see this in the mac file picker).  So that's the only concession I'd make.  But all the code that looks in all the "usual" spots for the binary should be axed and deleted for all OS's.

So, my proposal:
* Windows: require full path to .exe
* Linux: require full path to the executable file
* Mac OS: require full path to either the .app or to .app/Contents/MacOS/<executablefile>
* Remove all code that attempts to look up path to executables.

I cc'd Henrik because I think he will care about this issue.

Comment 2

[Jeff Hammel]

I've said it before and i'll say it again, I would greatly prefer defaulting to `which firefox`. I don't really care about the windows registry or the mac contents folder. If we're going to throw away this entire functionality, we should just throw away without exceptions.

Comment 3

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

As Clint pointed out the one and only feature we should keep is offering the user on OS X to specify the .app and not only the binary itself. Not sure if we hard-code the binary itself (i can't check the current code right now), but if we do we should probably check the file 'Contents/Info.plist' of the app bundle for the right binary file. That way we can be 100% sure to call the correct binary.

For all of our scripts we do not rely on any registry setting. We always specify the path to Firefox via the -b option. So I don't see a problem in removing all that code. Especially when I see how old and outdated it is. There is no mentioning of Nightly or Aurora even. So yeah, lets kill it.

Comment 4

[cmtalbert]

(In reply to Jeff Hammel [:jhammel] from comment #2)
> I've said it before and i'll say it again, I would greatly prefer defaulting
> to `which firefox`. I don't really care about the windows registry or the
> mac contents folder. If we're going to throw away this entire functionality,
> we should just throw away without exceptions.

The which code is fine, but I think it's going to work well in some platforms (linux) where firefox is in your path.  This generally won't be the case in mac or windows because no one on those platforms adds firefox to their path.  So I think we should just axe the code across the board, make it work the same on every platform, no exceptions.

Comment 5

[Jeff Hammel]

Attachment: axe everything

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 575936 [details] [diff] [review]
axe everything

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

::: mozrunner/mozrunner/runner.py
@@ -127,5 @@
> -            if binary is None:
> -                raise BinaryLocationException("Your binary could not be located; you will need to set it")
> -            return binary
> -        elif mozinfo.isMac and binary.find('Contents/MacOS/') == -1:
> -            return os.path.join(binary, 'Contents/MacOS/%s-bin' % cls.names[0])

I thought we wanted to keep this functionality as per Clint and Henrik's comment.

Comment 7

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

Also see bug 703607 for further information regarding the behavior on OS X. We should not remove the feature to also being able to specify the .app bundle.

Comment 8

[Andrew Halberstadt [:ahal]]

Hmm I didn't know that bug 703607 existed. I'm fine r+'ing this patch if we re-implement the .app functionality with Henrik's plist suggestion in the other bug.

Comment 9

[Jeff Hammel]

If we're going to special case mac someone who has mac internal knowledge and uses macs for development should probably do and test the patch

Comment 10

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

Well, I would separate the finding of the binary and the plist extraction into two separate issues. That's why we still have bug 703607 separated.

Comment 11

[Andrew Halberstadt [:ahal]]

Comment on attachment 575936 [details] [diff] [review]
axe everything

Changing to r+. Using plist doesn't require some magic "cls.names" so it seems this patch can go ahead and land and then we can start work on the other bug once that has happened.

Comment 12

[Burak Yiğit Kaya [:BYK]]

What about using a command line switch to turn on this feature and make it off by default?

Comment 13

[Jeff Hammel]

(In reply to Burak Yiğit Kaya from comment #12)
> What about using a command line switch to turn on this feature and make it
> off by default?

I think the main issue is code complexity and maintainability.  While for the `which firefox` case that's pretty bare bones, I think the idea is that we want an explicit path to the binary, either specified via command line or via an environment variable

Comment 14

[Jeff Hammel]

So per IRC conversation, we are going to land this patch as soon as testing is done and bump a version.

Comment 15

[Jeff Hammel]

Going to version 5 since this is a pretty major change

Comment 16

[Jeff Hammel]

pushed to master: https://github.com/mozilla/mozbase/commit/79ff9a49fd00522544b2b6e3b1e04964335a411c

will file some follow-up issues

Comment 18

[Jeff Hammel]

bumped master mozmill version to ensure compatability with trunk; tested with mutt and works: https://github.com/mozautomation/mozmill/commit/03d5f08769cd406b010c8a9e35657e49bfb043e6

Comment 19

[Jeff Hammel]

Filed a follow-up bug to address the issue in comment 6 : bug 711520

Comment 20

[Myk Melez [:myk] [@mykmelez]]

FYI- this means Jetpack will have to fork mozrunner, since binary detection is a must-have feature for us to give addon developers the best possible user experience.

Perhaps that's not such a bad thing--we aren't using much of mozrunner anyway, and we haven't updated our downstream copy for a while--but I thought y'all should know.

Comment 21

[Andrew Halberstadt [:ahal]]

Rather than forking it might be better to extend mozrunner and simply add that function back to your subclass. That way it'll still be easy to upstream changes.

Comment 22

[Jeff Hammel]

I can't really speak to the best recommendation for Jetpack testing, but I would guess that just forking the part of binary finding that makes sense and feeding that result into mozrunner would make the most sense.  I can help with this if you want to file a bug and CC.

(I would also recommend only using the code that you actually need, as there were some pretty crufty bits in there.)