545825 - New build columns should default Scrape to True instead of False

Status: RESOLVED FIXED

(Webtools Graveyard :: Tinderbox, defect)

Description

[Nick Thomas [:nthomas] (UTC+12)]

... at least on tinderbox.mozilla.org, but I think we need something in the server code first. 

The reason is that we TinderboxPrint alot of useful information these days (eg code revision, test results) and scraping gets that into the json datasource. Tinderboxpushlog relies on the json to assign a particular compile/test to a revision, so builds that aren't scraped are invisible to developers. Currently, we (RelEng) go round and manually set the bit as the new builders report, which relies on us not being forgetful.

Comment 1

[John Ford [:jhford] CET/CEST Berlin Time]

I have a bunch (3 platforms x 7 branches x ?? number of tests, one per column) that I will need to have this enabled on.

Comment 2

[Aki Sasaki (not active)]

Yeah, I was thinking about this too.

I think the offending code is here -- http://mxr.mozilla.org/webtools/source/tinderbox/processbuild.pl#180 .  I think my approach would be to rename scrapebuilds.pl to noscrapebuilds.pl, re-label the checkboxes in the admintree cgi, and test.

Probably not taking this on, however, unless something changes priority-wise.

Comment 3

[cls]

Attachment: v0.9

Here's a local change I made because I was tired of loading the admin page every time we added a build.  It doesn't completely re-arch the existing scrapebuilds.pl.  The default value should probably be moved to tbglobals.pl though and I get an extraneous numeric entry so it needs a minor cleanup.

Comment 4

[Aki Sasaki (not active)]

cc'ing bear.
I know TB1 isn't as nice as TB2 to hack on since it's non-modular, but would this be a quick fix for you and/or a bug of interest?

Comment 5

[Mike Taylor [:bear]]

I wouldn't mind at all taking it on - would be a great refresher for TB1 code

Comment 6

[Mike Taylor [:bear]]

Attachment: default to global scrape value of True

Takes cls' patch, adjusts it for current trunk code and also moves the default value to tbglobals.pl

Comment 7

[cls]

Comment on attachment 431362 [details] [diff] [review]
default to global scrape value of True

There appears to be some extra changes in processbuild.pl.

r=cls if you remove the "./ =>" bit.

Comment 8

[Mike Taylor [:bear]]

cool - i'll remove that errant diff piece and submit it for a formal review tonight.

thanks

Comment 9

[Mike Taylor [:bear]]

Attachment: default to global scrape value of True v0.2

removed errant keystrokes and applying for review

Comment 10

[Aki Sasaki (not active)]

Oh, hey, I have write perms.

Checking in doadmin.cgi;
/cvsroot/mozilla/webtools/tinderbox/doadmin.cgi,v  <--  doadmin.cgi
new revision: 1.42; previous revision: 1.41
done
Checking in processbuild.pl;
/cvsroot/mozilla/webtools/tinderbox/processbuild.pl,v  <--  processbuild.pl
new revision: 1.73; previous revision: 1.72
done
Checking in tbglobals.pl;
/cvsroot/mozilla/webtools/tinderbox/tbglobals.pl,v  <--  tbglobals.pl
new revision: 1.73; previous revision: 1.72
done

Comment 11

[Aki Sasaki (not active)]

Filed bug 552834 for tb.m.o update.

Comment 12

[Reed Loden [:reed]]

(In reply to comment #10)
> Oh, hey, I have write perms.

Just because you have write perms doesn't mean that you

Comment 13

[Reed Loden [:reed]]

(In reply to comment #12)
> Just because you have write perms doesn't mean that you

Ugh, this got sent too early. Basically, your CVS access is restricted to build-specific things (as per bug 480037, comment #0 and the bug it mentions), even though there's no technical controls stopping you from checking-in anywhere in /cvsroot (aside from js, nss, nspr, and a few other things). Tinderbox is outside of build-controlled files/directories, so you should not have checked this in.

Comment 14

[Aki Sasaki (not active)]

(In reply to comment #13)
> Ugh, this got sent too early. Basically, your CVS access is restricted to
> build-specific things (as per bug 480037, comment #0 and the bug it mentions),
> even though there's no technical controls stopping you from checking-in
> anywhere in /cvsroot (aside from js, nss, nspr, and a few other things).
> Tinderbox is outside of build-controlled files/directories, so you should not
> have checked this in.

Would you like me to back out this specific patch, or would you like to back out this patch? Or are you just pointing this out?

Comment 15

[Reed Loden [:reed]]

(In reply to comment #14)
> Would you like me to back out this specific patch, or would you like to back
> out this patch? Or are you just pointing this out?

Obviously no need to back it out, as that's pointless. Just please note it for the future. Thanks! :)

Comment 16

[[e-mail] (:bhearsum)]

Comment on attachment 431747 [details] [diff] [review]
default to global scrape value of True v0.2

Backed out because it broke mail parsing and log loading:
Checking in webtools/tinderbox/doadmin.cgi;
/cvsroot/mozilla/webtools/tinderbox/doadmin.cgi,v  <--  doadmin.cgi
new revision: 1.43; previous revision: 1.42
done
Checking in webtools/tinderbox/processbuild.pl;
/cvsroot/mozilla/webtools/tinderbox/processbuild.pl,v  <--  processbuild.pl
new revision: 1.74; previous revision: 1.73
done
Checking in webtools/tinderbox/tbglobals.pl;
/cvsroot/mozilla/webtools/tinderbox/tbglobals.pl,v  <--  tbglobals.pl
new revision: 1.74; previous revision: 1.73
done

Comment 17

[Mike Taylor [:bear]]

Attachment: default to global scrape value of True v0.3

adjusted patch to remove incorrect "my" from the tbglobals.pl definition of $::default_scrape

Comment 18

[Nick Thomas [:nthomas] (UTC+12)]

Justin (jabba), could we take attachment 433545 [details] [diff] [review] for a spin on tinderbox-stage ? Would be great to include it in bug 559348 if all is well.

Comment 19

[Nick Thomas [:nthomas] (UTC+12)]

ping IT: requesting that we apply attachment 433545 [details] [diff] [review] to tinderbox-stage.m.o

Comment 20

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

Found during triage.

(In reply to comment #19)
> ping IT: requesting that we apply attachment 433545 [details] [diff] [review] to tinderbox-stage.m.o

jabba/zandr: can you please apply the (refreshed and fixed) patch to tinderbox-stage?

Comment 21

[Phil Ringnalda (:philor)]

Isn't the way that it usually works that you land, you file an IT bug to update tinderbox-stage.m.o, then when it doesn't break you file yet another IT bug to update tinderbox.m.o?

Comment 22

[Reed Loden [:reed]]

(In reply to comment #21)
> Isn't the way that it usually works that you land, you file an IT bug to update
> tinderbox-stage.m.o, then when it doesn't break you file yet another IT bug to
> update tinderbox.m.o?

That is correct, but somebody should land the patch first before filing either one of those bugs. :)

Comment 23

[Phil Ringnalda (:philor)]

Checking in webtools/tinderbox/doadmin.cgi;
/cvsroot/mozilla/webtools/tinderbox/doadmin.cgi,v  <--  doadmin.cgi
new revision: 1.44; previous revision: 1.43
done
Checking in webtools/tinderbox/processbuild.pl;
/cvsroot/mozilla/webtools/tinderbox/processbuild.pl,v  <--  processbuild.pl
new revision: 1.75; previous revision: 1.74
done
Checking in webtools/tinderbox/tbglobals.pl;
/cvsroot/mozilla/webtools/tinderbox/tbglobals.pl,v  <--  tbglobals.pl
new revision: 1.82; previous revision: 1.81
done

Comment 24

[Dave Miller [:justdave]]

Getting the following error in cronspam once per minute (after we figured out why cron wasn't spamming us) :

Missing right curly or square bracket at /var/www/webtools/tinderbox/processbuild.pl line 466, at end of line
syntax error at /var/www/webtools/tinderbox/processbuild.pl line 466, at EOF
Execution of /var/www/webtools/tinderbox/processbuild.pl aborted due to compilation errors.

Comment 25

[Reed Loden [:reed]]

Attachment: follow-up to v3 to fix problems

This fixes a number of problems in the code and refactors it. I haven't tested this.

Comment 26

[Mike Taylor [:bear]]

(In reply to comment #25)
> Created attachment 503917 [details] [diff] [review]
> follow-up to v3 to fix problems
> 
> This fixes a number of problems in the code and refactors it. I haven't tested
> this.

If I get access to staging tbox soon(ish) i'll test it there, if not i'll break out my notes and set up a local one.

Comment 27

[Dave Miller [:justdave]]

ok, this patch is locally-applied on tinderbox-stage.  I nuked the 160k messages out of the incoming mail queue so it's starting from scratch. (it'd take it a week or two to catch up on a queue that size).

Comment 28

[Mike Taylor [:bear]]

thanks justdave!

Comment 29

[Chris Cooper [:coop] (he/him)]

(In reply to comment #27)
> ok, this patch is locally-applied on tinderbox-stage.  I nuked the 160k
> messages out of the incoming mail queue so it's starting from scratch. (it'd
> take it a week or two to catch up on a queue that size).

Has anyone checked tinderbox-stage to see whether this is working? Is the reviewed patch(es) ready to land?

Comment 30

[Reed Loden [:reed]]

Checking in processbuild.pl;
/cvsroot/mozilla/webtools/tinderbox/processbuild.pl,v  <--  processbuild.pl
new revision: 1.76; previous revision: 1.75
done

Comment 31

[Phil Ringnalda (:philor)]

And that got deployed on tinderbox.m.o, and then armenzg added crashtest-ipc and reftest-ipc, and as his email says, "you need to check your trees and enabling scraping" because it isn't checked by default.

Comment 32

[Armen [:armenzg]]

This is still not fixed as I enabled XP debug unit tests and I had to set scraping to True.

Comment 33

[Chris Cooper [:coop] (he/him)]

(In reply to comment #31)
> And that got deployed on tinderbox.m.o, and then armenzg added crashtest-ipc
> and reftest-ipc, and as his email says, "you need to check your trees and
> enabling scraping" because it isn't checked by default.

Do we actually know this got deployed to production?

I see bug 624613 for updating tinderbox-stage but no corresponding bug for production.

bear: can you please followup with IT to make sure it's running in production?

Comment 34

[Mike Taylor [:bear]]

this is running in production but in a limited manner - the admin page doesn't reflect the default change.  philor discovered this during his testing last week.

marking as closed as I think this is "fixed enough"

Comment 35

[Phil Ringnalda (:philor)]

I'm not sure that's the case. The current scheme works fine if nobody ever touches admintree, so it's great for -Release trees, but when the first builds for the Firefox4.0 tree finally showed up, I needed to hide the WinXP Md1 column, so I made that single change in admintree, and as a result wiped out all the WinXP debug tests for a day or so, since they were relying on defaulting to scrape, and I didn't think to check them since they were fine. I don't remember having done anything to Cedar at all, but I noticed this morning that it didn't have any Windows debug tests or Ripc or Cipc showing in tbpl, because of what must have been the same thing: someone made the one change they wanted to make, and by submitting blew away the defaulting.