Closed
Bug 703646
Opened 12 years ago
Closed 11 years ago
Decide what to do about MozRunner executable finding
Categories
(Testing :: Mozbase, defect)
Testing
Mozbase
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: k0scist, Unassigned)
References
Details
Attachments
(1 file)
6.57 KB,
patch
|
ahal
:
review+
|
Details | Diff | Splinter Review |
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
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.
Reporter | ||
Comment 2•12 years ago
|
||
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•12 years ago
|
||
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.
(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.
Reporter | ||
Comment 5•12 years ago
|
||
Attachment #575936 -
Flags: review?(ahalberstadt)
Comment 6•12 years ago
|
||
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.
Attachment #575936 -
Flags: review?(ahalberstadt) → review-
Comment 7•12 years ago
|
||
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•12 years ago
|
||
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.
Reporter | ||
Comment 9•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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.
Attachment #575936 -
Flags: review- → review+
Comment 12•12 years ago
|
||
What about using a command line switch to turn on this feature and make it off by default?
Reporter | ||
Comment 13•12 years ago
|
||
(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
Reporter | ||
Comment 14•12 years ago
|
||
So per IRC conversation, we are going to land this patch as soon as testing is done and bump a version.
Reporter | ||
Comment 15•11 years ago
|
||
Going to version 5 since this is a pretty major change
Reporter | ||
Comment 16•11 years ago
|
||
pushed to master: https://github.com/mozilla/mozbase/commit/79ff9a49fd00522544b2b6e3b1e04964335a411c will file some follow-up issues
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 18•11 years ago
|
||
bumped master mozmill version to ensure compatability with trunk; tested with mutt and works: https://github.com/mozautomation/mozmill/commit/03d5f08769cd406b010c8a9e35657e49bfb043e6
Reporter | ||
Comment 19•11 years ago
|
||
Filed a follow-up bug to address the issue in comment 6 : bug 711520
Comment 20•11 years ago
|
||
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•11 years ago
|
||
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.
Reporter | ||
Comment 22•11 years ago
|
||
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.)
You need to log in
before you can comment on or make changes to this bug.
Description
•