talos should install pageloader as an extension, not a bundle, and bundles should be deprecated

RESOLVED FIXED

Status

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: k0scist, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozbase][good first bug][mentor=jhammel])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

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

Comment 1

7 years ago
We already install pageloader as an extension for the mobile case.  It would be nice to have parity here
(Reporter)

Updated

7 years ago
Whiteboard: [mozbase][good first bug][mentor=jhammel]

Comment 2

7 years ago
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?
Attachment #609984 - Flags: review?(madbyk)
Attachment #609984 - Flags: review?(jmaher)
Attachment #609984 - Flags: review?(jhammel)
(Reporter)

Comment 3

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

Comment 4

7 years ago
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.
Attachment #609984 - Flags: review?(jhammel)
(Reporter)

Comment 5

7 years ago
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.
Attachment #609984 - Flags: review?(madbyk) → review+

Comment 7

7 years ago
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).
Attachment #609984 - Flags: review?(jmaher) → review+
(Reporter)

Comment 9

7 years ago
Morphing bug wrt comment 8
Summary: talos should clean up after pageloader bundle install → talos should install pageloader as an extension, not a bundle, and bundles should be deprecated

Comment 10

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

Comment 12

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

Comment 13

7 years ago
Created attachment 611240 [details] [diff] [review]
Deleted all code referencing bundles in talos
Attachment #611240 - Flags: review?(madbyk)
Attachment #611240 - Flags: review?(jmaher)
Attachment #611240 - Flags: review?(jhammel)
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.
Attachment #611240 - Flags: review?(jmaher) → review+

Comment 15

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

Comment 16

7 years ago
Created attachment 611463 [details] [diff] [review]
Deleted all code referencing bundles in talos

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.
Attachment #611240 - Attachment is obsolete: true
Attachment #611463 - Flags: review?(madbyk)
Attachment #611463 - Flags: review?(jmaher)
Attachment #611463 - Flags: review?(jhammel)
Attachment #611240 - Flags: review?(madbyk)
Attachment #611240 - Flags: review?(jhammel)
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.
Attachment #611463 - Flags: review?(jmaher) → review-

Comment 18

7 years ago
Created attachment 611491 [details] [diff] [review]
Deleted all code referencing bundles in talos

Added sample.config. The changes from the first patch are also here, should that be made obsolete?
Attachment #611463 - Attachment is obsolete: true
Attachment #611491 - Flags: review?(madbyk)
Attachment #611491 - Flags: review?(jmaher)
Attachment #611491 - Flags: review?(jhammel)
Attachment #611463 - Flags: review?(madbyk)
Attachment #611463 - Flags: review?(jhammel)
(Reporter)

Comment 19

7 years ago
Comment on attachment 611491 [details] [diff] [review]
Deleted all code referencing bundles in talos

looks good to me
Attachment #611491 - Flags: review?(jhammel) → review+
Attachment #611491 - Flags: review?(jmaher) → review+
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?

Comment 23

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

Comment 25

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

Comment 26

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

Comment 29

7 years ago
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!
Attachment #611240 - Flags: review+
(Reporter)

Comment 32

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

Comment 33

7 years ago
Created attachment 612780 [details]
Screenshot of my tsvg problem

(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 37

7 years ago
Created attachment 612883 [details] [diff] [review]
Deleted all code referencing bundles in talos and extensions are now put in a staged diectory
Attachment #609984 - Attachment is obsolete: true
Attachment #611491 - Attachment is obsolete: true
Attachment #612883 - Flags: review?(madbyk)
Attachment #612883 - Flags: review?(jmaher)
Attachment #612883 - Flags: review?(jhammel)
Attachment #611491 - Flags: review?(madbyk)
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!
Attachment #612883 - Flags: review?(jmaher) → review+
(Reporter)

Comment 39

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

Comment 40

7 years ago
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*
Attachment #612883 - Flags: review?(jhammel) → review+
(Reporter)

Updated

7 years ago
See Also: → bug 693743
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.
Attachment #612883 - Flags: review?(madbyk) → review+
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

Comment 45

7 years ago
Great that looks much nicer!
the warnings and errors are not to be concerned about, they are infrastructure related.
http://hg.mozilla.org/build/talos/rev/4343f5e91fae
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.