Closed Bug 426444 Opened 15 years ago Closed 14 years ago

make talos work with xulrunner


(Release Engineering :: General, defect, P2)



(Not tracked)



(Reporter: dougt, Assigned: anodelman)



(Keywords: mobile, perf)


(1 file, 7 obsolete files)

talos needs to be trained to work with xulrunner.

The first step is to be able to pass a addition argument to the application.  We will do this by creating a new variable in the browser config object.
Take a look at the included diff to the sample config (not to be checked in, but a nice way of seeing how this is to work).

Only tested on linux, but the mac and windows changes should work (they are similar enough, right?)

There are other changes needed to make talos completely work (such as, um, monitoring the right process and not just monitoring the process named `firefox`).  Taking babysteps.
Attachment #313003 - Flags: review?
Comment on attachment 313003 [details] [diff] [review]
adds additional arg so that we can launch xulapps

wrong patch. doh.
Attachment #313003 - Attachment is obsolete: true
Attachment #313003 - Flags: review?
Attached patch the actual changes to talos (obsolete) — Splinter Review
that last patch was a hack to mozgtkembed and a separate issue.
Comment on attachment 313004 [details] [diff] [review]
the actual changes to talos

oh, hmm. is there a way to have a optional option in config?  This change will require that extra_args be defined in all config files as:

extra_args = {}

if extra_args isn't used.
Attached patch includes process_name (obsolete) — Splinter Review
this patch also includes the needed bits to allow talos to monitor a different process name than firefox. Before my change, we simply always looked for a process name of "firefox".  After my change, we can change this value in the config file.
Attachment #313004 - Attachment is obsolete: true
Found in triage. 

Can we move this to Future, as RelEng isnt doing anything here until your work is done? Also, what is the status of the above patch - noone is marked as reviewer.
Assignee: doug.turner → nobody
Priority: -- → P2
oops. somehow lost doug as assignee. reseting.
Assignee: nobody → doug.turner
Attachment #313625 - Flags: review?(anodelman)
patch ready for review.  this change is required by 419947.
Blocks: 419947
Comment on attachment 313625 [details] [diff] [review]
includes process_name

Probably don't want to use your path as the default:

# Path to Firefox to test
-firefox: firefox/firefox.exe
+firefox: /home/dougt/builds/moz1.9/mozilla/obj-i686-pc-linux-gnu-ff/dist/bin/firefox

If we are trying to generalize this to handle more applications than firefox then the 'firefox' setting in the config file should be changed out for something like 'appPath' or the like.  That would mean changing the config file and the references to the browser_config:

   browser_config = {'preferences'  : yaml_config['preferences'],
                     'extensions'   : yaml_config['extensions'],
                     'firefox'      : yaml_config['firefox'],
+                    'process_name' : yaml_config['process_name'],
+                    'extra_args'   : yaml_config['extra_args'],
                     'branch'       : yaml_config['branch'], 
                     'buildid'      : yaml_config['buildid'],

Looks like you missed a reference to firefox here:

-  cmd = ffprocess.GenerateFirefoxCommandLine(firefox_path, profile_dir, init_url)
+  cmd = ffprocess.GenerateFirefoxCommandLine(firefox_path, extra_args, profile_dir, init_url)
   (match, timed_out) = ffprocess.RunProcessAndWaitForOutput(cmd,
-                                                              'firefox',
-                                                              PROFILE_REGEX,
-                                                              30)
+                                                            'firefox',
+                                                            PROFILE_REGEX,
+                                                            30)

Also, the patch needs to be merged with the new talos process handling code that has been checked in.
Attachment #313625 - Flags: review?(anodelman) → review-
Attached patch patch v.2 (obsolete) — Splinter Review
ignore the sample.config, i will post that separately.

I would said this is good to go, but I am seeing some profile locking problems which I am unsure if they are related to my xulrunner build or my talos changes.

alice, is there a way we can test these talos changes against ff3 on all plaforms (a try server for talos?)
I have a talos staging environment that includes all platforms.  Not quite a talos try since I have to log in and manually apply my patch, but it will get it appropriately tested before we try anything crazy.
great.  Could you try this last patch out?

with xulrunner, i am seeing:

/home/dougt/builds/moz1.9/mozilla/obj-i686-pc-linux-gnu/dist/bin/xulrunner /home/dougt/builds/fennec/browser/application.ini \
-profile /tmp/tmpovVZks/profile -tp page_load_test/jss/jss.manifest -tpchrome -tpformat tinderbox -tpcycles 1
WARNING: failed to os.chmod(/tmp/tmpovVZks/profile/lock): 2 : No such file or directory

I did not see this with the last version of talos.
Keywords: mobile
The V2 patch includes both the code to work with xulrunner and the xres code - I think that we should keep that in two separate patches.

Also - you are going to want to ignore some of the changes; specifically to the manifest files and the sample.config.  You are working with the standalone manifest files which force talos to do local file load of test pages instead of through localhost, but the default behavior should continue to be to load through localhost.  Standalone talos also has some custom changes to sample.config that we don't want in in the standard, checked-in version of talos.  While we will want your changes to the sample.config having to do with path/appname/extra_args, we won't want the path to your own local copy of firefox or to the output directory.
right, that last patch isn't ready for review.  I am still seeing some issues when running against xulrunner+fennec that needs to be fixed.
Attached patch patch v.3 (obsolete) — Splinter Review
Attachment #313625 - Attachment is obsolete: true
Attachment #317130 - Attachment is obsolete: true
Comment on attachment 318281 [details] [diff] [review]
patch v.3

sample config file needs to add:

#process_name: firefox                                                                                                                                                                                        
#extra_args: ""
Attachment #318281 - Flags: review?(anodelman)
This looks good, but I want to do some testing on staging-talos before we move ahead.  At the worst all of the checked in config files used by talos machines will have to be patched to understand the new variables.  Not a big change but it would have to be lined up properly with the push of this code so as not to cause giant talos redness on the waterfall.
Alice, can you also update the standalone talos?
I was going to test the patch and ran into a couple of problems:

- can't apply the patch, throws this error: 
patching file
patch: **** malformed patch at line 50:

- patch does include changes to the sample.config file, weren't there some that are necessary?

As to the patch not applying I'm pretty confused by it.  All reference that I can find to that sort of error indicate a patch damaged during downloading, but everything looks okay to me.
should be the same thing.

I did not make any changes to sample.config dispite changes being required.  I added two new attributes:


This is the name of the process that Talos will look for when trying to determine if xul runner is still alive.

If you are using a standard version of XUL Runner, the process name is "xulrunner-bin".

extra_args: ""

This is the path to the location of your "application.ini" file. This must be a full path.
Attachment #318281 - Attachment is obsolete: true
Attachment #318281 - Flags: review?(anodelman)
Comment on attachment 320281 [details]
same as v.3, but hopefully applies for alice.

Applied the patch, ran it and got:

Traceback (most recent call last):
  File "", line 355, in <module>
  File "", line 292, in test_file
    'process_name' : yaml_config['process_name'],
KeyError: 'process_name'

I think that we want that process_name to be optional (you have a default setting for it a few lines down).  Looks like this is going to need a little finesse to make it not throw an exception when that key/value is not present in the sample.config.
Attachment #320281 - Flags: review-
as I mentioned in the bug: process_name, and extra_args are NOT optional.

Any idea how to make optional params with yaml?  I remember you saying not to worry about it.
over to alice.

All of our production mozilla tinderboxes pull from HEAD so checking this in as is will break the world.  This means you can not extend talos and check in without worrying about breaking the entire world.  I suggest that our infrastructure pulls from a tag so that you can separate development from deployment.

I would be happy to help you look into using something other than yaml_config, but it sound (from IRC) like you might have an idea of how to support optional arguments.  I didn't know that the configuration options were not extensible. I really think for talos to be more useful, supporting additional arguments is going to be needed.

Assignee: doug.turner → anodelman
Keywords: perf
OS: Mac OS X → All
(In reply to comment #22)
> as I mentioned in the bug: process_name, and extra_args are NOT optional.

extra_args should definitely be optional
well, extra_args are not optional for xulrunner applications (xulrunner <path_to_app>/application.ini)

The point is, that these can have default values in the config file.
Triaged back to Doug, while investigations continue. 

It would be great if its possible to have a solution that does not require updating, and restarting, every single talos machine in a series of scheduled downtimes...
Assignee: anodelman → doug.turner
Blocks: 419776
(In reply to comment #26)

> It would be great if its possible to have a solution that does not require
> updating, and restarting, every single talos machine in a series of scheduled
> downtimes...

That is tough to do as talos has "firefox" hardcoded in several places.
Having firefox hardcoded isn't a very big obstacle - talos machines pull fresh code per test run, so if we put together a patch that expunges the use of "firefox" it should be okay.

What this current patch does require is config file changes, which is a much more messy prospect.  We have a lot of production config files checked in and each would have to be patched.  I'd prefer if the patch could use the current config file format by having truly optional arguments that handle the xulrunner case.
sure.  easy fix.
Attached patch v.4 (obsolete) — Splinter Review
same as before, but i moved the two optional parameters (extra_args and process_name) to be tested for before calling yaml_config.  Turns out that we do this for other config settings that are optional.
Attachment #320281 - Attachment is obsolete: true
Attachment #322034 - Flags: review?(anodelman)
Blocks: 439052
this patch works for me.  over to default.
Assignee: doug.turner → nobody
Component: Release Engineering: Talos → Release Engineering: Future
Priority: P2 → P3
Blocks: 455755
hey guys, what is going on with this bug?  patch is waiting on review since May...
Component: Release Engineering: Future → Release Engineering
I'm going to re-work the patch for the latest talos code, it's on my list and should be done in the next two weeks.

It isn't currently blocking any of the work being done to automate the running of talos tests, as far as I know.
Assignee: nobody → anodelman
Priority: P3 → P2
any attempt to get talos working on maemo/fennec will require this patch.
No longer blocks: 419776
This patch covers:
- removing hard coded references to 'firefox' and replacing with variable process (for process name that should be monitored) and browser_path (for full path to browser)
- adding variable extra_args to be able to pass more options to browser during start up
- general clean up of internal function calls named 'Firefox' or 'firefox'

This is currently running on the talos stage set up and does not affect the running of standard talos boxes.
Attachment #322034 - Attachment is obsolete: true
Attachment #350240 - Flags: review?(catlee)
Attachment #322034 - Flags: review?(anodelman)
Attachment #350240 - Flags: review?(catlee) → review+
Comment on attachment 350240 [details] [diff] [review]
[Checked in]make talos suitable for mobile testing

Checking in;
/cvsroot/mozilla/testing/performance/talos/,v  <--
new revision: 1.7; previous revision: 1.6
Checking in README.txt;
/cvsroot/mozilla/testing/performance/talos/README.txt,v  <--  README.txt
new revision: 1.5; previous revision: 1.4
Checking in;
/cvsroot/mozilla/testing/performance/talos/,v  <--
new revision: 1.3; previous revision: 1.2
Checking in;
/cvsroot/mozilla/testing/performance/talos/,v  <--
new revision: 1.4; previous revision: 1.3
Checking in;
/cvsroot/mozilla/testing/performance/talos/,v  <--
new revision: 1.2; previous revision: 1.1
Checking in;
/cvsroot/mozilla/testing/performance/talos/,v  <--
new revision: 1.8; previous revision: 1.7
Checking in;
/cvsroot/mozilla/testing/performance/talos/,v  <--
new revision: 1.9; previous revision: 1.8
Checking in;
/cvsroot/mozilla/testing/performance/talos/,v  <--
new revision: 1.10; previous revision: 1.9
Checking in;
/cvsroot/mozilla/testing/performance/talos/,v  <--
new revision: 1.8; previous revision: 1.7
Checking in;
/cvsroot/mozilla/testing/performance/talos/,v  <--
new revision: 1.6; previous revision: 1.5
Checking in;
/cvsroot/mozilla/testing/performance/talos/,v  <--
new revision: 1.32; previous revision: 1.31
Checking in sample.config;
/cvsroot/mozilla/testing/performance/talos/sample.config,v  <--  sample.config
new revision: 1.20; previous revision: 1.19
Checking in;
/cvsroot/mozilla/testing/performance/talos/,v  <--
new revision: 1.17; previous revision: 1.16
Attachment #350240 - Attachment description: make talos suitable for mobile testing → [Checked in]make talos suitable for mobile testing
Attachment #350240 - Flags: checked‑in+
Follow up for renaming filenames in bug 467683
Successfully cycling on production talos boxes.  Further talos/mobile engineering work is being tracked in bug 455755.
Closed: 14 years ago
Resolution: --- → FIXED
Product: → Release Engineering
You need to log in before you can comment on or make changes to this bug.