We currently install pageloader into appdir/distribution/bundles: http://hg.mozilla.org/build/talos/file/ff646574951c/talos/ffsetup.py#l251 However, we never clean this up. This has a few negative consequences: - if a bundle is changed and installed, we don't clean up the files between installs. This means that if there are vestiges of the old bundle laying around (the main, maybe only, case being pageloader) there can be unintended behaviour. - we litter the filesystem with these bundles. We also might not have write access to the browser directory. The strange thing is that talos will complain about the bundle not being installed anyway, even though it was clearly there from another talos instantiation. We should clean up the bundles. Alternatively, we can install pageloader as an extension and forget about bundles; as best I can tell, pageloader is the only bundle actually used.

We already install pageloader as an extension for the mobile case. It would be nice to have parity here

If pageloader is the only bundle used should the bundles functionality be removed entirely?

(In reply to Fahim Zahur from comment #2) > Created attachment 609984 [details] [diff] [review] > Changed sample.config to load pageloader as extension instead of bundle > > If pageloader is the only bundle used should the bundles functionality be > removed entirely? IMHO yes. However, other parties say no

Comment on attachment 609984 [details] [diff] [review] Changed sample.config to load pageloader as extension instead of bundle I can't really review this. I would like to deprecate pageloader as a bundle but have been told not to.

if we do deprecate bundles + talos, we should probably deprecate them whole-heartedly, moreso than what attachment 609984 [details] [diff] [review] does

Comment on attachment 609984 [details] [diff] [review] Changed sample.config to load pageloader as extension instead of bundle Review of attachment 609984 [details] [diff] [review]: ----------------------------------------------------------------- I am not going against what jhammelsaid, and I think there is more to this bug than this attachment but the attachment itself seems good and can be used with the final resolution of the bug.

jmaher, when you review can you outline the direction we should take w.r.t. the bundle for future vesions of talos to clear up the confusion on the bug from jhammel and BYK? thanks.

Comment on attachment 609984 [details] [diff] [review] Changed sample.config to load pageloader as extension instead of bundle Review of attachment 609984 [details] [diff] [review]: ----------------------------------------------------------------- This has been a topic of argument in the past. We switched about 1.5 years ago to bundles ({approot}/distribution/bundles) for unpacked extensions. The main reason was compatibility, and I think there are a few other smaller reasons. Technically there is no difference in how our tests are run or the results if we use either. On mobile we do NOT have a distribution/bundles option, so moving to extensions is ideal. Lets test this really well, and then clean up our code that touches distribution/bundles (yes, this is just removing code which is always a win).

Morphing bug wrt comment 8

Is cleaning up bundles a part of this bug then?

I would say it should be. We can make it a secondary patch! Most likely this is going to be removing code:)

Sounds good I'll get to work on it then :)

Comment on attachment 611240 [details] [diff] [review] Deleted all code referencing bundles in talos Review of attachment 611240 [details] [diff] [review]: ----------------------------------------------------------------- thanks! code removal is great.

Comment on attachment 611240 [details] [diff] [review] Deleted all code referencing bundles in talos >diff -r 8c0c35cac3e2 talos/addon.config >--- a/talos/addon.config Mon Mar 19 21:36:15 2012 -0400 >+++ b/talos/addon.config Sat Mar 31 20:29:12 2012 -0500 >@@ -64,17 +64,12 @@ > > > # Extensions to install in test (use "extensions: []" for none) >-extensions: [] >+extensions: ['${talos}/pageloader'] > > #any directories whose contents need to be installed in the browser before running the tests > # this assumes that the directories themselves already exist in the firefox path > dirs: {} > >-# any extension-like bundles which should be installed with the application >-# in a distribution/bundles directory. >-bundles: >- talos_pageloader : ${talos}/pageloader >- > # Environment variables to set during test (use env: {} for none) > env : > NO_EM_RESTART : 1 >diff -r 8c0c35cac3e2 talos/cycles.config >--- a/talos/cycles.config Mon Mar 19 21:36:15 2012 -0400 >+++ b/talos/cycles.config Sat Mar 31 20:29:12 2012 -0500 >@@ -56,17 +56,12 @@ > browser.cache.disk.smart_size.first_run: false > > # Extensions to install in test (use "extensions: []" for none) >-extensions: [] >+extensions: ['${talos}/pageloader'] > > #any directories whose contents need to be installed in the browser before running the tests > # this assumes that the directories themselves already exist in the firefox path > dirs: {} > >-# any extension-like bundles which should be installed with the application >-# in a distribution/bundles directory. >-bundles: >- talos_pageloader : ${talos}/pageloader >- > # Environment variables to set during test (use env: {} for none) > env : > NO_EM_RESTART : 1 >diff -r 8c0c35cac3e2 talos/ffsetup.py >--- a/talos/ffsetup.py Mon Mar 19 21:36:15 2012 -0400 >+++ b/talos/ffsetup.py Sat Mar 31 20:29:12 2012 -0500 >@@ -246,35 +246,6 @@ > for fromfile in fromfiles: > self.ffprocess.copyFile(fromfile, todir) > >- def InstallBundleInBrowser(self, browser_path, bundlename, bundle_path): >- """ >- Take the given directory and unzip the bundle into >- distribution/bundles/bundlename. >- """ >- >- # sanity check >- if not os.path.exists(bundle_path): >- raise talosError("bundle path '%s' does not exist") >- >- destpath = os.path.join(os.path.dirname(browser_path), >- 'distribution', 'bundles', bundlename) >- if os.path.exists(destpath): >- shutil.rmtree(destpath) >- >- if os.path.isdir(bundle_path): >- # XXX work around shutil.copytree: >- # "The destination directory must not already exist." >- parent = os.path.dirname(destpath) >- if not os.path.exists(parent): >- os.makedirs(parent) >- >- # bundle is a directory >- shutil.copytree(bundle_path, destpath) >- else: >- # bundle is an xpi >- os.makedirs(destpath) >- zip_extractall(zipfile.ZipFile(bundle_path), destpath) >- > def InitializeNewProfile(self, profile_dir, browser_config): > """Runs browser with the new profile directory, to negate any performance > hit that could occur as a result of starting up with a new profile. >diff -r 8c0c35cac3e2 talos/run_tests.py >--- a/talos/run_tests.py Mon Mar 19 21:36:15 2012 -0400 >+++ b/talos/run_tests.py Sat Mar 31 20:29:12 2012 -0500 >@@ -498,7 +498,6 @@ > ] > optional = {'addon_id': 'NULL', > 'bcontroller_config': 'bcontroller.yml', >- 'bundles': {}, > 'branch_name': '', > 'child_process': 'plugin-container', > 'develop': False, >@@ -528,8 +527,6 @@ > # fix paths to substitute > # `os.path.dirname(os.path.abspath(__file__))` for ${talos} > # https://bugzilla.mozilla.org/show_bug.cgi?id=705809 >- browser_config['bundles'] = dict([(i, utils.interpolatePath(j)) >- for i,j in browser_config['bundles'].items()]) > browser_config['extensions'] = [utils.interpolatePath(i) > for i in browser_config['extensions']] > browser_config['dirs'] = dict([(i, utils.interpolatePath(j)) >diff -r 8c0c35cac3e2 talos/sample.2.config >--- a/talos/sample.2.config Mon Mar 19 21:36:15 2012 -0400 >+++ b/talos/sample.2.config Sat Mar 31 20:29:12 2012 -0500 >@@ -57,17 +57,12 @@ > > > # Extensions to install in test (use "extensions: []" for none) >-extensions: [] >+extensions: ['${talos}/pageloader'] > > #any directories whose contents need to be installed in the browser before running the tests > # this assumes that the directories themselves already exist in the firefox path > dirs: {} > >-# any extension-like bundles which should be installed with the application >-# in a distribution/bundles directory. >-bundles: >- talos_pageloader : ${talos}/pageloader >- > # Environment variables to set during test (use env: {} for none) > env : > NO_EM_RESTART : 1 >diff -r 8c0c35cac3e2 talos/ttest.py >--- a/talos/ttest.py Mon Mar 19 21:36:15 2012 -0400 >+++ b/talos/ttest.py Sat Mar 31 20:29:12 2012 -0500 >@@ -250,11 +250,6 @@ > utils.debug(browser_config['process'] + msg) > running_processes_str = ", ".join([('[%s] %s' % (pid, process_name)) for pid, process_name in running_processes]) > raise talosError("Found processes still running: %s. Please close them before running talos." % running_processes_str) >- >- for bundlename in browser_config['bundles']: >- self._ffsetup.InstallBundleInBrowser(browser_config['browser_path'], >- bundlename, >- browser_config['bundles'][bundlename]) > > # add any provided directories to the installed browser > for dir in browser_config['dirs']: >diff -r 8c0c35cac3e2 talos/xperf.config >--- a/talos/xperf.config Mon Mar 19 21:36:15 2012 -0400 >+++ b/talos/xperf.config Sat Mar 31 20:29:12 2012 -0500 >@@ -57,17 +57,12 @@ > > > # Extensions to install in test (use "extensions: []" for none) >-extensions: [] >+extensions: ['${talos}/pageloader'] > > #any directories whose contents need to be installed in the browser before running the tests > # this assumes that the directories themselves already exist in the firefox path > dirs: {} > >-# any extension-like bundles which should be installed with the application >-# in a distribution/bundles directory. >-bundles: >- talos_pageloader : ${talos}/pageloader >- > # Environment variables to set during test (use env: {} for none) > env : > NO_EM_RESTART : 1

I just realized there was a patch from an older bug hidden inside the previous attachment. So I've uploaded a corrected version. Sorry for the messiness.

Comment on attachment 611463 [details] [diff] [review] Deleted all code referencing bundles in talos Review of attachment 611463 [details] [diff] [review]: ----------------------------------------------------------------- I think you are missing sample.config.

Added sample.config. The changes from the first patch are also here, should that be made obsolete?

Comment on attachment 611491 [details] [diff] [review] Deleted all code referencing bundles in talos looks good to me

pushing to try: https://tbpl.mozilla.org/?tree=Try&rev=05ecf77034d0

Try run for 05ecf77034d0 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=05ecf77034d0 Results (out of 76 total builds): success: 32 failure: 44 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jhammel@mozilla.com-05ecf77034d0

all of the tests that depend on pageloader failed by hanging and crashing or needing to be killed. Can you test your patch with tsvg to see what might be going on?

When running tsvg I keep getting a prompt to either open the svg page with IE or save to file. This happens even when I'm not using the patch. Is this a bug?

that is a good indicator that pageloader isn't installed properly. I wonder if there is a little detail we are missing when changing from bundle to extension.

I'll look into this when I have some free moments, but if someone were to beat me to it I wouldn't complain about that either. FWIW, I have not tried the patch yet.

I tried this with tsvg last night and it worked fine. There are a lot of weird failures in the push, like: Traceback (most recent call last): File "c:\talos-slave\talos-data\talos\bcontroller.py", line 252, in ? sys.exit(main()) File "c:\talos-slave\talos-data\talos\bcontroller.py", line 249, in main bcontroller.run() File "c:\talos-slave\talos-data\talos\bcontroller.py", line 192, in run results_file = open(self.browser_log, "a") IOError: [Errno 13] Permission denied: 'browser_output.txt' Screen width/height:1280/1024 colorDepth:24 Browser inner width/height: 1016/622 I doubt this has anything to do with bundle/extension. Fahim, can you give more detail a screen shot, etc about your issue in comment 23? It might be worth pushing to try again. IIRC, there was infrastructure issues around the time of push (can't recall the details)

I was just going to comment in here and jhammel beat me to it. I ran tsvg and tp5 just fine. lets push up to try server again and see if we can get green. Fahim, lets try to figure out what your problem was running the tests locally. It has to be something simple.

re-trying: https://tbpl.mozilla.org/?tree=Try&rev=a32646882c57

It seems like i get mad Firefox crashes when trying this patch. Its possible i pushed on top of a crashy result set. It is more likely somehow we're doing something that crashes Firefox. I hadn't seen this locally. Someone will need to research this issue. While I could believe that we're doing something wrong here, it is hard to fathom why we would crash Firefox.

Jeff, "Permission denied" errors are likely due to some messed up permissions in your file system. NTFS permissions tend to get messed up, especially if you run something in admin mode without noticing it. Whatever it is, it does not seem to be introduced with Fahim's patch to me.

Comment on attachment 611240 [details] [diff] [review] Deleted all code referencing bundles in talos Looks good to me. Also it seems like a very careful and neat work. Well done!

(In reply to Burak Yiğit Kaya [:BYK] from comment #30) > Jeff, > > "Permission denied" errors are likely due to some messed up permissions in > your file system. NTFS permissions tend to get messed up, especially if you > run something in admin mode without noticing it. Whatever it is, it does not > seem to be introduced with Fahim's patch to me. That is likely. But the rerun does not have file permission (and other infrastructure) errors, but browser crashes for the most part. As I said, it is possible I pushed on top of a bad patch. But it seems unlikely. In any case, we should look into this. Of course, I can just push to try again, but itd be nice to have confidence in doing so. My local run does not have this crashiness, but I've only had time to do a single test case.

(In reply to Jeff Hammel [:jhammel] from comment #26) > I tried this with tsvg last night and it worked fine. There are a lot of > weird failures in the push, like: > > Traceback (most recent call last): > File "c:\talos-slave\talos-data\talos\bcontroller.py", line 252, in ? > sys.exit(main()) > File "c:\talos-slave\talos-data\talos\bcontroller.py", line 249, in main > bcontroller.run() > File "c:\talos-slave\talos-data\talos\bcontroller.py", line 192, in run > results_file = open(self.browser_log, "a") > IOError: [Errno 13] Permission denied: 'browser_output.txt' > Screen width/height:1280/1024 > colorDepth:24 > Browser inner width/height: 1016/622 > > I doubt this has anything to do with bundle/extension. > > Fahim, can you give more detail a screen shot, etc about your issue in > comment 23? > > It might be worth pushing to try again. IIRC, there was infrastructure > issues around the time of push (can't recall the details) I get the following command-line output. It's on a fresh copy of talos: setting debug DEBUG: running test file mytest.yml DEBUG: using testdate: 1333677582 DEBUG: actual date: 1333677582 RETURN:<a href = "http://hg.mozilla.org/releases/mozilla-release/rev/d03b d">rev:d03b51a9b2bd</a> qm-pxp01: Started Thu, 05 Apr 2012 20:59:43 Running test tsvg: Started Thu, 05 Apr 2012 20:59:43 DEBUG: operating with platform_type : w7_ DEBUG: created profile ERROR:root:[Errno 10054] An existing connection was forcibly closed by th e host Screen width/height:1440/900 colorDepth:24 Browser inner width/height: 1006/631 DEBUG: initialized firefox.exe DEBUG: command line: '"C:\Program Files (x86)\Mozilla Firefox\firefox.exe file c:\\\users\\\fzahur\\\appdata\\\local\\\temp\\\tmpvygwor\\\profile - :\C:\Users\fzahur\Talos\talos\page_load_test\svg\svg.manifest.develop -tp -tpnoisy -tpformat tinderbox -tpcycles 5 -tppagecycles 1'

Try run for a32646882c57 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=a32646882c57 Results (out of 60 total builds): success: 27 failure: 33 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jhammel@mozilla.com-a32646882c57

I downloaded the try server build and talos bits and I found out that the pageloader extension is disabled by default! I know one of the reasons for the bundles is to allow compatibility and enabled by default. We don't experience this on android, nor on mochitest. I assume there is something small we need to adjust.

I can't get this patch to apply at all, but I figured out the problem! We need to fix ffsetup.py to put the extension in a staged directory: http://hg.mozilla.org/build/talos/file/a9f14a82860e/talos/ffsetup.py#l178 this line: addon_path = os.path.join(profile_path, 'extensions', addon_id) should be: addon_path = os.path.join(profile_path, 'extensions', 'staged', addon_id) There are 3 instances in ffsetup.py that need to be adjusted accordingly. Once this change is in place we are good to go. If you could rebase the patch (remove it, then 'hg pull -u', then reapply and create new diff), that might help me apply it easier.

Comment on attachment 612883 [details] [diff] [review] Deleted all code referencing bundles in talos and extensions are now put in a staged diectory Review of attachment 612883 [details] [diff] [review]: ----------------------------------------------------------------- thanks!

I don't understand the magical staged thing. In previous testing of jetpack generated addons I've never had to do this. I wonder what the magic is ...

Comment on attachment 612883 [details] [diff] [review] Deleted all code referencing bundles in talos and extensions are now put in a staged diectory wfm; i wish i understood *why*

Try run for 20cd3d0b435a is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=20cd3d0b435a Results (out of 95 total builds): success: 22 warnings: 1 failure: 72 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmaher@mozilla.com-20cd3d0b435a

these failures are due to a downtime, lets repush and see what happens.

Comment on attachment 612883 [details] [diff] [review] Deleted all code referencing bundles in talos and extensions are now put in a staged diectory Review of attachment 612883 [details] [diff] [review]: ----------------------------------------------------------------- LGTM ::: talos/ffsetup.py @@ +224,4 @@ > self.ffprocess.addRemoteServerPref(profile_dir, webserver) > > # Install the extensions. > + extension_dir = os.path.join(profile_dir, 'extensions', 'staged') I think this variable should be defined earlier and used while constructing the addon_file and addon_path variables.

Try run for 092c213909c8 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=092c213909c8 Results (out of 75 total builds): exception: 1 success: 71 warnings: 1 failure: 2 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmaher@mozilla.com-092c213909c8

Great that looks much nicer!

the warnings and errors are not to be concerned about, they are infrastructure related.