Remove assumption that xpcshell etc in same directory as app executable

RESOLVED FIXED in mozilla1.9.2a1

Status

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: cmtalbert, Assigned: ted)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.2a1
fixed1.9.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

10 years ago
Mochitests currently make the assumption that xpcshell and other utilities are in the same directory as your application executable.  However on XULRunner projects like Fennec, you often get this directory structure:
<objdir>/dist/bin/:
application.ini
<executable>
components/
chrome/
defaults/
xulrunner/

And inside that xulrunner/ directory is all the normal goodies that Mochitest needs like xpcshell, certutil etc.

While this is pretty standard, I don't think it's set in stone either. So, I advocate for another parameter on runtests.py an app-path (thanks Gavin) argument so that mochitest can look there for other necessary applications and libraries.

The value of app-path should default this way:
* If no parameters set: it should default to be the standard path of the standard firefox installation (current behavior of the tool)
* If a specific parameter is called out using --app and no --app-path specified, then it should default to the containing directory of the executable in --app. (current behavior of the tool)
* Otherwise, app-path is specified, and that value is used instead of the current defaults for this value.
We'll need this if we're ever going to run tests on a packaged build as well.
I looked into this yesterday and overriding the bin-path and certutil path is straightforward. The main problem I found was changing the server base path:

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/server.js#131

"/" on the server is mapped to "_tests/testing/mochitest" appended to the install directory. Of course if you move the _tests directory out of the build, this path is bogus. I found no way to get the current working directory instead. If someone could figure this out I think we could get this working.
We could add a --test-path parameter to runtests.py, and pass that test path to xpcshell/server.js via an environment variable. If the env var is unset it can default to what it currently does.
Created attachment 350088 [details] [diff] [review]
patch with flags to override path to binaries and certs

This patch adds three flags to runtests.py. After taking the mochitest directory out of the build, these flags must be specified:

--appname=<absolute path to application executable>
--test-dir=<absolute path to mochitest directory>
--bin-path=<path to binaries>
--certs-path=<path to certificates>

Looking for people to try this on other platforms (I tested it on Ubuntu, both in the build and out of the build). Also, looking for input on the flag names and descriptions. Finally, the test-dir flag is just specifying the directory the script is running from, maybe this should just be a boolean flag instead of making the user type in the absolute path. This is a lot of arguments, maybe it could be bundled in some way.
I think I reinvented your patch while working on bug 421611. I'll take a look at it and see if I can merge the two. As it turns out, I can't keep xpcshell in a different directory for testing packaged builds, as it looks in the same directory as the executable for the components/ dir. RE: comment #2, I'm testing a patch in bug 470914 that adds a __LOCATION__ variable to the global object for files loaded with "xpcshell -f file" (like we currently do for JS components). This should enable server.js to find its current directory (and thus the test path) without any trickery.
(Assignee)

Updated

10 years ago
Blocks: 421611
Created attachment 354308 [details] [diff] [review]
my WIP

Here's what I've been working on. I have yet to look at your previous patch, but I'll do so before I finish. This patch relies on the patch in bug 470914 to be able to find the test path correctly based on the location of server.js.
(Assignee)

Updated

10 years ago
Depends on: 470914
(Assignee)

Updated

10 years ago
Assignee: nobody → ted.mielczarek
Status: NEW → ASSIGNED
That's fine. I think whatever packaging you're doing could be easily ported for Fennec/Xulrunner apps (and easily transfered to the mobile devices), so assuming that the binaries, etc are in one directory is perfectly reasonable.
I'm coordinating with Joel to make sure that my end result is usable for mobile as well, so it should work out well.
After speaking with bsmedberg, we could fix the xpcshell issue I mentioned in comment 5. I filed bug 470971 about it.
Depends on: 470971
Created attachment 354325 [details] [diff] [review]
works for me

Heather: yeah, my patch wound up being eerily similar to yours. :) I'm able to use this to successfully run mochitest from outside my objdir. There are still some caveats, but this is getting closer.
Attachment #350088 - Attachment is obsolete: true
Attachment #354308 - Attachment is obsolete: true
maybe, but I like yours better :-p . Hopefully no mochitests themselves depend on this.
Created attachment 357143 [details] [diff] [review]
add --xre-path

This adds a --xre-path option. After Joel's testing, I realized that my previous approach was broken for the mobile case, since they have separate app and xulrunner dirs. Using this with a mobile build, you'd pass --xre-path=/path/to/xulrunner.
Attachment #354325 - Attachment is obsolete: true
Created attachment 359121 [details] [diff] [review]
cleaned up and updated

Ok, this changes a few things:
1) makes --xre-path default to the directory containing the app binary (so we don't even have to pass it in the non-xulrunner case)
2) Allow relative paths for all the parameters that take filenames
3) Adds an --extra-profile-file parameter, which you can pass multiple times, which lets you specify extra random files to put in the Mochitest profile. This is useful for getting the test plugin into the testing profile.

I've tested this on Windows/Mac/Linux and it works on all three.
Attachment #357143 - Attachment is obsolete: true
Attachment #359121 - Flags: review?(jwalden+bmo)
Created attachment 359299 [details] [diff] [review]
fix one problem

Oops, Joel tested and noted that I broke --xre-path. Simple fix.
Attachment #359121 - Attachment is obsolete: true
Attachment #359299 - Flags: review?(jwalden+bmo)
Attachment #359121 - Flags: review?(jwalden+bmo)
Created attachment 359737 [details] [diff] [review]
fix another problem

Apparently my previous patch broke running without --appname. (Oops.)
Attachment #359299 - Attachment is obsolete: true
Attachment #359737 - Flags: review?(jwalden+bmo)
Attachment #359299 - Flags: review?(jwalden+bmo)

Updated

10 years ago
Attachment #359737 - Flags: review?(jwalden+bmo) → review+

Comment 16

10 years ago
Comment on attachment 359737 [details] [diff] [review]
fix another problem

Seems okay, but change getfullpath to getFullPath, camelCaps style.  I wish we didn't have to pass so many parameters here to be able to do this... :-(
Yeah, I had considered an approach whereby we stuck all the necessary bits right in the harness dir when packaging, so the script could look at the default locations from automation.py, and fall back to looking directly in the harness dir. Do you think that's worth pursuing? (Even if so, I don't think having these parameters is bad, since it makes the script's dependencies visible.)

Comment 18

10 years ago
Eh...probably not; this is mostly about automated situations like tinderboxen, not really for manual use.
(Assignee)

Updated

10 years ago
Depends on: 476919
Pushed to m-c (the second time, since the first time broke because of bug 476919):
http://hg.mozilla.org/mozilla-central/rev/2e50c07f49a1
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
1.9.1:
This is needed for the context diff of bug 480077.
Note that bug 460515 landed before this bug on branch :-/
Whiteboard: [needs 1.9.1 landing]
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
(In reply to comment #20)
> Note that bug 460515 landed before this bug on branch :-/

Bug 468913 !
Pushed to 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/415a87eb0e4a
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing]
You need to log in before you can comment on or make changes to this bug.