393092 - tracking bug for next try server version

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P3)

Description

[[e-mail] (:bhearsum)]

This version will (likely) include the following things:
* Add a field that lets people enter a short phrase that is added to the filename. This will help developers find their builds easier.
* Ability to specify the patch level (-p)
* Ability to upload mozconfig
* Ability to specify a pull date/time/branch/tag
* Show e-mail address/build number on main Tinderbox page 
* Show download link on main Tinderbox page
* Ability to build from HG repositories

I also don't want to deploy this before the existing slave bugs are fixed.

Comment 1

[[e-mail] (:bhearsum)]

Attachment: script to process .info files and send changes to buildbot

This is a replacement for downloadpatches.sh. I re-wrote it in Perl because it was becoming unmanageably complex in shell.

There may be changes made to this if bugs crop up during testing.

Comment 2

[[e-mail] (:bhearsum)]

Attachment: updated cgi script; many new options + hg support

This is an update to the Try server cgi script. It supports the following new things: specifying the patch level, pulling by branch/tag/date, adding a user-specified identifier to the final package name, uploading a mozconfig file, and HG repository support!

I imagine there will be a bug or two uncovered during testing, so this probably won't be the final version.

Comment 4

[Robert Helmer [:rhelmer]]

Comment on attachment 281310 [details]
script to process .info files and send changes to buildbot

>#!/usr/bin/perl
>
># File: processchanges.pl
># Author: Ben Hearsum
># Description:
>#   TODO
>#
>
>use strict;
>use warnings;
>use LWP::Simple;

>`wget --no-check-certificate -r -l1 -np -nc -nd -R"index.html*" -P $PATCHDIR $PATCHURL &>/dev/null`;

Why are you 'use'ing LWP if you're not actually using it? :)


>            # TODO: is there a perl way to do this?
>            system("touch $file");


check out utime

Comment 5

[[e-mail] (:bhearsum)]

Attachment: same as before but addresses rhelmer's comments

I removed the 'use LWP::Simple' and switched the system(..) to utime.

Thanks for getting to this so quickly.

Comment 6

[J. Paul Reed [:preed]]

Comment on attachment 281312 [details]
updated cgi script; many new options + hg support

Hey bhearsum: can you post this as a diff? I thought there was an original sendchange.cgi somewhere... is it not in the tree?

Comment 7

[[e-mail] (:bhearsum)]

There's uploadpatch.cgi; I don't have CVS access (and cvsdo will not work for me) so I can't create a diff.

I should have a CVS account in the next day or two if you want to wait until then, but because of the rename I don't think it will make a difference.

Comment 8

[J. Paul Reed [:preed]]

Comment on attachment 281312 [details]
updated cgi script; many new options + hg support


In general: have you thought about separating out the HTML (presentation) from the business logic? I think that would help a lot here. However, this is not a blocker. I r-'d for the specific reasons below, but as we rely on this more and more, and add more features, I think it's going to become increasingly harder to wade through the HTML vs. the logic. We-as-a-group made this mistake with Bugzilla, Tinderbox, Bonsai... you name it. :-)

>    my $mozillaRepoPath = "http://hg.mozilla.org/mozilla-central";
>    my $tamarinRepoPath = "http://hg.mozilla.org/tamarin-central";

Should these be constants?

>    my $err = "";

I believe what you're really wanting here is "if (exists($args{'patchLevel'})," etc., because you code won't get executed if I have a branch named, for example '0', and that's probably not what you're intending.

>    if ($args{'patchLevel'}) {
>        $patchLevel = $args{'patchLevel'};
>    }
>    if ($args{'branch'}) {
>        $branch = $args{'branch'};
>    }
>    if ($args{'identifier'}) {
>        $identifier = $args{'identifier'};
>    }
>    if ($args{'description'}) {
>        $description = $args{'description'};
>    }
>    if ($args{'type'}) {
>        $type = $args{'type'};
>    }
>    if ($args{'mozillaRepoPath'}) {
>        $mozillaRepoPath = $args{'mozillaRepoPath'};
>    }
>    if ($args{'tamarinRepoPath'}) {
>        $tamarinRepoPath = $args{'tamarinRepoPath'};
>    }
>    if ($args{'err'}) {
>        $err = $args{'err'};
>    }
>    my $limit = $SIZE_LIMIT / 1024;
>    my $i;
>

><form action="http://localhost/~bhearsum/cgi-bin/buildbot/sendchange.cgi"
>      method="post" enctype="multipart/form-data">

Assuming that'll change. :-)

Should it be a constant (then you can easily change it for devtest)?

>    for $i ("0".."9") {
>        if ($patchLevel == $i) {
>            print '<option selected="selected">' . $i . "</option>\n";
>        }
>        else {
>            print '<option>' . $i . '</option>';
>        }
>    }

Is this what you define my $i for above? I'd reduce the scope of $i to the loop, and you don't have to quote the args:

foreach my $i (0 .. 9) {
   print $i;
}

(Admittedly, that's a perlism I hate, so I'd do the much more verbose: for (my $i = 0; $i < 10; $i++).... but that's me ;-)

>    # only allow alphanumeric, '_', and whitespace
>    my $allowedChars = '^[^\w\s]+$';
>    my $filenameChars = '^([\w-]|\.[\w-])+$';

This is very good; make it a constant?

>    my %args = (
>        patchLevel          => $patchLevel,
>        branch              => $branch,
>        identifier          => $identifier,
>        description         => $description,
>        type                => $type,
>        mozillaRepoPath     => $mozillaRepoPath,
>        tamarinRepoPath     => $tamarinRepoPath,
>        err                 => "",
>    );

So, I'm gonna call this one out a little bit. So, we do function calls in perl a little differently to help improve readability. I think you're cheating a bit here, because you're constructing the args to pass in %args, and then later call WritePage(%args), thus defeating the main purpose of the convention: readability of function arguments at the call site of the function. It's also a helpful style guideline because I think if you were to change this to:

WritePage(patchLevel          => $patchLevel,
 branch              => $branch,
identifier          => $identifier,
 description         => $description,
 type                => $type,
 mozillaRepoPath     => $mozillaRepoPath,
 tamarinRepoPath     => $tamarinRepoPath,
 err                 => "",), it would look a little weird/possibly absurd... and you'd be correct.

This hints to WritePage() itself taking too many arguments, and implies that a refactor of the code might be in order. Maybe this should be an encapsulated object? Maybe the html code should be separated from the biz logic? There are lots of ways to solve it... and I'd be happy to help you come up with one. It's something to think about, though. :-)

>    # Using a patchFile
>    if ($type eq "patch") {
...
>    } elsif ($type eq "hg") {

...
>        # generate the infofile name
>        $infoFile = "$time-hg.info";
>    }

What if it's neither? $type comes from the form, correct? If so, you don't want to trust that input.

Also, I think I may have mentioned this before, but I would not create files on the file system this way; there's a race condition here if two people submit at the exact same second, which... we all think is pretty uncommon, but it's more common than you think. :-)

Have you investigated File::Temp, or just doing something like $time-$user-hg.info?

($user may not be smart/safe to use either, but... some suggestions to think about)

>    if ($type eq "patch") {
>        print INFO "patchFile: $patchFile\n";
>        print INFO "patchLevel: $patchLevel\n";
>        print INFO "branch: $branch\n";
>    }
>    elsif ($type eq "hg") {
>        print INFO "mozillaRepoPath: $mozillaRepoPath\n";
>        print INFO "tamarinRepoPath: $tamarinRepoPath\n";
>    }

Again, if it's neither?

Find me on IRC if you'd like help fixing some of these or going through them more thoroughly; I actually think a lot of this will be quick/easy to fix; they're the typically potholes that you run into when you start doing a lot of webdev. :-)

Comment 9

[[e-mail] (:bhearsum)]

Attachment: address preed's concerns in comment#8

This is a full diff of the buildbot-try directory, so it contains the CGI scripts and processchanges.pl. Rob, I didn't request review from you again because there have been no changes to processchanges.pl since your review.

As for the CGI scripts...I've addressed all your concerns, Paul. All of the "presentation" has been moved to sendchange-ui.pm. I've also made $err into @err, which gets rid of the silly '<br />'s in Process().

Other notable things:
The if($args{'patchLevel'}) has been fixed. It now tests for existence and a non-empty value. (All of these arguments _should_ exist on every call, so testing for emptiness is necessary.) 

I got rid of %args in Process() and am now doing WritePage(foo => bar, baz => blah, ...). This doesn't fix the fact that WritePage probably takes too many arguments, but it makes it a harder issue to ignore). I plan to address this with the next upgrade.

Comment 10

[J. Paul Reed [:preed]]

Comment on attachment 281711 [details] [diff] [review]
address preed's concerns in comment#8

(In reply to comment #9)
> Created an attachment (id=281711) [details]
> address preed's concerns in comment#8

[snip]

> As for the CGI scripts...I've addressed all your concerns, Paul.

Looks good! r=preed.

> Other notable things:
> The if($args{'patchLevel'}) has been fixed. It now tests for existence and a
> non-empty value. (All of these arguments _should_ exist on every call, so
> testing for emptiness is necessary.) 

I did notice this; you might consider changing it to, for example:

if (exists($args{'mozillaRepoPath'}) && $args{'mozillaRepoPath'} !~ /^\s*/$/, if a bunch of spaces are also invalid.

But I wasn't sure about that; either way, easy enough to fix if it is.

Comment 11

[J. Paul Reed [:preed]]

(In reply to comment #10)

> if (exists($args{'mozillaRepoPath'}) && $args{'mozillaRepoPath'} !~ /^\s*/$/,
> if a bunch of spaces are also invalid.

Er, that should be:

if (exists($args{'foo'}) && $args{'foo'} !~ /^\s*$/) {
  ...
}

Comment 12

[[e-mail] (:bhearsum)]

Attachment: master.cfg currently being used on staging-build-console

No review for this yet, I'm posting it for bsmedberg's benefit.

Comment 16

[[e-mail] (:bhearsum)]

<strike>* Add a field that lets people enter a short phrase that is added to the
filename. This will help developers find their builds easier.</strike>
<strike>* Ability to specify the patch level (-p)</strike>
<strike>* Ability to upload mozconfig</strike>
<strike>* Ability to specify a pull date/time/branch/tag</strike>
* Show e-mail address/build number on main Tinderbox page 
* Show download link on main Tinderbox page -- DROPPED
<strike>* Ability to build from HG repositories</strike>

Comment 17

[[e-mail] (:bhearsum)]

(Copied from bug#397687)
Per discussion in today's build meeting:

- we'll firewall / vlan off the try machines from the rest of the build network
and the mozilla network in general. IT to implement.
- setup read-only access to the cvs server and hg server.
- we'll move the try buildmaster from build-console VM to its own VM, which
would be in the above firewalled area.
- developers would be able to use their own personal hg repos to try patches.

Comment 18

[[e-mail] (:bhearsum)]

Attachment: updates to processchange.pl and cgi form

Changes in this version:
* processchange.pl works properly when there is more than one patch (it tests by inode rather than mtime now).
* added indication of required fields on the form
* use an array for error messages. this removes the need for <br />'s as part of the message and feels cleaner to me.
* fixed a couple of typos when printing out the info file (mozilla-repo => mozillaRepoPath)

I left the '$args{'patchLevel'} ne ""' stuff how it was because values that get passed on to WritePage() are processed by Process() first. The test in WritePage() is simply to see if the user entered a value. Presumably, if that value gets into WritePage() it will be valid.

Comment 19

[J. Paul Reed [:preed]]

Comment on attachment 283545 [details] [diff] [review]
updates to processchange.pl and cgi form

>Index: processchanges.pl
>===================================================================
>RCS file: processchanges.pl
>diff -N processchanges.pl

Ahh... the good 'ol shell wall! :-)

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ processchanges.pl	4 Oct 2007 14:42:54 -0000
>@@ -0,0 +1,123 @@
>+#!/usr/bin/perl
>+
>+# File: processchanges.pl
>+# Author: Ben Hearsum
>+# Description:
>+#   TODO
>+#

One note about these headers... I'm assuming that this is under the standard open source license, so all of the files here should really have the standard Mozilla boilerplate:

http://www.mozilla.org/MPL/boilerplate-1.1/

Now that this is perl, is there not a reason to use MozBuild::Util?

The reason I ask:

>+# set up the patch directory
>+if (-e $PATCHDIR) {
>+    if (! -d $PATCHDIR) {
>+        print STDERR "Patch directory is a file\n";
>+        exit 1;
>+    }
>+}
>+else {
>+    if (! mkdir($PATCHDIR)) {

mkdir() will fail is you ever pass it anything other than a single string. MkdirWithPath is probably more useful, however I see that $PATHDIR is set to '.'; is that for testing, or production or both?

>+# set-up the logfile
>+open(LOGFILE, ">$LOGFILE");

Are you meaning to blow away the log file every time this runs?

>+`wget --no-check-certificate -r -l1 -np -nc -nd -R"index.html*" -P $PATCHDIR $PATCHURL &>/dev/null`;

MozBuild::RunShellCommand()?

>+opendir(DIR, $PATCHDIR);
>+my @files = grep { /^.*\.info$/ } readdir(DIR);

This will match files named ".info"; will this work properly if you have a file called that? You might want /^\w+\.info$/ instead...

Also, when you're done with the directory, closedir()?

>+# check for a lastpatch hardlink
>+if (-e "$PATCHDIR/lastpatch") {

This might be a good candidate for the use of catfile(), btw.

>+    # lastpatch exists, find any changes newer than it and forget about them
>+    my $file;
>+    while ($file = shift(@files)) {
>+        # stat(file)[9] is the last modified time

Nit: to help readability, I usually define $ST_MTIME as 9 up top and then use that instead.

>+        my $lastpatchInode = (stat("$PATCHDIR/lastpatch"))[1];
>+        my $fileInode = (stat("$PATCHDIR/$file"))[1];
>+        if ($lastpatchInode == $fileInode) {
>+            # the newest, processed change has been reached; get out of the loop
>+            # break out
>+            last;
>+        }
>+    }
>+}

I may be reading this wrong, but it seems to me those code assumes that you get the files back from readdir() in a sorted order. Am I reading that right? If so, I don't know if this code will always work, but readdir() doesn't guarantee files to be returned in any particular order.

>+
>+if (length(@files) == 0) {
>+    print LOGFILE "`date` - No Patches, exiting...";
>+    return 0;
>+}

No need to call out to date; try scalar(localtime()).

>+
>+# any changes left still need a sendchange generated
>+foreach my $file (@files) {
>+    my (%info, $key, $value, $cmd);
>+
>+    open(INFOFILE, "$PATCHDIR/$file");
>+    while (<INFOFILE>) {
>+        ($key, $value) = split(/: ?/, $_, 2);
>+        chomp($value);
>+        $info{$key} = $value;
>+    }
>+    close(INFOFILE);
>+
>+    if ($info{'type'} eq "patch") {
>+        $cmd = "$PYTHON_PATH $BUILDBOT_PATH sendchange "
>+             . "--username $info{'submitter'} --master $MASTER_HOST "
>+             . "--branch $PATCH_BRANCH --comments '$info{'description'}' "
>+             . "'mozconfig: $info{'mozconfig'}' "
>+             . "'identifier: $info{'identifier'}' "
>+             . "'branch: $info{'branch'}' "
>+             . "'patchLevel: $info{'patchLevel'}' "
>+             . "'patchFile: $info{'patchFile'}'";
>+    }
>+    elsif ($info{'type'} eq "hg") {
>+        $cmd = "$PYTHON_PATH $BUILDBOT_PATH sendchange "
>+             . "--username $info{'submitter'} --master $MASTER_HOST "
>+             . "--branch $HG_BRANCH --comments '$info{'description'}' "
>+             . "'mozconfig: $info{'mozconfig'}' "
>+             . "'identifier: $info{'identifier'}' "
>+             . "'mozillaRepoPath: $info{'mozillaRepoPath'}' "
>+             . "'tamarinRepoPath: $info{'tamarinRepoPath'}'";
>+    }
>+    
>+    my $changeSent = 1;
>+    open(SENDCHANGE, "$cmd |");

You should definitely close() this, and check for the return value, since that's of particular interest to ensure the command succeeded... but again, RunShellCommand() will probably help here.

>+    while (<SENDCHANGE>) {
>+        # if sending the change fails
>+        if (index($_, "NOT") != -1) {
>+            $changeSent = 0;
>+            print "Could not send change: $file\n";
>+            # if sending the change fails, this should make it try again next
>+            # time
>+            utime(time(), time(), $file);
>+        }
>+    }
>+
>+    if ($changeSent) {
>+        # create a hard link to the change just sent
>+        unlink("$PATCHDIR/lastpatch");
>+        link("$PATCHDIR/$file", "$PATCHDIR/lastpatch");
>+        sleep($DELAY);

You might consider using symlinks instead of hardlinks here. I think that'd help make it clearer when someone is poking around on the fileystem was "lastpatch" is really used for.

>Index: sendchange-ui.pm
>===================================================================
>RCS file: sendchange-ui.pm
>diff -N sendchange-ui.pm
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ sendchange-ui.pm	4 Oct 2007 14:42:54 -0000
>@@ -0,0 +1,337 @@
>+# File: sendchange-ui.cgi
>+# Author: Ben Hearsum
>+# Description:
>+#   This file is require'd by sendchange.cgi. It provides the UI for the cgi
>+#   form.

Same deal with the boilerplate.

>Index: sendchange.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/buildbot-try/sendchange.cgi,v
>retrieving revision 1.1
>diff -u -r1.1 sendchange.cgi
>--- sendchange.cgi	23 Jul 2007 21:30:35 -0000	1.1
>+++ sendchange.cgi	4 Oct 2007 14:42:54 -0000
>@@ -1,353 +1,129 @@
> #!/usr/bin/perl
>+#
>+# File: sendchange.cgi
>+# Author: Ben Hearsum
>+# Description:
>+#   This cgi script displays a simple form that allows a user to submit a diff
>+#   that will eventually be uploaded to a Buildbot master. It can also be used
>+#   to point a Buildbot master at a set of Mercurial repositories.
>+#   It generates a .info file that contains all necessary information for
>+#   Buildbot to produce a build with the requested patch or repositories.
>+#   processchanges.pl should be used to download these patches and send them
>+#   to Buildbot.

Ditto.

>+    if ($description eq "") {
>+        $description = 'No description given';
>     }

I think you want this test to be $description =~ /^\s*$/.

>+        binmode MOZCONFIG;
>+        while (<$mozconfigHandle>) {
>+            print MOZCONFIG;
>+        }
>+        close MOZCONFIG;

So I can sorta see why you might put the patch itself into binmode when you write it out (although, I could go either way on that... but I don't understand why you put the mozconfig into binmode; any reason?)

Comment 20

[Robert Helmer [:rhelmer]]

Comment on attachment 283545 [details] [diff] [review]
updates to processchange.pl and cgi form

What preed said :)

You probably don't need review from both of us for what it's worth.

I'll echo the sentiments like using MozBuild::RunShellCommand or something else that makes handling the shell easier, checking errors on opens and closes (and making sure to always close, so you can check for errors) e.g.:

open() || die();
close() || die();

Comment 21

[[e-mail] (:bhearsum)]

Attachment: address the concerns in the last review

OK. I've addressed all of your concerns in your last comment as well as rhelmer's (open() || die). I also made some changes to adhere better to the style guide (close FOO => close(FOO)). I've tested all of my changes locally and once a couple of IT bugs are RESO FIXED I can test it on the actual machines.

Comment 22

[[e-mail] (:bhearsum)]

Attachment: fix small bug in processchange.pl

Fixed a bug in processchanges.pl (It wasn't printing newlines when re-printing the INFO file)

Comment 23

[[e-mail] (:bhearsum)]

Comment on attachment 283751 [details] [diff] [review]
fix small bug in processchange.pl

There's problems with this one, too. I'm going to test this more thoroughly and post a new one next week.

Comment 24

[[e-mail] (:bhearsum)]

Attachment: addresses preed's concerns; works properly

As well as what's stated above, this version fixes the RunShellCommand arguments. (They were quoted before, and getting quoted a second time by RunShellCommand).

Comment 25

[J. Paul Reed [:preed]]

Comment on attachment 284166 [details] [diff] [review]
addresses preed's concerns; works properly


>+my @files = grep { /\w+\.info/ } readdir(DIR);

Anchor the grep(), i.e. grep { /^\w+\.info$/ } readdir(DIR);

>+        if ($rv->{'exitValue'} == 0 && index($rv->{'output'}, "NOT") == -1) {

It's good to get into the habit (weird though it may at first seem) to write these tests as:

if (0 == $rv->{'exitValue'}), reason being that it will catch errors like this: if ($rv->{'exitValue'} = 0). This is not a problem in Python, for instance, but in Perl, C, etc., it is.

+        if ($rv->{'exitValue'} == 0 && index($rv->{'output'}, "NOT") == -1) {
+            # sendchange succeeded
+            open(INFO, ">$infoFilename")
+              || die("Could not open info file: $file\n");
+            print INFO "processed: 1\n";
+            foreach $key (keys(%info)) {
+                print INFO "$key: $info{$key}\n";
+            }
+            close(INFO) || die("Coould not close info file: $file\n");
+            sleep($DELAY);
+        }
+        else {
+            # sendchange failed
+            print LOGFILE "Could not send change: $file\n";
+        }
+    }
+}

If I'm reading this correctly, there's a minor performance tweak you can make:

It looks like you're adding a value to each change file to see if it's been processed, and then skipping it if it has. That's fine (I like it better than using symlinks), however you might consider what opening a large number of patchfiles (say, 10,000) will do over time. It may make the script start to slow down. Just something to think about. If you print the information in the file out to a logfile, is it possible to just remove the file altogether?

Anyway, if you don't like that, one other thing to try: after the sendchange has succeeded, you can just open the file in append mode (i.e. ">>$infoFilename") and tack the "processed: 1" onto the bottom of it. Then you're not rewriting each file from scratch.

>+    if (exists $args{'patchLevel'} && $args{'patchLevel'} ne "") {
>+        $patchLevel = $args{'patchLevel'};
>+    }
>+    if (exists $args{'branch'} && $args{'branch'} ne "") {
>+        $branch = $args{'branch'};
>+    }
>+    if (exists $args{'identifier'} && $args{'identifier'} ne "") {
>+        $identifier = $args{'identifier'};
>+    }
>+    if (exists $args{'description'} && $args{'description'} ne "") {
>+        $description = $args{'description'};
>+    }
>+    if (exists $args{'type'} && $args{'type'} ne "") {
>+        $type = $args{'type'};
>+    }
>+    if (exists $args{'mozillaRepoPath'} && $args{'mozillaRepoPath'} ne "") {
>+        $mozillaRepoPath = $args{'mozillaRepoPath'};
>+    }
>+    if (exists $args{'tamarinRepoPath'} && $args{'tamarinRepoPath'} ne "") {
>+        $tamarinRepoPath = $args{'tamarinRepoPath'};
>+    }

I *think* I mentioned this before, but are a series of spaces valid input for these? If not, the test should really be:

if (exists($args{'blah'}) && $args{'blah'} !~ /^\s*$/)

>-        $filename = "$PATCH_DIR/$patchFile";
>+        my $filename = "$PATCH_DIR/$patchFile";

catfile()?

>+        $mozconfig = "$time-$key-$mozconfig";
>+        my $filename = "$PATCH_DIR/$mozconfig";

catfile()?

>-    close(INFO);
>+    close(INFO) || die("Could not close info file\n");

On *all* of the die()'s, you probably want to something like "Failure message: $!\n";

>Index: MozBuild/Util.pm
>===================================================================
>RCS file: MozBuild/Util.pm
>diff -N MozBuild/Util.pm
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ MozBuild/Util.pm	9 Oct 2007 16:28:24 -0000

You do *NOT* want to do this.

Did you make any changes to MozBuild::Util? If so, can you provide a diff for those changes?

If not, then you'll want to check out the copy of MozBuild that is in mozilla/tools/release *or* move it to a more common location (and still check it out from there).

But either way, there should not be a duplicate Util.pm in the tree. You can see the disaster that is our 18-copies-of-nsinstall.c for a reason why ;-)

I think you're really close here (in fact, the only reason I gave a hard r- was the Util.pm duplication).

I was looking back through my comments, and I've made some of them before (anchoring the grep(), using /^\s*$/ instead of "eq ''"), so you might want to look through those comments one last time.

Comment 26

[[e-mail] (:bhearsum)]

Attachment: [checked in] buildbot try, again

OK. I've gone through all of the previous comments and fixed a couple bugs I found. Changes in this version are:
* Use /^[\w.-]+\.info$/ to detect info files
* Ignore empty lines in info files
* Handle a 'type' other than 'patch' or 'hg' in processchanges.pl
* Use catfile instead of "$foo/$bar"
* Proper error checking on all open() and close()'s
* Whitespace is now an invalid form input.

I believe that covers everything.

Comment 27

[J. Paul Reed [:preed]]

Comment on attachment 284954 [details] [diff] [review]
[checked in] buildbot try, again


>+    open(INFOFILE, $infoFilename) ||
>+      die("Could not open info file: $file\nFailure message: $!\n");
>+    while (<INFOFILE>) {
>+        if ($_ !~ /^\s*$/) {

This is good; you might also want to do if ($_ !~ /^[\s#]*$/), so it would match a comment-like line, i.e. "# blah" (might be useful later, even though I know these are auto-generated...)

>+    if (exists $args{'tamarinRepoPath'} &&
>+      $args{'tamarinRepoPath'} ne "") {
>+        $tamarinRepoPath = $args{'tamarinRepoPath'};
>+    }

Did this one get missed in the (ne "") => !~ /^\s*$/) conversion?

Otherwise, looks really good. Nice work, Ben!

Comment 28

[[e-mail] (:bhearsum)]

RCS file: /cvsroot/mozilla/webtools/buildbot-try/processchanges.pl,v
done
Checking in processchanges.pl;
/cvsroot/mozilla/webtools/buildbot-try/processchanges.pl,v  <--  processchanges.pl
initial revision: 1.1
done
Removing processchanges.sh;
/cvsroot/mozilla/webtools/buildbot-try/processchanges.sh,v  <--  processchanges.sh
new revision: delete; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/buildbot-try/sendchange-ui.pm,v
done
Checking in sendchange-ui.pm;
/cvsroot/mozilla/webtools/buildbot-try/sendchange-ui.pm,v  <--  sendchange-ui.pm
initial revision: 1.1
done
Checking in sendchange.cgi;
/cvsroot/mozilla/webtools/buildbot-try/sendchange.cgi,v  <--  sendchange.cgi
new revision: 1.2; previous revision: 1.1
done

Comment 29

[[e-mail] (:bhearsum)]

Attachment: [checked in] put newlines in the logfile

This patch just adds a "\n" to the end of a logfile print.

Comment 30

[[e-mail] (:bhearsum)]

Comment on attachment 285102 [details] [diff] [review]
[checked in] put newlines in the logfile

Checking in processchanges.pl;
/cvsroot/mozilla/webtools/buildbot-try/processchanges.pl,v  <--  processchanges.pl
new revision: 1.2; previous revision: 1.1
done

Comment 31

[[e-mail] (:bhearsum)]

Attachment: one more small fix

When I brought the try server back up processchanges.pl wasn't working properly. It seems that turning on '-nc' stops wget from retrieving any files newer than the first oldest existing one. For the try server, that means it wasn't picking up uploaded patches.

This patch turns off '-nc' and instead rejects files ending in '.1', to prevent an endless amount of duplicate files.

Comment 32

[[e-mail] (:bhearsum)]

Attachment: try server buildbot configs

These are the buildbot configs currently used on the try server. I've added the mozconfig's, waterfall.css, and processchanges.pl. The idea is that you can check these files out into a buildbot master directory and have a functioning try server.

I'm not sure if you have time to get to this Rob, so change the r? if you see fit.

Comment 33

[Robert Helmer [:rhelmer]]

Comment on attachment 285359 [details] [diff] [review]
try server buildbot configs

Looks ok, only general comments I have:

* inline ''.join()s sure are ugly :P why do it that way? 
* there's a lot of copy/paste code in master.cfg, can some of this be moved to methods that take OS as argument?

Comment 34

[[e-mail] (:bhearsum)]

> * inline ''.join()s sure are ugly :P why do it that way? 

They are extremely ugly, I agree. The problem is that the WithProperties makes use of Python's %s string formatting. The PKG_BASENAME needs to be inserted into that string when the master.cfg is processed; but there needs to be a "%s" in the string at BuildStep runtime that WithProperties interpolates. (IIRC correctly I couldn't do "mozilla/%s/%%s-%s-mac.dmg" % (OBJDIR, PKG_BASENAME))

> * there's a lot of copy/paste code in master.cfg, can some of this be moved to
> methods that take OS as argument?

You're mainly talking about the packaging/upload steps, right? If so..yeah I guess I could've done that. When I began, it seemed better to keep it out, but maybe that's not the case now.

Comment 35

[[e-mail] (:bhearsum)]

Attachment: [checked in] same patch, plus the forgotten files

Sorry, I forgot a few files. This patch adds the buildbot/ directory. I had to locally override the Build class in order to make locks work more intuitively. Without these changes a "build started" e-mail is sent out when a build starts waiting on a lock, rather than when it actually starts building.

Comment 36

[[e-mail] (:bhearsum)]

Checking in master.cfg;
/cvsroot/mozilla/tools/buildbot-configs/tryserver/master.cfg,v  <--  master.cfg
new revision: 1.3; previous revision: 1.2
done
Checking in mozbuild.py;
/cvsroot/mozilla/tools/buildbot-configs/tryserver/mozbuild.py,v  <--  mozbuild.py
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/tools/buildbot-configs/tryserver/mozconfig-linux,v
done
Checking in mozconfig-linux;
/cvsroot/mozilla/tools/buildbot-configs/tryserver/mozconfig-linux,v  <--  mozconfig-linux
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/tools/buildbot-configs/tryserver/mozconfig-mac,v
done
Checking in mozconfig-mac;
/cvsroot/mozilla/tools/buildbot-configs/tryserver/mozconfig-mac,v  <--  mozconfig-mac
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/tools/buildbot-configs/tryserver/mozconfig-win32,v
done
Checking in mozconfig-win32;
/cvsroot/mozilla/tools/buildbot-configs/tryserver/mozconfig-win32,v  <--  mozconfig-win32
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/tools/buildbot-configs/tryserver/processchanges.pl,v
done
Checking in processchanges.pl;
/cvsroot/mozilla/tools/buildbot-configs/tryserver/processchanges.pl,v  <--  processchanges.pl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/tools/buildbot-configs/tryserver/waterfall.css,v
done
Checking in waterfall.css;
/cvsroot/mozilla/tools/buildbot-configs/tryserver/waterfall.css,v  <--  waterfall.css
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/tools/buildbot-configs/tryserver/buildbot/__init__.py,v
done
Checking in buildbot/__init__.py;
/cvsroot/mozilla/tools/buildbot-configs/tryserver/buildbot/__init__.py,v  <--  __init__.py
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/tools/buildbot-configs/tryserver/buildbot/process/__init__.py,v
done
Checking in buildbot/process/__init__.py;
/cvsroot/mozilla/tools/buildbot-configs/tryserver/buildbot/process/__init__.py,v  <--  __init__.py
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/tools/buildbot-configs/tryserver/buildbot/process/base.py,v
done
Checking in buildbot/process/base.py;
/cvsroot/mozilla/tools/buildbot-configs/tryserver/buildbot/process/base.py,v  <--  base.py
initial revision: 1.1
done

Comment 37

[J. Paul Reed [:preed]]

Comment on attachment 285127 [details] [diff] [review]
one more small fix

Sorry this r took so long; I didn't notice it for a bit ;-)

Looks fine to me, although if I understand where this is used, I'm wondering why these files can't somehow be deleted after they've been processed so you don't have dups lying around.

I might be missing something in the details, though; this approach seems fine.

Comment 38

[[e-mail] (:bhearsum)]

> Looks fine to me, although if I understand where this is used, I'm wondering
> why these files can't somehow be deleted after they've been processed so you
> don't have dups lying around.
> 

There probably isn't a huge need to keep them around after being processed, but I decided to err on the side of caution here. (eg. What happens if the script thinks the change is sent, but it isn't.)

Comment 39

[[e-mail] (:bhearsum)]

Checking in processchanges.pl;
/cvsroot/mozilla/webtools/buildbot-try/processchanges.pl,v  <--  processchanges.pl
new revision: 1.3; previous revision: 1.2
done

Comment 40

[[e-mail] (:bhearsum)]

Moving try server bugs to the new component.