431375 - try server needs to be able to handle non-Firefox builds

Status: RESOLVED WONTFIX

(Release Engineering :: General, defect, P3)

Description

[Dan Mosedale (:dmosedale, :dmose)]

My understanding is that the main thing that would be necessary would be to make the code able to upload Tb (and Lightning, ideally) bits to the server.

Comment 1

[David Ascher (:davida)]

Note: this is a bug about changes needed to the tryserver code to handle non-Fx builds.  There will be a separate bug about provisioning a VM host and configuring it for Tb.  

Comment 2

[[e-mail] (:bhearsum)]

I think the "right" way to fix this is to abstract away the "product" that is being tried. Currently, 'browser' or 'firefox' is hardcoded in a bunch of places. If we make this customizable, anything that follows the standard build process (checkout client.mk && make -f client.mk checkou && make -f client.mk build) should be able to built with the try server.

This is something that would be absolutely great. I personally don't have the time for it right now but patches are always accepted.


@David: For Tb3 you don't actually need separate slaves. Fx3 and Tb3 use the same toolchain so there isn't any need for separate slaves (unless I'm mistaken).

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ben, where the TryServer code is located? Would be interesting for someone who would like to help us here.

Comment 4

[[e-mail] (:bhearsum)]

The CGI script and change processing script are located here:
http://lxr.mozilla.org/mozilla/source/webtools/buildbot-try

The Buildbot configs are located here:
http://lxr.mozilla.org/mozilla/source/tools/buildbot-configs/tryserver/

I'm happy to give guidance to anyone working on this.

Comment 5

[[e-mail] (:bhearsum)]

While I was talking to Henrik about this a bit it occurred to me that non-firefox and non-thunderbird products may reap benefit from the try server as well. The "right way" to get Thunderbird support in the try server is to abstract away the product/app being built anyways, so I'm modifying the summary to match that.

Comment 6

[David Ascher (:davida)]

Attachment: Untested patch

work in progress (untested)

Comment 7

[David Ascher (:davida)]

bhearsum, any feedback on the patch would be welcome.  I'm happy to try it in the staging environment as soon as i get the right password setup.

I've deliberately not done the refactoring of function names away from firefox_... to make the patch easier to read.

I have also not tackled how to make PGK_BASENAME vary based on project, as I have some questions as to what the best approach is.

Comment 8

[David Ascher (:davida)]

Attachment: Better (still untested) patch

Incorporating some changes suggested by reed.

Still to do:
 
* get rid of first two lines of each of the mozconfigs, as they should be equivalent in the browser case to the two lines written by MozillaDownloadMozconfig

* figure out what to do about lightning

* rename the firefoxy function names in the master.cfg

* figure out if PKG_BASENAME really needs to be project-specific (it'd be simpler if not)

* test

Comment 9

[David Ascher (:davida)]

Attachment: Bester (untested) patch

forgot some basic HTML along the way.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 320116 [details] [diff] [review]
Bester (untested) patch

>+        <option value="necko">necko</option>
>+        <option value="tamarin">tamarin</option>
>+        <option value="composer">composer</option>
>+        <option value="toolkit">toolkit</option>

Are this real applications which can be build by using enable-application? If yes, we should probably update MDC: http://developer.mozilla.org/en/docs/Configuring_Build_Options#Choose_an_Application

Comment 11

[Dave Townsend [:mossop]]

Is there a reason for using a fixed list of projects as opposed to just letting the developer enter free-form?

Comment 12

[[e-mail] (:bhearsum)]

This looks really good. I won't have time to give it a full review until next week but David, ping me again today and I'll get you the passwords to the staging try server environment so you can test it out.(In reply to comment #11)

> Is there a reason for using a fixed list of projects as opposed to just letting
> the developer enter free-form?

I think the drop-down is a good idea if for nothing else than avoiding typos. I have no problem with the drop down plus an 'Other Project' field as well, though.


Todo: Make sure all of the platforms in the drop-down will build on the try server slaves. I'm pretty sure they're all on the same ref platform but we want to make sure try server builds aren't being built with a different gcc, or whatever, than the regular builds.

Comment 13

[David Ascher (:davida)]

My current thinking is that the dropdown should be trimmed for now to only the apps we know will work, and then have another entry which is "none (upload mozconfig!)" -- that way people can specify whatever mozconfig they want, whether that's for lightning, or any other upcoming project.  That way it'll be somewhat more futureproof.

@bhearsum: i'll find you on IRC.

Comment 14

[[e-mail] (:bhearsum)]

I like that better than having an extra text field, let's do it.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #13)
> mozconfig!)" -- that way people can specify whatever mozconfig they want,
> whether that's for lightning, or any other upcoming project.  That way it'll be
> somewhat more futureproof.

This idea is even more fantastic when looking at the possibility in being able to create a debug build, integrate the tests, or whatever is possible.

Comment 16

[Dave Townsend [:mossop]]

(In reply to comment #13)
> My current thinking is that the dropdown should be trimmed for now to only the
> apps we know will work, and then have another entry which is "none (upload
> mozconfig!)" -- that way people can specify whatever mozconfig they want,
> whether that's for lightning, or any other upcoming project.  That way it'll be
> somewhat more futureproof.

Ah excellent, that'll do nicely

Comment 17

[David Ascher (:davida)]

Attachment: Change to the Perl UI stuff

Comment 18

[David Ascher (:davida)]

Attachment: Patches to the tryserver config files

Comment 19

[David Ascher (:davida)]

Attachment: Patch to the buildbotcustom steps.py file

Cleaning up mozconfig handling, using more pythonic code.  Needs some battle testing.

Comment 20

[David Ascher (:davida)]

For those watching at home, my naive attempt at squeezing all the projects out of a single mozconfig was stillborn, so I'm doing something else which will have a default mozconfig per project per platform.  

Comment 21

[David Ascher (:davida)]

Attachment: Patch to processpchanges.pl

Comment 22

[David Ascher (:davida)]

Attachment: Patch to the buildbotcustom steps.py file

Comment 23

[David Ascher (:davida)]

Attachment: Patches to the tryserver config files

Comment 24

[David Ascher (:davida)]

Attachment: New mozconfigs for browser, mail, suite, calendar, lightning

Ok, the above set of patches should work, minus some of the commented out changes to master.cfg which are only staging server specific changes.

I've tested thunderbird and seamonkey patches on linux, it'd be good to run the other configurations through, but there is no staging mac slave (and I don't have a windows VM setup yet).

I haven't tested the Hg builds, or the layering of mozconfigs.

I'll review the TODO list, but I think this a review by bhearsum would be useful.

Comment 25

[David Ascher (:davida)]

Attachment: revised patch for processchanges.pl w/o debugging comment

Comment 26

[Justin Wood (:Callek)]

Comment on attachment 320269 [details] [diff] [review]
Change to the Perl UI stuff

> sub WritePage
> {
>     my %args = @_;
>     my $patchLevel = "0";
>+    my $project = "browser";
>     my $branch = "";
...
>+    if (exists $args{'project'} && $args{'project'} !~ /^\s*$/) {
>+        $project = $args{'project'};
>+    }
...
>+    <td class="lbl" id="projectfield"><label for="project">Project:</label></td>
>+    <td class="field">
>+      <select id="project" name="project">
>+        <option selected="selected" value="browser">browser (Firefox)</option>
>+        <option value="mail">mail (Thunderbird)</option>
>+        <option value="suite">suite (SeaMonkey)</option>
>+        <option value="calendar">calendar (Sunbird)</option>
>+        <option value="">none (must upload a mozconfig!)</option>

Note that you'll still set project as "browser" when you choose none.

Comment 27

[David Ascher (:davida)]

ah, nice catch Callek.  Yeah, I didn't test that codepath either =(

Comment 28

[Justin Wood (:Callek)]

Comment on attachment 320482 [details] [diff] [review]
Patches to the tryserver config files

>-    s(MozillaCreateUploadDirectory,
...
>-    s(MozillaUploadTryBuild,
...
>+#    s(MozillaCreateUploadDirectory,
...
>+#    s(MozillaUploadTryBuild,

Omission, or are these commented out on purpose? (if so, why?)

Comment 29

[David Ascher (:davida)]

Those are commented out on the try server, where i generated the patch.  they wouldn't go in to a real patch.

Comment 30

[[e-mail] (:bhearsum)]

Yeah, those are commented out on the staging try server since its builds don't get uploaded anywhere.

David, I probably can't review this today but I'll aim for tomorrow.

Comment 31

[[e-mail] (:bhearsum)]

Comment on attachment 320269 [details] [diff] [review]
Change to the Perl UI stuff

>+    <td class="lbl" id="projectfield"><label for="project">Project:</label></td>
>+    <td class="field">
>+      <select id="project" name="project">
>+        <option selected="selected" value="browser">browser (Firefox)</option>

WritePage can be called during form validation, we need to make sure to preserve whatever value was selected here. patchLevel works the same way (albeit with simpler data), you can probably do something similar to this:
http://lxr.mozilla.org/mozilla/source/webtools/buildbot-try/sendchange-ui.pm#292

>+        <option value="mail">mail (Thunderbird)</option>
>+        <option value="suite">suite (SeaMonkey)</option>
>+        <option value="calendar">calendar (Sunbird)</option>
>+        <option value="">none (must upload a mozconfig!)</option>

Nit: I think "other" is better than "none" here.

>+        <!-- not sure if these are supported enable-applications

They all look OK to me. devmo has MOZ_CO_PROJECT=mail,calendar here: http://developer.mozilla.org/en/docs/Configuring_Build_Options#Thunderbird.2C_Debugging_Build, but unless Lightning is enabled I don't think that matters. (More on that below.)

>+        .. and something for Lightning !-->

Lightning is a bit trickier...it requires modifications to MOZ_CO_PROJECT and --enable-applications...I think we can go two routes heres:
1) add "suite + calendar (Thunderbird w/ Lightning)" (and possibly a second entry for SeaMonkey).
2) Require that lightning builds have an uploaded mozconfig.

I imagine lightning builds are going to be more common as time passes so I think option #1 is preferable...what do you think?

Comment 32

[[e-mail] (:bhearsum)]

Comment on attachment 320481 [details] [diff] [review]
Patch to the buildbotcustom steps.py file

>Index: steps.py
>===================================================================
>RCS file: /cvsroot/mozilla/tools/buildbotcustom/tryserver/steps.py,v
>retrieving revision 1.2
>diff -u -8 -p -r1.2 steps.py
>--- steps.py	3 Jan 2008 21:52:50 -0000	1.2
>+++ steps.py	12 May 2008 01:00:55 -0000
>@@ -116,58 +116,45 @@ class MozillaDownloadMozconfig(FileDownl
>         self.patchDir = patchDir
>         # masterscr and slavedest get overridden in start()
>         FileDownload.__init__(self, mastersrc=mastersrc, slavedest=".mozconfig",
>                               **kwargs)
> 
>     def start(self):
>         changes = self.step_status.build.getChanges()
>         args = parseSendchangeArguments(changes[0].files)
>+        project = self.getProperty("project")

parseSendchangeArguments contains all of the arguments from the .info file, you can just use args['project']

>+        else:
>+            # use the browser one to have something.
>+            basemozconfig = os.path.join('mozconfigs', 'browser', self.mastersrc)

I think it's OK to require people to submit a full mozconfig if project is "other" (that's a condition that should be checked in sendchange.cgi, probably). So, in this case we should be able to just set self.mastersrc to the uploaded mozconfig and skip the rest of the logic (except FileDownload.start(self), obviously).

>+
>+        if not os.path.exists(basemozconfig): return SKIPPED
>+

If the base one doesn't exist (which we _should_ never hit) but there is an uploaded one let's try to fall back on it. If they both don't exist I'd consider than an error - it will most certainly cause the build to fail, so return FAILURE instead.

>+        newConfig = open(basemozconfig).read()
>+
>+        uploadedFile = path.join(self.patchDir, args['mozconfig'])
> 
>         # if we were passed in a mozconfig and also have an uploaded one
>         # they need to be combined, with the uploaded one overwriting any
>         # settings set by the passed in one
>+        if os.path.exists(uploadedFile):
>+            uploadedMozconfig = open(uploadedFile).read()
>+            newConfig += uploadedMozconfig
>+
>+        self.mastersrc = "%s-%s-%s" % (uploadedFile, project, 
>+                                       self.getProperty("slavename"))
>+
>+        # now write out the whole new thing
>+        mozconfig = open(self.mastersrc, "w")
>+        mozconfig.write(newConfig)
>+        mozconfig.close()

This section got simplified a lot....nice.


> class MozillaUploadTryBuild(ShellCommand):
>+        @type  filenameSuffix: string
>+        @param filenameSuffix: The filename (without the identifier) of the file

Nit: s/filename/filename suffix/

>                              that will be transferred
>         @type  scpString:  string
>         @param scpString:  The scp user@host:/dir string to upload the file to.
>                            For example,
>                              [e-mail]:/var/www.
>                            This user should have passwordless access to the
>                            host.
>         """
>         self.slavedir = slavedir
>-        self.baseFilename = baseFilename
>+        self.filenameSuffix = filenameSuffix
>         self.scpString = scpString
> 
>         ShellCommand.__init__(self, **kwargs)
> 
>     def start(self):
>         # we need to append some additional information to the package name
>         # to make sure we don't overwrite any existing packages
>         changes = self.step_status.build.getChanges()
>         args = parseSendchangeArguments(changes[0].files)
>         # the REMOTE_USER from the submission form
>         changer = changes[0].who
>         # the time the change was processed, this should be the same for every
>         # build in a set
>         when = strftime("%Y-%m-%d_%H:%M", localtime(changes[0].when))
>         dir = "%s-%s-%s" % (when, changer, args['identifier'])
>         # this is the filename of the package built on the slave
>-        filename = "%s-%s" % (args['identifier'], self.baseFilename)
>+        filename = "%s-%s-try-%s" % (args['identifier'], args(['project']),

The final names will look a little different now, eg. 'browser' instead of 'firefox' - but I'm OK with that behaviour change.


r- because of the DownloadMozconfig comments, the rest looks good.

Comment 33

[[e-mail] (:bhearsum)]

Comment on attachment 320483 [details]
New mozconfigs for browser, mail, suite, calendar, lightning

I was going to make some review comments here but I think it's just easier if I attach a patch...incoming shortly.

Comment 34

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #31)
> Lightning is a bit trickier...it requires modifications to MOZ_CO_PROJECT and
> --enable-applications...I think we can go two routes heres:
> 1) add "suite + calendar (Thunderbird w/ Lightning)" (and possibly a second
> entry for SeaMonkey).
> 2) Require that lightning builds have an uploaded mozconfig.
> 
> I imagine lightning builds are going to be more common as time passes so I
> think option #1 is preferable...what do you think?
To build lightning, its also possible to MOZ_CO_PROJECT=calendar and --enable-application=calendar. Theoretically we don't even need calendar to build lightning, but since the tree doesn't allow us to not build an application, we need something.

The important part would be to be able to upload the extension from dist/xpi-stage to the ftp server.

I'm not sure how things work out, but my suggestion would be to provide a "calendar (Sunbird and Lightning)" option that has MOZ_CO_PROJECT=calendar and --enable-application=calendar and --enable-extensions=lightning, and uploads both the respective Sunbird packages and the respective Lightning packages.

This would also allow a one-stop patching. When we globally install lightning with thunderbird, then a "mail + calendar" option would also be nice, I guess.

Comment 35

[[e-mail] (:bhearsum)]

Attachment: updated try server mozconfigs

Mostly, I just synced up with the nightly mozconfig files (http://lxr.mozilla.org/mozilla/source/tools/tinderbox-configs/). Here's a summary of the changes
* Switch to simply '--enable-optimize' for all platforms.
* Make sure -gstabs+ is on for all Linux configs.
* Remove --disable-pedantic lines from Linux configs.


I really really hate having so many mozconfigs, but solving bug 419687 (need a better way to manage tryserver mozconfigs) is way out of scope here.


I haven't touched the lightning configs. What we do with them depends on how we handle "other" projects. I don't _think_ we'll need a separate directory for them, but let's see.

Comment 36

[[e-mail] (:bhearsum)]

(In reply to comment #34)
 as time passes so I
> > think option #1 is preferable...what do you think?
> To build lightning, its also possible to MOZ_CO_PROJECT=calendar and
> --enable-application=calendar. Theoretically we don't even need calendar to
> build lightning, but since the tree doesn't allow us to not build an
> application, we need something.
> 

I'm not sure what you mean...if we're building Thunderbird we can build Lightning as an extension and it will end up in the Thunderbird package, right?

> The important part would be to be able to upload the extension from
> dist/xpi-stage to the ftp server.
> 

I'm hoping this will not be necessary (see a few lines up). The try server was never intended to build/upload extensions...That's not to say it shouldn't be made to, but it's more difficult than just adding "Tb+Lightning".

> I'm not sure how things work out, but my suggestion would be to provide a
> "calendar (Sunbird and Lightning)" option that has MOZ_CO_PROJECT=calendar and
> --enable-application=calendar and --enable-extensions=lightning, and uploads
> both the respective Sunbird packages and the respective Lightning packages.
> 
> This would also allow a one-stop patching. When we globally install lightning
> with thunderbird, then a "mail + calendar" option would also be nice, I guess.
> 

This part is lost on me. I may not understand how Lightning is packaged with/fits into Thunderbird, though.

Comment 37

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #36)
> I'm not sure what you mean...if we're building Thunderbird we can build
> Lightning as an extension and it will end up in the Thunderbird package, right?
I always assumed this was not the case, but you are right, that was fixed with bug 349870.

> I'm hoping this will not be necessary (see a few lines up). The try server was
> never intended to build/upload extensions...That's not to say it shouldn't be
> made to, but it's more difficult than just adding "Tb+Lightning".
I see what you mean, I guess I'll leave that for a different bug and when its really needed.

> This part is lost on me. I may not understand how Lightning is packaged
> with/fits into Thunderbird, though.
My suggestion depends on the possibility to also upload extensions, so go ahead and disregard. Just in case you are still interested in what I mean, you can build sunbird and lightning together, without packaging lightning inside sunbird. You will have the calendar package built in dist/bin and lightning packaged separately in dist/xpi-stage. This way you don't have to build mail to get a lightning.xpi.

Comment 38

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

> +ac_add_app_options ppc --enable-prebinding

Prebinding is useless and should be removed from all mozconfigs, see bug 407399.

It looks like all of the other mozilla.org apps except Camino are getting specific, "built-in" support here?

Comment 39

[[e-mail] (:bhearsum)]

I've got no problem adding Camino here too. Is its build+packaging process the same as Fx/Tb/etc?

Comment 40

[David Ascher (:davida)]

I was planning on adding Camino after I figured out how to make things fail gracefully for those machines for which it didn't make sense to build, but that may be unnecessary.

re: Lightning: figuring out a way to make Tryserver build Thunderbird w/ enabled Lightning on trunk is going to be quite important for us (MoMo) soon.  I don't actually think it's hard, as long as the package ends up being a modified Tb (with Lightning pre-installed) rather than a standalone xpi.

Figuring out how to build lightning on branch I could see being a bit too hard for the current architecture.

Comment 41

[[e-mail] (:bhearsum)]

The try server doesn't actually work on MOZILLA_1_8_BRANCH anyways, because of toolchain differences.

Comment 42

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #39)
> I've got no problem adding Camino here too. Is its build+packaging process the
> same as Fx/Tb/etc?

We pass different values in in various places, but it's all driven by client.mk, $appname/installer/Makefile.in, and the various other Makefiles like all the other apps. I can't think of anything off the top of my head that would be fundamentally incompatible with shared build automation.

(In reply to comment #40)
> I was planning on adding Camino after I figured out how to make things fail
> gracefully for those machines for which it didn't make sense to build, but that
> may be unnecessary.

Yeah, preventing Win/Lin builds would probably be the biggest hang-up I can think of, too.

Comment 43

[[e-mail] (:bhearsum)]

(In reply to comment #42)
> (In reply to comment #39)
> > I've got no problem adding Camino here too. Is its build+packaging process the
> > same as Fx/Tb/etc?
> 
> We pass different values in in various places, but it's all driven by
> client.mk, $appname/installer/Makefile.in, and the various other Makefiles like
> all the other apps. I can't think of anything off the top of my head that would
> be fundamentally incompatible with shared build automation.
> 
> (In reply to comment #40)
> > I was planning on adding Camino after I figured out how to make things fail
> > gracefully for those machines for which it didn't make sense to build, but that
> > may be unnecessary.
> 
> Yeah, preventing Win/Lin builds would probably be the biggest hang-up I can
> think of, too.
> 

That's a good point. I would love it if we could solve this as part of bug 391364 (try server should have the option of only building specified platforms). I'd be open to a short term solution too, though.

Comment 44

[[e-mail] (:bhearsum)]

Comment on attachment 320484 [details] [diff] [review]
revised patch for processchanges.pl w/o debugging comment

This is fine, but I think we'll also need project for hg stuff. I think you're tackling that later though, so r+ for now.

Comment 45

[[e-mail] (:bhearsum)]

Comment on attachment 320482 [details] [diff] [review]
Patches to the tryserver config files

I'm going to put off reviewing this until you're done with the rest of the files. I don't expect the master.cfg to be complicated at all though.

Comment 46

[[e-mail] (:bhearsum)]

Hey David, how's this going?

Comment 47

[David Ascher (:davida)]

I haven't had any time to finish the patches.  There's no real stumbling block, except for my ability to find a little chunk of time to work on it.  If you felt like finishing it, that's just fine by me.  Otherwise i'll likely get to it not next week but the week after that.

Comment 48

[[e-mail] (:bhearsum)]

No rush, just wanted to make sure the bug was still accurate. Thanks again for doing this!

Comment 49

[David Ascher (:davida)]

Note: I'm not finding any time to finish this.  anyone who wants to pick up the ball is more than welcome to.

Comment 50

[Philipp Kewisch [:Fallen][📱📆]]

David, is there anything you have outside of this patch? I'll possibly take a look at this, I'd like to be able to use try-server for lightning/sunbird builds too.

Comment 51

[Justin Wood (:Callek)]

I just spoke to him on IRC a few days back about this, he has nothing beyond whats in here...

And ideally we can abandon the CVS variant here and just go with Hg (as CVS is dead as far as try-server needs are concerned)

Comment 52

[Philippe M. Chiasson (:gozer)]

Attachment: Changes to the try-server front-end

based on all existing comments, I've made the required changes to the try-server front-end stuff. CGI and processchanges.pl.  With this changes, I believe project support would be complete on the front-end, and in a forward-compatible matter.

This could be pushed live to the try-server and not break anything. Of course, until buildbot is patched as well, builds would always be of browser/firefox.

In any case, feedback welcome. The buildbot config changes will be coming soon.

Comment 53

[Philippe M. Chiasson (:gozer)]

Attachment: Changes to buildbot config and tryserver.steps for non-Firefox builds

This is a rather large patch, but it contains all that has been discussed so far and a little more. I've tested this on my own setup with a Linux builder, and it's able to run both Firefox and Thunderbird builds successfully.

Overall, the idea is to front-process stuff in MozillaTryProcessing and use build properties to track everything that is potentially project-specific. Then the rest of the build logic can stay the same and just use WithProperties() and the like to grab things. Look at how distdir is changed when building the mail project for an example.

The only non included change is a not quite necessary change to BuildSlaves.py (not in source control I could find), to make platform-specific changes/logic much easier. The idea is to use BuildSlave properties to keep track of the platform of a given builder. Then it's easy to do things like:

if self.getProperties('platform') == 'win32':
  ...

For now, I've only done this for the Mac OS X distdir being different (objdir/ppc/dist) than on all the other platforms.

But looking at all the cut-n-paste where the only difference is 'linux' or '.tar.bz2' vs '.zip', it could be cleaned up further.

if self.getProperties('platform') == 'linux':
  self.setProperty('package_extension','.tar.bz2')

For example.

Here is a sample from my BuildSlaves.py illustrating this:

SlaveList = [
    BuildSlave("builder1", "pwd", properties = { 'platform': 'linux' } ),
    BuildSlave("builder2", "pwd", properties = { 'platform': 'mac' } ),
    BuildSlave("builder3", "pwd", properties = { 'platform': 'win32' } ),

Comment 54

[[e-mail] (:bhearsum)]

Comment on attachment 367239 [details] [diff] [review]
Changes to the try-server front-end

This patch seems completely fine...I'd like to see it running somewhere, though. Do you have it on a webserver somewhere?

Comment 55

[[e-mail] (:bhearsum)]

Comment on attachment 368088 [details] [diff] [review]
Changes to buildbot config and tryserver.steps for non-Firefox builds

>diff --git a/tryserver/steps.py b/tryserver/steps.py
> class MozillaTryProcessing(BuildStep):
>     warnOnFailure = True
>     name = "try server pre-processing"
> 
>     """This step does some preprocessing that the try server needs.
>        1) Resubmits any extra changes attached to this Build.
>        2) Sets all of the sendchange arguments as build properties
>-       3) Provides a short header to tho build to help easily identify it
>+       3) Sets up any project-specific build properties
>+       4) Provides a short header to the build to help easily identify it
>     """
>+    def __init__(self, defaultProject, objdir, **kwargs):

Why no default to defaultProject?

> 
>+    # Convenient setProprety wrapper that labels all properties we set
>+    def setProp(self, name, value):
>+        self.setProperty(name, value, "MozillaTryProcessing")

I think it would be better to override setProperty and call self.super.setProperty or BuildStep.setProperty here

>+
>+    def setDefaultTryProperties(self):
>+
>+        # hg.mozilla.org/try pushes will not have a project
>+        try:
>+            project = self.getProperty('project')
>+        except KeyError:
>+            project = self.defaultProject
>+            self.setProp('project', self.defaultProject)
>+        except:
>+            raise

You shouldn't raise in runtime code. Instead, use self.finished(FAILED) && return FAILED to indicate an error. This way, Buildbot will turn the build red instead of purple.

>+# This is a step needed by comm-central builds, set the property
>+# run_client_py to True not to skip over this step. (default skips)
>+class MozillaRunClientPy(ShellCommand):
>+    def __init__(self, **kwargs):
>+        kwargs['description'] = "python client.py"
>+        kwargs['descriptionDone'] =  "python client.py"
>+        kwargs['name'] = "run client.py"
>+        kwargs['command'] = ['python', 'client.py']

Please define this as class level args for consistency.


>@@ -125,58 +210,49 @@ class MozillaDownloadMozconfig(FileDownl
<snip>
>         # everything is set up, download the file
>         FileDownload.start(self)
> 

I want to see this in action....but it looks correct. Thank you for simplifying the hell out of it.

> 
> class MozillaPatchDownload(FileDownload):
>     """This step reads a Change for a filename and downloads it to the slave.
>     It is typically used in conjunction with the MozillaCustomPatch step.
>@@ -221,52 +297,63 @@ class MozillaPatchDownload(FileDownload)
>         # now that everything is set-up, download the file
>         FileDownload.start(self)
> 
> 
> 
> class MozillaUploadTryBuild(ShellCommand):
>     warnOnFailure = True
> 
>-    def __init__(self, slavedir, baseFilename, scpString, **kwargs):
>+    def __init__(self, slavedir, distdir, filenameSuffix, scpString, **kwargs):

We already need to pass objdir to at least one other step, right? Can we just pass that here rather than adding an additional variable?


>+    #This is needed for comm-central builds, skipped otherwise
>+    s(MozillaRunClientPy, name="run client.py",

Any reason you can't define name/halt/flunk/timeout in steps.py?


r- specifically for the raise instead of return FAILED, but I'd like to the see other nits fixed as well unless you have strong objections. You also need to attach default mozconfig's for all of the defined projects

Overall, I'm very happy with these patches. I'd like to get it a run in our staging environment once all the nits are fixed to see it in action.

One more note....we're experiencing a bit of a load crunch on the try server right now...I don't want to block deploying this on that being fixed, but it might delay things a bit.

Comment 56

[Philippe M. Chiasson (:gozer)]

(In reply to comment #54)
> (From update of attachment 367239 [details] [diff] [review])
> This patch seems completely fine...I'd like to see it running somewhere,
> though. Do you have it on a webserver somewhere?

Yes I do, it's running live right now, I can get you access. Grab me on IRC and I can get you what you need to see it (a slightly tweaked version that puts Thunderbird/comm-central as the default)

Comment 57

[Philippe M. Chiasson (:gozer)]

(In reply to comment #55)
> (From update of attachment 368088 [details] [diff] [review])
> >diff --git a/tryserver/steps.py b/tryserver/steps.py
> > class MozillaTryProcessing(BuildStep):
> >     warnOnFailure = True
> >     name = "try server pre-processing"
> > 
> >     """This step does some preprocessing that the try server needs.
> >        1) Resubmits any extra changes attached to this Build.
> >        2) Sets all of the sendchange arguments as build properties
> >-       3) Provides a short header to tho build to help easily identify it
> >+       3) Sets up any project-specific build properties
> >+       4) Provides a short header to the build to help easily identify it
> >     """
> >+    def __init__(self, defaultProject, objdir, **kwargs):
> 
> Why no default to defaultProject?

No reason, I guess defaultProject="browser" makes sense here ?

> > 
> >+    # Convenient setProprety wrapper that labels all properties we set
> >+    def setProp(self, name, value):
> >+        self.setProperty(name, value, "MozillaTryProcessing")
> 
> I think it would be better to override setProperty and call
> self.super.setProperty or BuildStep.setProperty here

Awesome, didn't think of doing it that way, makes a lot more sense.

> >+
> >+    def setDefaultTryProperties(self):
> >+
> >+        # hg.mozilla.org/try pushes will not have a project
> >+        try:
> >+            project = self.getProperty('project')
> >+        except KeyError:
> >+            project = self.defaultProject
> >+            self.setProp('project', self.defaultProject)
> >+        except:
> >+            raise
> 
> You shouldn't raise in runtime code. Instead, use self.finished(FAILED) &&
> return FAILED to indicate an error. This way, Buildbot will turn the build red
> instead of purple.

If I do that, the only thing I am worried about is loosing the exception itself.
Will this return FAILED somehow capture the exception that was thrown ?

> >+# This is a step needed by comm-central builds, set the property
> >+# run_client_py to True not to skip over this step. (default skips)
> >+class MozillaRunClientPy(ShellCommand):
> >+    def __init__(self, **kwargs):
> >+        kwargs['description'] = "python client.py"
> >+        kwargs['descriptionDone'] =  "python client.py"
> >+        kwargs['name'] = "run client.py"
> >+        kwargs['command'] = ['python', 'client.py']
> 
> Please define this as class level args for consistency.

Not sure what you mean by that ? this ? :

def __init__(self, **kwargs):
        ShellCommand.__init__(self, 
                              description = "python client.py",
                              descriptionDone = "python client.py",

> [...]
> > class MozillaUploadTryBuild(ShellCommand):
> >     warnOnFailure = True
> > 
> >-    def __init__(self, slavedir, baseFilename, scpString, **kwargs):
> >+    def __init__(self, slavedir, distdir, filenameSuffix, scpString, **kwargs):
> 
> We already need to pass objdir to at least one other step, right? Can we just
> pass that here rather than adding an additional variable?

Actually, this made me realize that after MozillaTryProcessing, there is always
going to be a distdir build property set, so I can just look it up there instead of requiring another argument.

> >+    #This is needed for comm-central builds, skipped otherwise
> >+    s(MozillaRunClientPy, name="run client.py",
> 
> Any reason you can't define name/halt/flunk/timeout in steps.py?

Nope, no reason at all, I'll tidy that up

> r- specifically for the raise instead of return FAILED, but I'd like to the see
> other nits fixed as well unless you have strong objections. You also need to
> attach default mozconfig's for all of the defined projects

Yes, I agree with all your nits, expect a cleaned up patch soon.

As for the mozconfigs, I have punted on doing them, waiting for these patches to get reviewed. Getting nice/clean mozconfigs will be next.

> Overall, I'm very happy with these patches. I'd like to get it a run in our
> staging environment once all the nits are fixed to see it in action.

Cool, I could even set it up myself, if I have sufficient privileges to get to
staging.

Comment 58

[Philippe M. Chiasson (:gozer)]

Attachment: Changes tryserver.steps for non-Firefox builds v2

Splitting the master.cfg patch and tryserver.steps patch to make them easier to review.

This new patch to tryserver.steps addresses, I believe, all the good feedback in comment #55

Comment 59

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

David, Dan, Philippe; 

Whats the plan for handling the extra workload this will add to TryServer? 

We're already having fun just getting TryServer to keep up with the current
volume of Firefox builds/unittests/talos, and I'm worried about how to handle
the extra load from Thunderbird and other community try builds.

(If this was already discussed somewhere and I missed it, sorry, just point me
to that.)

Comment 60

[David Ascher (:davida)]

I think it's important to dissociate what the code is able to do (IMO, this bug), and what MoCo's tryserver farm is tasked with.

MoMo is responsible (IMO) for running the tryservers for Thunderbird (and we're happy to host Seamonkey tryservers as well).  I'm happy to consider non-Tb-Sm try builds as well.  

I don't think MoCo's tryserver farm needs to worry about these builds.

Gozer will correct me when I stray, but my understanding is that we're in the later stages of testing our independent tryserver & builder farm, off of https://build.mozillamessaging.com/try, with a client cert authentication scheme.

It feels like maybe something we need is the ability for the code to have a config file that says what products a particular tryserver instance should be building. (Maybe the code already has that, I haven't looked at gozer's patches).  MoCo could opt-in to Firefox only, and MoMo to a different set.

FYI, we're also working on backing our tryserver with EC2 instances (for Windows/Linux).  When that's working, it might be useful for you folks to deal with backlog or whatever.

Comment 61

[Philippe M. Chiasson (:gozer)]

(In reply to comment #60)
> I think it's important to dissociate what the code is able to do (IMO, this
> bug), and what MoCo's tryserver farm is tasked with.

Yes!

> MoMo is responsible (IMO) for running the tryservers for Thunderbird (and we're
> happy to host Seamonkey tryservers as well).  I'm happy to consider non-Tb-Sm
> try builds as well.  

Yup, for now, we are still getting it 100% up and working, but that's the plan.

> I don't think MoCo's tryserver farm needs to worry about these builds.

No, I don't think so.

> Gozer will correct me when I stray, but my understanding is that we're in the
> later stages of testing our independent tryserver & builder farm, off of
> https://build.mozillamessaging.com/try, with a client cert authentication
> scheme.

Yes, that's correct.

> It feels like maybe something we need is the ability for the code to have a
> config file that says what products a particular tryserver instance should be
> building. (Maybe the code already has that, I haven't looked at gozer's
> patches).  MoCo could opt-in to Firefox only, and MoMo to a different set.

Yeah, sortof. It's configured in some top-level global variables, but it probably be made proprely configureable, not very hard.

> FYI, we're also working on backing our tryserver with EC2 instances (for
> Windows/Linux).  When that's working, it might be useful for you folks to deal
> with backlog or whatever.

Yup, with these and buildbot's LatentBuilders, you can have the best of both worlds, and have buildbot fire off additionnal instances to pick up the load when the pending queue grows.

Building the Amazon EC2 AMIs has been annoying, but I can share them with you guys easily, just let me know.

Comment 62

[[e-mail] (:bhearsum)]

(In reply to comment #57)
> (In reply to comment #55)
> > Why no default to defaultProject?
> 
> No reason, I guess defaultProject="browser" makes sense here ?
> 

Yeah. The reason I want a default is so we don't end up with 'project' set to None if 'project' isn't passed to MozillaTryProcessing nor if 'project' isn't already set.

> > > 
> > >+    # Convenient setProprety wrapper that labels all properties we set
> > >+    def setProp(self, name, value):
> > >+        self.setProperty(name, value, "MozillaTryProcessing")
> > 
> > I think it would be better to override setProperty and call
> > self.super.setProperty or BuildStep.setProperty here
> 
> Awesome, didn't think of doing it that way, makes a lot more sense.
> 
> > >+
> > >+    def setDefaultTryProperties(self):
> > >+
> > >+        # hg.mozilla.org/try pushes will not have a project
> > >+        try:
> > >+            project = self.getProperty('project')
> > >+        except KeyError:
> > >+            project = self.defaultProject
> > >+            self.setProp('project', self.defaultProject)
> > >+        except:
> > >+            raise
> > 
> > You shouldn't raise in runtime code. Instead, use self.finished(FAILED) &&
> > return FAILED to indicate an error. This way, Buildbot will turn the build red
> > instead of purple.
> 
> If I do that, the only thing I am worried about is loosing the exception
> itself.
> Will this return FAILED somehow capture the exception that was thrown ?
> 

If you're concerned about that you can use self.addCompleteLog to print the exception in. Eg;
try:
...
except KeyError:
...
except:
    msg = 'Caught error: ' % sys.exc_info()[0]
    self.addCompleteLog('error', msg)

> > >+# This is a step needed by comm-central builds, set the property
> > >+# run_client_py to True not to skip over this step. (default skips)
> > >+class MozillaRunClientPy(ShellCommand):
> > >+    def __init__(self, **kwargs):
> > >+        kwargs['description'] = "python client.py"
> > >+        kwargs['descriptionDone'] =  "python client.py"
> > >+        kwargs['name'] = "run client.py"
> > >+        kwargs['command'] = ['python', 'client.py']
> > 
> > Please define this as class level args for consistency.
> 
> Not sure what you mean by that ? this ? :
> 

Define them like this instead:
class MozillaRunClientPy(ShellCommand):
    description = "python client.py"
    descriptionDone = "python client.py"
    etc.

You can probably get rid of the __init__ method altogether by doing this.

Comment 63

[[e-mail] (:bhearsum)]

Oh, are there any updates you need to do to the buildbot-configs side of things?

Comment 64

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to comment #61)
> (In reply to comment #60)
> > I think it's important to dissociate what the code is able to do (IMO, this
> > bug), and what MoCo's tryserver farm is tasked with.
> Yes!
> > MoMo is responsible (IMO) for running the tryservers for Thunderbird (and we're
> > happy to host Seamonkey tryservers as well).  I'm happy to consider non-Tb-Sm
> > try builds as well.  
> Yup, for now, we are still getting it 100% up and working, but that's the plan.
> 
> > I don't think MoCo's tryserver farm needs to worry about these builds.
> No, I don't think so.
> 
> > Gozer will correct me when I stray, but my understanding is that we're in the
> > later stages of testing our independent tryserver & builder farm, off of
> > https://build.mozillamessaging.com/try, with a client cert authentication
> > scheme.
> 
> Yes, that's correct.


Ah, cool. Sharing one code base for all this is very cool. I was concerned we were also talking about sharing one TryServer instance, hence my "how much load" questions. Thanks for clarifying.

Comment 65

[Philippe M. Chiasson (:gozer)]

(In reply to comment #63)
> Oh, are there any updates you need to do to the buildbot-configs side of
> things?

Needed, not really, what's there is working. However, they need to be cleaned up and synced to what's current in buildbot-config/ as there were still CVS based builds, when davida originally created them.

I'll post updated versions.

Comment 66

[[e-mail] (:bhearsum)]

Comment on attachment 372076 [details] [diff] [review]
Changes tryserver.steps for non-Firefox builds v2

Removing review request as per comment #65

Comment 67

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Found this during cleanup - its been over a year since the last comment. Anything still to do here?

Comment 68

[[e-mail] (:bhearsum)]

Nothing ever landed here, so unless it's WONTFIX it's still a valid bug. MoMo runs their on Try Server though, so their need of it is gone AFAIK. Not sure about SeaMonkey.

Comment 69

[Robert Kaiser]

We're using the MoMo try server right now, as gozer was friendly enough to give those access who want it.
I wonder if the code changes he did there are public and/or merged into the more general try code, though.

Comment 70

[Philippe M. Chiasson (:gozer)]

(In reply to comment #69)
> We're using the MoMo try server right now, as gozer was friendly enough to give
> those access who want it.
> I wonder if the code changes he did there are public and/or merged into the
> more general try code, though.

Nope, they are still made up of a bunch of unclean local changes on our try master. See bug 532625 for the status on cleaning that up and making it public.

It should certainly be upstreamed, IMO, but the current solution just isn't quite generic enough.

Comment 71

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

I'm closing this as WONTFIX because:

1) The MoMo tryserver seems to be useful for gozer and kairo, so that original need looks like it is handled.

2) The cleanup work on gozer's patches to make tryserver code more generic is being tracked separately in bug#532625, not here. 

...so I think this bug is all done. If I'm missing something, please reopen with details.

Comment 72

[Justin Wood (:Callek)]

(In reply to comment #71)
> I'm closing this as WONTFIX because:
> 
> 1) The MoMo tryserver seems to be useful for gozer and kairo, so that original
> need looks like it is handled.

Useful yes [for me too] but it would be better if "push to try" worked as well ;-)

> 2) The cleanup work on gozer's patches to make tryserver code more generic is
> being tracked separately in bug#532625, not here. 

Given this, I'm not opposed to having this bug closed out; but I do forsee a big benefit from allowing the features of this bug (per summary) exposed on official try.

[e.g. XULRunner, comm-central based builds, Fennec; etc.]