If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

make talos work with xulrunner

RESOLVED FIXED

Status

Release Engineering
General
P2
normal
RESOLVED FIXED
10 years ago
4 years ago

People

(Reporter: dougt, Assigned: alice)

Tracking

({mobile, perf})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

10 years ago
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.
(Reporter)

Comment 1

10 years ago
Created attachment 313003 [details] [diff] [review]
adds additional arg so that we can launch xulapps

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?
(Reporter)

Comment 2

10 years ago
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?
(Reporter)

Comment 3

10 years ago
Created attachment 313004 [details] [diff] [review]
the actual changes to talos

that last patch was a hack to mozgtkembed and a separate issue.
(Reporter)

Comment 4

10 years ago
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.
(Reporter)

Comment 5

10 years ago
Created attachment 313625 [details] [diff] [review]
includes process_name

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
(Reporter)

Updated

10 years ago
Attachment #313625 - Flags: review?(anodelman)
(Reporter)

Comment 8

10 years ago
patch ready for review.  this change is required by 419947.
Blocks: 419947
(Assignee)

Comment 9

10 years ago
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:

talos/ffsetup.py
-  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-
(Reporter)

Comment 10

10 years ago
Created attachment 317130 [details] [diff] [review]
patch v.2

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?)
(Assignee)

Comment 11

10 years ago
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.
(Reporter)

Comment 12

10 years ago
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.

Updated

10 years ago
Keywords: mobile
(Assignee)

Comment 13

10 years ago
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.
(Reporter)

Comment 14

10 years ago
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.
(Reporter)

Comment 15

10 years ago
Created attachment 318281 [details] [diff] [review]
patch v.3
Attachment #313625 - Attachment is obsolete: true
Attachment #317130 - Attachment is obsolete: true
(Reporter)

Comment 16

10 years ago
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)
(Assignee)

Comment 17

10 years ago
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.
(Reporter)

Comment 18

10 years ago
Alice, can you also update the standalone talos?
(Assignee)

Comment 19

10 years ago
I was going to test the patch and ran into a couple of problems:

- can't apply the patch, throws this error: 
patching file ffprocess_linux.py
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.
(Reporter)

Comment 20

10 years ago
Created attachment 320281 [details]
same as v.3, but hopefully applies for alice.

should be the same thing.

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


process_name:

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)
(Assignee)

Comment 21

10 years ago
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 "run_tests.py", line 355, in <module>
    test_file(arg)
  File "run_tests.py", 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-
(Reporter)

Comment 22

10 years ago
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.
(Reporter)

Comment 23

10 years ago
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
(Reporter)

Comment 25

10 years ago
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

Updated

10 years ago
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.
(Assignee)

Comment 28

10 years ago
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.
(Reporter)

Comment 29

10 years ago
sure.  easy fix.
(Reporter)

Comment 30

10 years ago
Created attachment 322034 [details] [diff] [review]
v.4

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)

Updated

9 years ago
Blocks: 439052
(Reporter)

Comment 31

9 years ago
this patch works for me.  over to default.
Assignee: doug.turner → nobody
Component: Release Engineering: Talos → Release Engineering: Future
Priority: P2 → P3
(Assignee)

Updated

9 years ago
Blocks: 455755

Comment 32

9 years ago
hey guys, what is going on with this bug?  patch is waiting on review since May...
Component: Release Engineering: Future → Release Engineering
(Assignee)

Comment 33

9 years ago
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)

Updated

9 years ago
Assignee: nobody → anodelman
Priority: P3 → P2
(Reporter)

Comment 34

9 years ago
any attempt to get talos working on maemo/fennec will require this patch.
(Assignee)

Updated

9 years ago
No longer blocks: 419776
(Assignee)

Comment 35

9 years ago
Created attachment 350240 [details] [diff] [review]
[Checked in]make talos suitable for mobile testing

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)

Updated

9 years ago
Attachment #350240 - Flags: review?(catlee) → review+
(Assignee)

Comment 36

9 years ago
Comment on attachment 350240 [details] [diff] [review]
[Checked in]make talos suitable for mobile testing

Checking in PerfConfigurator.py;
/cvsroot/mozilla/testing/performance/talos/PerfConfigurator.py,v  <--  PerfConfigurator.py
new revision: 1.7; previous revision: 1.6
done
Checking in README.txt;
/cvsroot/mozilla/testing/performance/talos/README.txt,v  <--  README.txt
new revision: 1.5; previous revision: 1.4
done
Checking in cmanager_linux.py;
/cvsroot/mozilla/testing/performance/talos/cmanager_linux.py,v  <--  cmanager_linux.py
new revision: 1.3; previous revision: 1.2
done
Checking in cmanager_mac.py;
/cvsroot/mozilla/testing/performance/talos/cmanager_mac.py,v  <--  cmanager_mac.py
new revision: 1.4; previous revision: 1.3
done
Checking in cmanager_win32.py;
/cvsroot/mozilla/testing/performance/talos/cmanager_win32.py,v  <--  cmanager_win32.py
new revision: 1.2; previous revision: 1.1
done
Checking in ffprocess.py;
/cvsroot/mozilla/testing/performance/talos/ffprocess.py,v  <--  ffprocess.py
new revision: 1.8; previous revision: 1.7
done
Checking in ffprocess_linux.py;
/cvsroot/mozilla/testing/performance/talos/ffprocess_linux.py,v  <--  ffprocess_linux.py
new revision: 1.9; previous revision: 1.8
done
Checking in ffprocess_mac.py;
/cvsroot/mozilla/testing/performance/talos/ffprocess_mac.py,v  <--  ffprocess_mac.py
new revision: 1.10; previous revision: 1.9
done
Checking in ffprocess_win32.py;
/cvsroot/mozilla/testing/performance/talos/ffprocess_win32.py,v  <--  ffprocess_win32.py
new revision: 1.8; previous revision: 1.7
done
Checking in ffsetup.py;
/cvsroot/mozilla/testing/performance/talos/ffsetup.py,v  <--  ffsetup.py
new revision: 1.6; previous revision: 1.5
done
Checking in run_tests.py;
/cvsroot/mozilla/testing/performance/talos/run_tests.py,v  <--  run_tests.py
new revision: 1.32; previous revision: 1.31
done
Checking in sample.config;
/cvsroot/mozilla/testing/performance/talos/sample.config,v  <--  sample.config
new revision: 1.20; previous revision: 1.19
done
Checking in ttest.py;
/cvsroot/mozilla/testing/performance/talos/ttest.py,v  <--  ttest.py
new revision: 1.17; previous revision: 1.16
done
Attachment #350240 - Attachment description: make talos suitable for mobile testing → [Checked in]make talos suitable for mobile testing
Attachment #350240 - Flags: checked‑in+
(Assignee)

Comment 37

9 years ago
Follow up for renaming filenames in bug 467683
(Assignee)

Comment 38

9 years ago
Successfully cycling on production talos boxes.  Further talos/mobile engineering work is being tracked in bug 455755.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.