The default bug view has changed. See this FAQ.

Clobber support for mozilla-central depend builds and unit tests

RESOLVED FIXED

Status

Release Engineering
Other
P3
normal
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: bsmedberg, Assigned: catlee)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 12 obsolete attachments)

2.08 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
2.16 KB, patch
nthomas
: review+
Details | Diff | Splinter Review
5.48 KB, patch
nthomas
: review+
Details | Diff | Splinter Review
4.82 KB, patch
bhearsum
: review+
catlee
: checked-in+
Details | Diff | Splinter Review
2.54 KB, patch
bhearsum
: review+
catlee
: checked-in+
Details | Diff | Splinter Review
69.49 KB, patch
bhearsum
: review+
catlee
: checked-in+
Details | Diff | Splinter Review
4.73 KB, patch
bhearsum
: review+
catlee
: checked-in+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
I'm going to file this together, though it might end up being separate:

Every time we merge a new NSPR into mozilla-central, various boxes go red, including the unit-test boxes and at least some of the depend build machines. In this particular case it's because of autoconf differences leading to CVS conflicts, but there are other reasons we regularly may need to clobber these builds.

Ideally what I'd like to see is a form on the buildbot waterfall or someplace similar that lets the sheriff a clobber, but I've heard that's hard... doing something from a checkin like with do with CVS clobber files is ok too.

Note that if we do the clobber-file thing, it should trigger a new build immediately, instead of waiting for the next checkin.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 431529
(Reporter)

Comment 2

9 years ago
Reopening: this bug will be about the depend builders, and bug 431529 will be about the unit-test boxes, which are on a different master.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Blocks: 433384
Priority: -- → P3
(Reporter)

Comment 3

9 years ago
Created attachment 322851 [details] [diff] [review]
Rough clobber support

This is a rough draft of clobber support via an external webtool that doesn't require LDAP/MPT access.
I don't really like this method but I've been racking my brain all week trying to come up with a better way, and I can't find one. Writing a proper Buildbot patch that integrates with Waterfall, can clobber all slaves when requested is a hard task. Therefore I'm in favour of this approach, especially given the false-positive performance regression.

I think we need to put the actual form behind ldap, probably on build.mozilla.org (all hg users have an LDAP account anyways, so this shouldn't unnecessarily restrict anyone).

I think it makes sense to have it do periodic clobbers too. Either clobbering if there hasn't been one for X hours (24 hours, maybe?), or clobbering when time hits X hour of the day (2am? *shrug).

Benjamin, if you can write the rest of the patch I'd be happy to test it on all platforms for you.
+1 to bhearsum's comments.

I was thinking about periodic clobbers this weekend as well. We could just add a separate Clobber buildstep inside a Periodic scheduler that does the business of wiping out a tree followed by a build.
I'll finish this up this week.
Assignee: nobody → bhearsum
Status: REOPENED → NEW
Depends on: 445309
Created attachment 329822 [details] [diff] [review]
[checked in] ugly, basic front-end for clobbering

This script does the same thing as yours, Benjamin, but it also records the name of the person that clobbers (working under the assumption we'll require LDAP to do it). I added short_name/long_name because PHP mangles <input name="..."> when there's special characters (such as spaces or dots). Because of this, I removed the ability to automatically add a new tree by simply visiting .../index.php?tree=...
Attachment #322851 - Attachment is obsolete: true
Attachment #329822 - Flags: review?(benjamin)
Created attachment 329824 [details] [diff] [review]
clobber buildstep - forced clobbers + periodic

Here's a BuildStep that reads polls the PHP script for forced clobbers, and also does periodic ones. I think the code is pretty straight forward...

It's set to clobber if there hasn't been one for 24 hours. This means that it *could* happen in the middle of the day. If that's not a great idea we can change it up.

One thing I don't really like is that slaves may only get one build of a tree before clobbering...eg, if moz2-win32-slave02 builds mozilla-central today, but then doesn't get another build of that until friday, it seems silly that it will clobber. To minimize this, I think it might be useful to bump up the periodic clobber time to a few days, a week, maybe longer if that's OK. Looking for opinions here (ted/bsmedberg?).

Forced clobbers have a similar problem in that it's possible that they don't build a tree that a clobber was forced on for a day or two. While it's good that they *do* clobber ASAP after the request, it could be confusing that it takes a day or two to happen.

I've tested this on a local Buildbot and it's working perfectly. I'll be running tests on our staging setup shortly.
Attachment #329824 - Flags: review?(ccooper)
Comment on attachment 329824 [details] [diff] [review]
clobber buildstep - forced clobbers + periodic

Hrm. Coop is out this week, moving the review...
Attachment #329824 - Flags: review?(ccooper) → review?(nthomas)
Comment on attachment 329824 [details] [diff] [review]
clobber buildstep - forced clobbers + periodic

A few questions, all clues welcome.

>Index: misc.py
>===================================================================
...
>+        if 'command' not in kwargs:
>+            kwargs['command'] = ["python", "-c", """
>+import sys, shutil, urllib2, urllib, os
>+from datetime import datetime, timedelta
>+ONE_DAY = timedelta(days=1)

Seems to be quite a lot of inline code here, what was the decision process there ? Could it be stashed in the build system somewhere, so that it's already on the slave ? Or does that create a chicken and egg problem on the first build ? Would MozillaClobber be the first addStep in the Factory, and therefore be before the hg pull ?

...
>+cur_force_date = urllib2.urlopen(url).read()

What happens if the information can't be retrieved ? eg the site is down or returning bogus info. Would it be worth checking for sane input here before trampling on the state files ? 

And do you think we'd want to return an error and prevent builds if url is down ? Probably not, but I guess there needs to be something relatively noisy if clobbers are failing. Perhaps a TinderboxPrint, if you can get to the log from here.

>+  print "Checking clobber URL: %s" % url
>+  print "Current clobber date: %s" % cur_force_date

Would be nice to move these up to where it really happens (either side of the urlopen call).

>+  if (last_clobber + ONE_DAY < cur_date):
>+    print "More than 24 hours have passed since the last clobber, clobbering " \
>+          "build directory"
>+    do_clobber()

I'm a bit concerned about this, given we could end up with N slow builds a day for N connected slaves (assuming there are enough jobs/slave for randomness to do it's thing when they're allocated). Backing it off to once a week would help, particularly if we can stagger the clobbers over that period. (I'd really love if we could send the tinderbox server some TinderPrints as part of the build start email, including the source revision and clobber, then it's clear to all why a build is slow and what it's building.)
(In reply to comment #10)
> (From update of attachment 329824 [details] [diff] [review])
> A few questions, all clues welcome.
> 
> >Index: misc.py
> >===================================================================
> ...
> >+        if 'command' not in kwargs:
> >+            kwargs['command'] = ["python", "-c", """
> >+import sys, shutil, urllib2, urllib, os
> >+from datetime import datetime, timedelta
> >+ONE_DAY = timedelta(days=1)
> 
> Seems to be quite a lot of inline code here, what was the decision process
> there ? Could it be stashed in the build system somewhere, so that it's already
> on the slave ? Or does that create a chicken and egg problem on the first build
> ? Would MozillaClobber be the first addStep in the Factory, and therefore be
> before the hg pull ?
> 

I don't know that this belongs in the build system but I could be swayed on that. The other option is to store this in a Python script alongside this BuildStep. The downside here is that we'd need an additional ShellCommand to check-out the script. Admittedly, this isn't much of a barrier.

> ...
> >+cur_force_date = urllib2.urlopen(url).read()
> 
> What happens if the information can't be retrieved ? eg the site is down or
> returning bogus info. Would it be worth checking for sane input here before
> trampling on the state files ? 
> 
> And do you think we'd want to return an error and prevent builds if url is down
> ? Probably not, but I guess there needs to be something relatively noisy if
> clobbers are failing. Perhaps a TinderboxPrint, if you can get to the log from
> here.
> 

Yeah, I think we just want to not clobber if we can't pull the URL. I'll add some error handling code here.

> >+  print "Checking clobber URL: %s" % url
> >+  print "Current clobber date: %s" % cur_force_date
> 
> Would be nice to move these up to where it really happens (either side of the
> urlopen call).
> 

Sure.

> >+  if (last_clobber + ONE_DAY < cur_date):
> >+    print "More than 24 hours have passed since the last clobber, clobbering " \
> >+          "build directory"
> >+    do_clobber()
> 
> I'm a bit concerned about this, given we could end up with N slow builds a day
> for N connected slaves (assuming there are enough jobs/slave for randomness to
> do it's thing when they're allocated). Backing it off to once a week would
> help, particularly if we can stagger the clobbers over that period.

Sure, can do.

> (I'd really
> love if we could send the tinderbox server some TinderPrints as part of the
> build start email, including the source revision and clobber, then it's clear
> to all why a build is slow and what it's building.)
> 

No kidding!
Created attachment 330253 [details] [diff] [review]
address nick's review comments
Attachment #329824 - Attachment is obsolete: true
Attachment #329824 - Flags: review?(nthomas)
Attachment #330253 - Flags: review?(nthomas)
Created attachment 330575 [details] [diff] [review]
master.cfg changes for clobbers
Attachment #330575 - Flags: review?(ccooper)

Comment 14

9 years ago
> (I'd really
> love if we could send the tinderbox server some TinderPrints as part of the
> build start email, including the source revision and clobber, then it's clear
> to all why a build is slow and what it's building.)
> 

Is there a bug filed on what sounds like a good idea against tinderbox server? :-)
Comment on attachment 330253 [details] [diff] [review]
address nick's review comments

r+, but with maybe two fixes on checkin please, and take a look at some suggested improvements.

>Index: misc.py
>===================================================================
>+except URLError:
>+  print "Couldn't poll %s, skipping forced clobber" % url

I ran a few tests with the lines added with this patch, and I think this will barf if there is a DNS problem (NameError). Might be worth just a blanket "except:" ? Hard to know what the main error conditions will be. You could change this without review.

>+  if (last_clobber + PERIODIC_CLOBBER_TIME < cur_date):
>+    print "More than 24 hours have passed since the last clobber, clobbering " \
>+          "build directory"

The "24 hours" part of this message is out of date now, so please fix it up in a way that displays PERIODIC_CLOBBER_TIME in a human-friendly way (the default print format for a timedelta isn't too bad). No further review req'd.

>+    try:
>+      do_clobber()
>+    except:
>+      print "Couldn't clobber properly, not updating last-clobber file"
>+      sys.exit(0)

These are both suggestions: 

* You could argue that failure to clobber will require intervention from us and should send the build red. Or perhaps that would be a PITA. I'm assuming that exit(1) propagates back up. What do you think ?
* Should there be a try wrapper around the do_clobber for the forced case as well as the periodic one ? Maybe move the try inside do_clobber ?
Attachment #330253 - Flags: review?(nthomas) → review+
(In reply to comment #15)
> (From update of attachment 330253 [details] [diff] [review])
> r+, but with maybe two fixes on checkin please, and take a look at some
> suggested improvements.
> 
> >Index: misc.py
> >===================================================================
> >+except URLError:
> >+  print "Couldn't poll %s, skipping forced clobber" % url
> 
> I ran a few tests with the lines added with this patch, and I think this will
> barf if there is a DNS problem (NameError). Might be worth just a blanket
> "except:" ? Hard to know what the main error conditions will be. You could
> change this without review.
> 

Yeah, that makes sense.

> >+  if (last_clobber + PERIODIC_CLOBBER_TIME < cur_date):
> >+    print "More than 24 hours have passed since the last clobber, clobbering " \
> >+          "build directory"
> 
> The "24 hours" part of this message is out of date now, so please fix it up in
> a way that displays PERIODIC_CLOBBER_TIME in a human-friendly way (the default
> print format for a timedelta isn't too bad). No further review req'd.
> 

Sounds good.

> >+    try:
> >+      do_clobber()
> >+    except:
> >+      print "Couldn't clobber properly, not updating last-clobber file"
> >+      sys.exit(0)
> 
> These are both suggestions: 
> 
> * You could argue that failure to clobber will require intervention from us and
> should send the build red. Or perhaps that would be a PITA. I'm assuming that
> exit(1) propagates back up. What do you think ?

I think the most common case here will be win32 only partly clobbering. I think it makes a lot of sense to fail out in that case, and probably most other failures in do_clobber()

> * Should there be a try wrapper around the do_clobber for the forced case as
> well as the periodic one ? Maybe move the try inside do_clobber ?
> 

Yeah, that makes sense too.
Status: NEW → ASSIGNED
Created attachment 330774 [details] [diff] [review]
[checked in] Clobberer BuildStep as landed, addresses Nick's last review comments

Checking in misc.py;
/cvsroot/mozilla/tools/buildbotcustom/steps/misc.py,v  <--  misc.py
new revision: 1.5; previous revision: 1.4
done
Attachment #330253 - Attachment is obsolete: true
(Reporter)

Updated

9 years ago
Attachment #329822 - Flags: review?(benjamin) → review+
Comment on attachment 330575 [details] [diff] [review]
master.cfg changes for clobbers

"clobberer" <- awesome
Attachment #330575 - Flags: review?(ccooper) → review+
Depends on: 447827
Whiteboard: [checkin to hg.m.o/build/tools]
Comment on attachment 330774 [details] [diff] [review]
[checked in] Clobberer BuildStep as landed, addresses Nick's last review comments

>Index: misc.py
>===================================================================
>+try:
>+  print "Checking clobber URL: %s" % url
>+  cur_force_date = urllib2.urlopen(url).read()
>+  print "Current forced clobber date: %s" % cur_force_date
>+  if not os.path.exists('force-clobber'):
>+    write_file(cur_force_date, 'force-clobber')
>+  else:
>+    old_force_date = open('force-clobber').read()
>+    print "Last forced clobber: %s" % old_force_date
>+    if old_force_date != cur_force_date:
>+      print "Clobber forced, clobbering build directory"
>+      do_clobber()
>+      write_file(cur_force_date, 'force-clobber')
>+      write_file(cur_force_date, 'last-clobber')
>+      sys.exit(0)
>+except:
>+  print "Couldn't poll %s, skipping forced clobber" % url

Turns out that sys.exit() throws an exception to get Python to quit, and this except statement will catch it. I've got to re-work this a little bit.
Comment on attachment 329822 [details] [diff] [review]
[checked in] ugly, basic front-end for clobbering

changeset:   0:7ae7fb134bf7
Attachment #329822 - Attachment description: ugly, basic front-end for clobbering → [checked in] ugly, basic front-end for clobbering
Trying to push this through now. Looks like Windows is having issues with the BuildStep. It's not printing _anything_ out in the logs, or clobbering properly.

Suspect it has something to do with MSYS i/o handling.

http://staging-master.build.mozilla.org:8010/builders/WINNT%205.2%20mozilla-central%20build/builds/95/steps/shell_2/logs/stdio
Priority: P3 → P2
Whiteboard: [checkin to hg.m.o/build/tools]
Created attachment 332401 [details] [diff] [review]
[checked in] clobbering code in a stand-alone script

I fussed about but couldn't get Windows to work with with our clobber step. This is mostly just a straight rip-out off that Python code. The only change I've made is to make sys.exit() work properly by catching SystemExit.

When I check this in, I will be deleting the MozillaClobber step from buildbotcustom.
Attachment #332401 - Flags: review?(nthomas)
Comment on attachment 332401 [details] [diff] [review]
[checked in] clobbering code in a stand-alone script

I'm open to suggestions about the directory structure in hg.m.o/build/tools, btw
Created attachment 332405 [details] [diff] [review]
master.cfg w/ external clobber script
Attachment #332405 - Flags: review?(nthomas)
Attachment #330575 - Attachment is obsolete: true

Updated

9 years ago
Attachment #332401 - Flags: review?(nthomas) → review+
Comment on attachment 332405 [details] [diff] [review]
master.cfg w/ external clobber script

r+ with nit's addressed.

>diff -r 93816cedf102 mozilla2-unittest/master.cfg
...
>+moz2_win32_unittest_factory2.addStep(ShellCommand,
>+    command=['rm', '-rfv', 'tools'],
>+    workdir='.',
>+    env=MozillaEnvironments['win32-vc8-mozbuild-unittest'])
>+moz2_win32_unittest_factory2.addStep(ShellCommand,
>+    command=['hg', 'clone', 'http://hg.mozilla.org/build/tools'],
>+    workdir='.',
>+    env=MozillaEnvironments['win32-vc8-mozbuild-unittest'])
>+moz2_win32_unittest_factory2.addStep(ShellCommand,
>+    command=['python', 'tools/clobberer/clobberer.py', clobberURL],
>+    workdir='.',
>+    env=MozillaEnvironments['win32-vc8-mozbuild-unittest'])

nit - LocalEnvironments['win32-vc8-mozbuild-unittest'] is used in subsequent steps, seems to be used for this factory only. We should probably be consistent within the factory, and merge the Environments for win32 elsewhere.

>diff -r 93816cedf102 mozilla2/config.py
...
>+BUILD_TOOLS_REPO = 'http://hg.mozilla.org/build/tools'

Nit: _URL suffix to match existing style, and fix use in master.cfg
Attachment #332405 - Flags: review?(nthomas) → review+
(In reply to comment #23)
> (From update of attachment 332401 [details] [diff] [review])
> I'm open to suggestions about the directory structure in hg.m.o/build/tools,
> btw

I don't have any dogma to dispense, other than "not too flat, not too nested". At least we can move/rename easily in Hg.

(In reply to comment #25)
> (From update of attachment 332405 [details] [diff] [review])
> r+ with nit's addressed.
> 
> >diff -r 93816cedf102 mozilla2-unittest/master.cfg
> ...
> >+moz2_win32_unittest_factory2.addStep(ShellCommand,
> >+    command=['rm', '-rfv', 'tools'],
> >+    workdir='.',
> >+    env=MozillaEnvironments['win32-vc8-mozbuild-unittest'])
> >+moz2_win32_unittest_factory2.addStep(ShellCommand,
> >+    command=['hg', 'clone', 'http://hg.mozilla.org/build/tools'],
> >+    workdir='.',
> >+    env=MozillaEnvironments['win32-vc8-mozbuild-unittest'])
> >+moz2_win32_unittest_factory2.addStep(ShellCommand,
> >+    command=['python', 'tools/clobberer/clobberer.py', clobberURL],
> >+    workdir='.',
> >+    env=MozillaEnvironments['win32-vc8-mozbuild-unittest'])
> 
> nit - LocalEnvironments['win32-vc8-mozbuild-unittest'] is used in subsequent
> steps, seems to be used for this factory only. We should probably be consistent
> within the factory, and merge the Environments for win32 elsewhere.
> 

I'm not sure what you mean by this...
Just that 
* moz2_win32_unittest_factory2 uses LocalEnvironments['win32-vc8-mozbuild-unittest']
* all the other factories use one of the MozillaEnvironments
* your copy and paste adds steps to moz2_win32_unittest_factory2 using  MozillaEnvironments
* I don't know if the two windows environments differ, so we should be consistent within a given factory
Yeah, just saw that, whoops.
Created attachment 332515 [details] [diff] [review]
master.cfg v3 - addresses Nick's comments
Attachment #332405 - Attachment is obsolete: true
Comment on attachment 332401 [details] [diff] [review]
[checked in] clobbering code in a stand-alone script

changeset:   1:881cad398b4f
Attachment #332401 - Attachment description: clobbering code in a stand-alone script → [checked in] clobbering code in a stand-alone script
Attachment #332515 - Flags: review?(nthomas)

Updated

9 years ago
Attachment #332515 - Flags: review?(nthomas) → review+
Priority: P2 → P3
(Assignee)

Updated

8 years ago
Duplicate of this bug: 436449
(Assignee)

Updated

8 years ago
Attachment #330774 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Assignee: bhearsum → catlee
Duplicate of this bug: 431529
Duplicate of this bug: 393417
(Assignee)

Comment 35

8 years ago
Created attachment 359056 [details] [diff] [review]
updated front-end and clobberer.py script
Attachment #359056 - Flags: review?(bhearsum)
(Assignee)

Comment 36

8 years ago
Created attachment 359059 [details] [diff] [review]
Add clobberURL to MozillaBuildFactory
Attachment #359059 - Flags: review?(bhearsum)
(Assignee)

Comment 37

8 years ago
Created attachment 359065 [details] [diff] [review]
Updates to mozilla2-staging to set clobber URL
Attachment #359065 - Flags: review?(bhearsum)
Comment on attachment 359065 [details] [diff] [review]
Updates to mozilla2-staging to set clobber URL

>diff -r 714ad802ae42 mozilla2-staging/config.py
>--- a/mozilla2-staging/config.py	Mon Jan 26 21:32:14 2009 +1300
>+++ b/mozilla2-staging/config.py	Tue Jan 27 12:24:35 2009 -0500
>@@ -13,18 +13,19 @@
> STAGE_BASE_PATH = '/home/ftp/pub/firefox'
> STAGE_GROUP = None
> STAGE_SSH_KEY = 'ffxbld_dsa'
> AUS2_USER = 'cltbld'
> AUS2_HOST = 'staging-stage.build.mozilla.org'
> DOWNLOAD_BASE_URL = 'ftp://ftp.mozilla.org/pub/mozilla.org/firefox'
> GRAPH_SERVER = 'graphs-stage.mozilla.org'
> GRAPH_SELECTOR = 'server'
> BUILD_TOOLS_REPO_PATH = 'users/stage-ffxbld/tools'
> DEFAULT_BUILD_SPACE = 5
>+CLOBBER_URL = 'http://build.mozilla.org/clobberer/staging.php'
> 

Is this going to be the final home for this? I'm a little worried about updating the PHP file and forgetting to change the database name in it. I think it's a bit nicer to have a separate directory so we can use the exact same php file without modification or worry. It's up to you though!

r=bhearsum either way
Attachment #359065 - Flags: review?(bhearsum) → review+
Comment on attachment 359059 [details] [diff] [review]
Add clobberURL to MozillaBuildFactory

>Index: factory.py
>===================================================================
>+        if self.clobberURL is not None:
>+            command = ['python', 'tools/clobberer/clobberer.py',
>+             self.clobberURL, self.branchName,
>+             WithProperties("%(buildername)s"),
>+             WithProperties("%(slavename)s")
>+            ]
>+            self.addStep(ShellCommand,
>+             command=command,
>+             description=['checking','clobber','times'],
>+             workdir='.',
>+             flunkOnFailure=False,

Why flunkOnFailure=False? Whether we clobber or not this script should return 0, unless something weird happens. Please remove this.

r=bhearsum with that change
Attachment #359059 - Flags: review?(bhearsum) → review+
Comment on attachment 359056 [details] [diff] [review]
updated front-end and clobberer.py script

>diff -r cf6614a8d619 clobberer/clobberer.py

>+  parser.add_option("-t", "--periodic", dest="period", type="float",
>+      default=24*7, help="hours between periodic clobbers")

Nice. Thanks for making this an option.

>+
>+  options, args = parser.parse_args()
>+  PERIODIC_CLOBBER_TIME = timedelta(hours = options.period)

camelcase this to make it consistent, please.

>+    cur_date = datetime.utcnow()
>+    try:
>+      last_clobber = str_to_datetime(open("last-clobber").read().strip())
>+      print "Last clobber: %s" % datetime_to_str(last_clobber)
>+      print "Current time: %s" % datetime_to_str(cur_date)
>+      if (last_clobber + PERIODIC_CLOBBER_TIME < cur_date):
>+        print "*** More than %s have passed since the last clobber, clobbering *** " % \
>+          PERIODIC_CLOBBER_TIME
>+        do_clobber(options.dryrun)
>+        # if do_clobber fails the script will exit and this will not be executed
>+        write_file(cur_date, "last-clobber")
>+    except ValueError:
>+      write_file(cur_date, "last-clobber")

What are you updating last-clobber on ValueError? Purposely, it doesn't get updated when the clobber fails.

>diff -r cf6614a8d619 clobberer/index.php

>@@ -1,86 +1,218 @@
>-<!---
>+<?php
>+/*
> This is a web interface that allows developers to clobber mozilla-central,
> actionmonkey, and tracemonkey builds.

Let's update this comment to say "...allow developers to clobber Mercurial based builds", or something to that effect.

> 
> This script simply updates a database that Buildbot reads from at the start of
> every build. The BuildStep that does this is here:
> http://mxr.mozilla.org/mozilla/source/tools/buildbotcustom/steps/misc.py#167

And remove the '#167' portion of this. (This is my fault, but since you're doing the patches anyways...;-)



>-$q = $dbh->query('SELECT count(*) FROM sqlite_master WHERE NAME="CLOBBERS"');
>+$q = $dbh->query('SELECT count(*) FROM sqlite_master WHERE NAME="clobber_times"');
> $exists = $q->fetch(PDO::FETCH_NUM);
>-if (!$exists[0]) {
>-  $dbh->exec('CREATE TABLE CLOBBERS (short_name VARCHAR(30),'
>-            .'long_name VARCHAR(50),'
>-            .'lastclobber VARCHAR(30),'
>-            .'clobberer VARCHAR(50))');
>+if (!$exists or !$exists[0]) {

Why !$exists?

> 
>+//
>+// Handle form submision

nit: fix the typo here

>+//
> if ($_POST['form_submitted']) {
>   $clobbers = array();
>+  $slaves = array();
>   foreach ($_POST as $k => $v) {
>     $t = explode('-', $k, 2);
>-    if ($t[0] == 'clobber') {
>-      array_push($clobbers, $t[1]);
>+    // We only care about slaves
>+    if ($t[0] == 'slave') {
>+      $id = e($t[1]);
>+      $user = e($_SERVER['REMOTE_USER']);
>+      $dbh->exec("UPDATE clobber_times SET lastclobber = DATETIME(\"NOW\"), clobberer = $user WHERE id = $id");

What's $id here? It doesn't look like this code handles slave+builder, so I'm thinking it would clobber all trees for a slave every time.

Everything else looks fine, holding off on changing the flag because of the question above.
Comment on attachment 359056 [details] [diff] [review]
updated front-end and clobberer.py script

Chris talked me through the form submission section, this looks good.
Attachment #359056 - Flags: review?(bhearsum) → review+
(Assignee)

Comment 42

8 years ago
Created attachment 359304 [details] [diff] [review]
updated front-end and clobberer.py script

This new patch should address the issues above.
Attachment #359056 - Attachment is obsolete: true
Attachment #359304 - Flags: review?(bhearsum)
Comment on attachment 359304 [details] [diff] [review]
updated front-end and clobberer.py script

Looks good to me.
Attachment #359304 - Flags: review?(bhearsum) → review+
(Assignee)

Comment 44

8 years ago
Created attachment 360709 [details] [diff] [review]
Add clobberURL and clobberTime to MozillaBuildFactory
Attachment #359059 - Attachment is obsolete: true
Attachment #360709 - Flags: review?(bhearsum)
(Assignee)

Comment 45

8 years ago
Created attachment 360712 [details] [diff] [review]
updated front-end and clobberer.py script
Attachment #359304 - Attachment is obsolete: true
Attachment #360712 - Flags: review?(bhearsum)
(Assignee)

Comment 46

8 years ago
Created attachment 360721 [details] [diff] [review]
Updates to mozilla2-staging to set clobber URL and clobber time
Attachment #359065 - Attachment is obsolete: true
Attachment #360721 - Flags: review?(bhearsum)
Comment on attachment 360709 [details] [diff] [review]
Add clobberURL and clobberTime to MozillaBuildFactory

>diff -r 8b3c53f98959 process/factory.py
>--- a/process/factory.py	Wed Feb 04 09:59:53 2009 -0500
>+++ b/process/factory.py	Thu Feb 05 09:01:53 2009 -0500
>+        if self.clobberURL is not None:
>+            if self.clobberTime is None:
>+                self.clobberTime = 24*7

I guess you can't assign self.clobberTime above, but can you set defaultClobberTime as a class-level variable, or some such?

Looks good otherwise. r=me with that change
Attachment #360709 - Flags: review?(bhearsum) → review+
Attachment #360721 - Flags: review?(bhearsum) → review+
Attachment #360712 - Flags: review?(bhearsum) → review+
Comment on attachment 360712 [details] [diff] [review]
updated front-end and clobberer.py script

>+every build.
>+http://mxr.mozilla.org/mozilla/source/tools/buildbotcustom/process/factory.py
>+*/

Should update this link now.

This is fine otherwise.
(Assignee)

Comment 49

8 years ago
Created attachment 360754 [details] [diff] [review]
Add clobberURL and clobberTime to MozillaBuildFactory

Changes here are we clobber the tools directory before starting, and then we don't have to do hg pull -u since we've just cloned it.

Also, pass '-s tools' to clobberer.py, which will skip clobbering the tools directory, which will let purge_builds.py run properly afterwards.
Attachment #360709 - Attachment is obsolete: true
Attachment #360754 - Flags: review?(bhearsum)
(Assignee)

Comment 50

8 years ago
Created attachment 360756 [details] [diff] [review]
updated front-end and clobberer.py script

Add support for skipping files/directories on the command line.
Attachment #360712 - Attachment is obsolete: true
Attachment #360756 - Flags: review?(bhearsum)
Attachment #360754 - Flags: review?(bhearsum) → review+
Attachment #360756 - Flags: review?(bhearsum) → review+
(Assignee)

Comment 51

8 years ago
Comment on attachment 360754 [details] [diff] [review]
Add clobberURL and clobberTime to MozillaBuildFactory

changeset:   187:9517989a64e5
Attachment #360754 - Flags: checked‑in+
(Assignee)

Comment 52

8 years ago
Comment on attachment 360721 [details] [diff] [review]
Updates to mozilla2-staging to set clobber URL and clobber time

changeset:   726:5a0a4e79714e
Attachment #360721 - Flags: checked‑in+
(Assignee)

Comment 53

8 years ago
Comment on attachment 360756 [details] [diff] [review]
updated front-end and clobberer.py script

changeset:   39:03ee28c55c27
Attachment #360756 - Flags: checked‑in+
(Assignee)

Comment 54

8 years ago
Created attachment 360784 [details] [diff] [review]
set default clobber URL and clobber time on production

for when we want to deploy this onto production
Attachment #360784 - Flags: review?(bhearsum)
Attachment #360784 - Flags: review?(bhearsum) → review+
(Assignee)

Comment 55

8 years ago
Comment on attachment 360784 [details] [diff] [review]
set default clobber URL and clobber time on production

changeset:   932:afd401b42348
Attachment #360784 - Flags: checked‑in+
(Assignee)

Comment 56

8 years ago
It looks like this is working well since being deployed yesterday.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago8 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.