The current Talos drops stuff into components/ and chrome/. With bug 579178 this is intentionally breaking, so we need Talos pageloader to instead use a distribution bundle.

This patch removes our use of CVS pageloader, and replaces it with repo, currently at http://hg.mozilla.org/users/bsmedberg_mozilla.com/new-pageloader I'd like permission (from bhearsum?) to go ahead and create hg.mozilla.org/build/pageloader for this.

This looks reasonable except for the integration with the current set up: - since we are just gutting generate component script we should ditch it and have the buildbotcustom steps modified to download the pageloader zip - we should keep a copy of the pageloader zip on the build network where it can be downloaded from (like we do with the dirty profiles)

Also, this should be tested on stage before roll out. This can be done by using the customTalos TalosFactory option to have it pull a local zip of talos instead of a fresh cvs check out.

I'll just commit pageloader.xpi to build-tools, if this is the right way to go. I'm not completely clear on what the mobile test factory ought to be doing, but I'm certain that running generate-tpcomponent.py is not right.

(In reply to comment #6) > Created attachment 459825 [details] [diff] [review] > Download pageloader.xpi directly from build-tools, rev. 1 > > I'll just commit pageloader.xpi to build-tools, if this is the right way to go. > I'm not completely clear on what the mobile test factory ought to be doing, but > I'm certain that running generate-tpcomponent.py is not right. Benjamin, that is how we have been generating the pageloader for the n810s and n900s for quite a while now and it has been working and is how, aiui, we generate pageloader for firefox builds. Is there any reason that we can't use the same new xpi for mobile builds?

No, you probably should use the same xpi. I just didn't understand how generate-tploader.py was downloaded in the mobile config: there didn't seem to be a download step as in the normal config factory.

I'll be responsible for staging this on the minis - you should check with Aki for how this will affect mobile set up.

Comment on attachment 459143 [details] [diff] [review] Talos (CVS) changes, rev. 1 I'd prefer this to use this formula: self.addStep(DownloadFile( url="%s/%s" % (self.supportUrlBase, self.plugins), workdir=os.path.join(self.workdirBase, "talos/base_profile"), )) So that the file is Downloaded not from the buildbot master, but from the releng web server (it's used to host dirty profiles/plugins/etc).

Comment on attachment 459825 [details] [diff] [review] Download pageloader.xpi directly from build-tools, rev. 1 Sorry, previous comment were for this patch not for the talos cvs changes.

Bad news about zipfile.extractall in your cvs talos patch: " New in version 2.6. " Talos slaves run with python 2.4, so all talos code has to be compliant with that.

I refactored the existing compatibility code into a helper function and used it in both places.

The buildbotcustom patch already uses self.supportUrlBase, which IIUC would download pageloader.xpi off of the releng server. I'm not sure I understand exactly what ought to change there: did you mean self.plugins exactly? It seems like that is already downloaded and extracted into talos/base_profile, but this needs to end up in talos/page_load_test/pageloader.xpi

Comment on attachment 459825 [details] [diff] [review] Download pageloader.xpi directly from build-tools, rev. 1 Re-requesting review because I think this is right.

I think that the unzipped pageloader should be checked into the build repository at tools/buildfarm/utils, then the xpi version can be created and posted to the build server - I like this better then checking in a xpi which seems wrong to me. And yes, sorry about the confusion about the previous buildbotcustom patch - I was misreading it as doing a local filedownload instead of from the server.

The unzipped version is already in hg.mozilla.org/build/pageloader. I'm happy to either commit a zipped version into build/tools, or just post the XPI which can be uploaded to the build server.

Same as the prior CVS patch except `cvs up`ped to tip.

Comment on attachment 460575 [details] [diff] [review] Talos (CVS) changes, rev. 2, rebased Green on talos-stage.

bsmedberg - can you update your buildbotcustom patch to reflect the location of the xpi at: http://build.mozilla.org/talos/xpis/pageloader.xpi

Where should we put the xpi in fennec (no distribution/bundles directory)?

Firefox doesn't have one either until we create one. Talos does this automatically.

This seems to work for the n900s in staging; awesome. At some point I'll test bug 454731 against this and update the patch.

Comment on attachment 461241 [details] [diff] [review] Download pageloader.xpi directly from build-tools, rev. 2 http://hg.mozilla.org/build/buildbotcustom/rev/ef1b3deb649b

Comment on attachment 460575 [details] [diff] [review] Talos (CVS) changes, rev. 2, rebased Checking in ffsetup.py; /cvsroot/mozilla/testing/performance/talos/ffsetup.py,v <-- ffsetup.py new revision: 1.21; previous revision: 1.20 done Checking in run_tests.py; /cvsroot/mozilla/testing/performance/talos/run_tests.py,v <-- run_tests.py new revision: 1.66; previous revision: 1.65 done Checking in sample.config; /cvsroot/mozilla/testing/performance/talos/sample.config,v <-- sample.config new revision: 1.49; previous revision: 1.48 done Checking in ttest.py; /cvsroot/mozilla/testing/performance/talos/ttest.py,v <-- ttest.py new revision: 1.46; previous revision: 1.45 done

The buildbotcustom patch as landed is a no-op for mobile. This is because it was landed on the wrong branch and also didn't modify the code that the devices are actually running. Currently, the devices download a pageloader tarball. This pageloader tarball is generated using the generate-tpcomponent.py script on production-mobile-master. In fact, generate-tpcomponent.py is still being used but is being run on the master which seems contrary to the goals of this bug and the patch that was landed. I am not sure if one of the goals of this bug is to get the mobile devices to actually make use of the new pageloader or not. This patch is to bring the buildbot-0.8.0 branch back inline with the default branch. I have another patch to do what I think is required to use the new xpi on mobile devices.

This patch removes the option of using generate-tpcomponents.py on the slave as well as in a tarball generated on the master using generate-tpcomponents.py. There would be a very small corresponding buildbot-configs patch required for this to work. I also need to test this in staging before I ask for review and land it. Before I test it, I would like to know that what I am doing is valid. With this patch, I will download pageloader.xpi into talos/page_load_tests. Looking at the changes made for desktop talos, it seems like that is the limit of changes needed, is that correct?

Comment on attachment 462978 [details] [diff] [review] use the new xpi file on mobile Why are you using wget instead of the DownloadFile step? If you're using wget, I think you need -O so that it doesn't rename the file if it already exists.

Comment on attachment 462976 [details] [diff] [review] Correct the mobiletestfactory.py changes Giving a tentative r+ here because this just seems to be putting back what was taken out in the original patch. However, I don't understand the other patch you asked bsmedberg for feedback on then because it seems to be fixing the same thing in a different way. Is this the quick fix/reversion and the other is the new/proper way?

Comment on attachment 462976 [details] [diff] [review] Correct the mobiletestfactory.py changes generate-tpcomponent doesn't make any sense (and should probably be removed) any more: Talos won't use it, and even if you were using an old version of talos it won't work as soon as I check in bug 579178, which is why we were doing all this in the first place.

(In reply to comment #29) > Comment on attachment 462978 [details] [diff] [review] > use the new xpi file on mobile > > Why are you using wget instead of the DownloadFile step? If you're using wget, > I think you need -O so that it doesn't rename the file if it already exists. I am using wget because that is what has always been done on mobile. My reading of http://hg.mozilla.org/build/buildbotcustom/file/5a52ac1ad69f/steps/misc.py#l369 seems to confirm that aside from missing --progress=dot:mega -N, this is functionally identical. Yes, in theory we should worry about overwriting vs appending a number to the filename, but these devices format the filesystem that they run tests from every single boot and reboot at the end of every test run. I could modify the arguments to match exactly what is in DownloadFile which results in command=['wget', '--progress=dot:mega', '-N', self.pageloader_url] We don't use -O in the DownloadFile step either, so I don't know if it should be used here. Using the -O flag means that we have to hardcode the output name to pageloader.xpi, which seems fine in this case. Looking at the talos changes that were landed, it looks like placing the zip file in page_load_test is the correct action, can someone please confirm this? If its ok with you, I can file a seperate bug to move the mobile testing infrastructure to use the DownloadFile step. (In reply to comment #30) > Comment on attachment 462976 [details] [diff] [review] > Correct the mobiletestfactory.py changes > > Giving a tentative r+ here because this just seems to be putting back what was > taken out in the original patch. However, I don't understand the other patch > you asked bsmedberg for feedback on then because it seems to be fixing the same > thing in a different way. > > Is this the quick fix/reversion and the other is the new/proper way? The purpose of this patch is to make the buildbot-0.8.0 branch match what is running in production on the devices. The reason that I'd like this patch to land is that it means that the same patch to start using the xpi can be applied to the default and buildbot-0.8.0 branch which will keep them in sync. I would be ok with landing patch -R on default as neither version of the patch has any effect on what is running in production in any way. This patch and the original patch landed during the downtime have no effect on what is running on device. That function that adds the steps to run generate-tpcomponent is not called because of http://hg.mozilla.org/build/buildbotcustom/file/5a52ac1ad69f/process/mobiletestfactory.py#l196 . As running in production, self.pageloader_tarball is not None, so the alternate of downloading the tarball is run instead. This tarball is generated nightly using <snip> TOOLS_REV="default" <snip> (cd tools && hg pull && hg up -C -r $TOOLS_REV) <snip> python tools/buildfarm/utils/generate-tpcomponent.py <snip> tar cjvf pageloader-$DATESTRING.tar.bz2 chrome components <snip> (In reply to comment #31) > Comment on attachment 462976 [details] [diff] [review] > Correct the mobiletestfactory.py changes > > generate-tpcomponent doesn't make any sense (and should probably be removed) > any more: Talos won't use it, and even if you were using an old version of > talos it won't work as soon as I check in bug 579178, which is why we were > doing all this in the first place. I fully understand that generate-tpcomponent is invalid and doesn't make sense and should be removed. As the code is in the repository right now, and running on device in production, it is still using generate-tpcomponent.py. The purpose of the patch I asked for feedback on is to actually use the pageloader.xpi that we should start using in preparation of the landings breaking the pageloader we currently use.

summary of quick meeting with coop, aki and jhford just now. 1) bsmedberg's patch was landed in yesterday's downtime. 2) bsmedberg's patch works for talos desktop, but is not part of the mobile talos code path, so not visible to mobile talos infrastructure. We believe this would have been caught if the patch been reviewed by someone who understood the mobile code. 3) jhford has written a separate followup patch, for the missing mobile functionality and is working out reviews with bsmedberg. Assuming reviews, and staging runs go perfectly, this should be ready to put into production in downtime tomorrow.

This patch uses the new xpi file. I have verified that the xpi is downloaded and is allowing Talos to run again on the mobile phones. I am still using a straight ShellCommand with wget. Changing to DownloadFile can be filed as a separate bug if it is desired.

required buildbotcustom factory changes

Comment on attachment 463241 [details] [diff] [review] buildbot-configs v1 As mentioned in person, I disagree about the tarball removal. We can discuss offline. I've added the xpi to the tarball creation so this is effectively fixed for mobile Talos already; this patch isn't needed. I would r+ this if the patch allowed for optional tarball usage rather than removing it entirely.

Comment on attachment 463242 [details] [diff] [review] buildbotcustom v1 r- for tarball removal.

(In reply to comment #36) > Comment on attachment 463241 [details] [diff] [review] > buildbot-configs v1 > > As mentioned in person, I disagree about the tarball removal. We can discuss > offline. > > I've added the xpi to the tarball creation so this is effectively fixed for > mobile Talos already; this patch isn't needed. I would r+ this if the patch > allowed for optional tarball usage rather than removing it entirely. The case that you mentioned in person, fennecmark, is something that I feel like we should account for specifically. If a test doesn't use fennecmark, I don't think we should be downloading or installing it. My reasoning is that it increases the chance of a download being cut off midstream by the network and that our testing environment should not have extraneous libraries. What problem does having the option to download the pageloader and fennecmark in the same tarball solve? If I were to write a follow up patch that has a configuration option for where to get a fennecmark tarball/xpi from and install it on its own for tests that use fennecmark, would you be ok with this patch landing as is? When testing new pageloaders and/or fennecmark versions, we could set pageloader_url and fennecmark_url in the config file to point to an arbitrary url. Would that work for you?

I think we can mark this bug fixed: desktop talos was updated earlier and I've updated the pageloader tarball to put the pageloader.xpi in distribution/bundles on mobile talos runs. John Ford and I can take our disagreement over further changes offline or to a different, non-blocking bug.