342767 - ability for tinderbox client to pull build date from quickparse output

Status: RESOLVED FIXED

(Webtools Graveyard :: Tinderbox, defect)

Description

[Robert Helmer [:rhelmer]]

Tinderbox clients needs to be able to request the latest build date for a particular build, and get the build date (for testing purposes). This makes it easier to correlate build to test results, as well as enable the possibility of appending to the build log.

Comment 1

[Robert Helmer [:rhelmer]]

Attachment: pull build date from tinderbox

Comment 2

[J. Paul Reed [:preed]]

Comment on attachment 227118 [details] [diff] [review]
pull build date from tinderbox 

> my $tbox_server_info = `wget -qO - \'$Settings::TinderboxServerURL\'`;

run_shell_command()?

Nitpicks: I'm not a huge fan of unless(), because it makes my brain always have to invert the logic.

Also, I would use "tinderbox.mozilla.org for the default value.

Comment 3

[timeless]

this is actually the wrong product. the tools/tinderbox stuff isn't part of the tinderbox server which is the component you've picked (really the whole product is generally about server stuff, not client stuff).

Comment 4

[Robert Helmer [:rhelmer]]

(In reply to comment #3)
> this is actually the wrong product. the tools/tinderbox stuff isn't part of the
> tinderbox server which is the component you've picked (really the whole product
> is generally about server stuff, not client stuff).
> 

This occurred to me too.. we have "Tinderbox Platforms" and "Tinderbox Configuration" under mozilla.org, I didn't see anything else appropriate. Do you have a suggestion for a more proper product?

Comment 5

[Robert Helmer [:rhelmer]]

(In reply to comment #2)
> (From update of attachment 227118 [details] [diff] [review] [edit])
> > my $tbox_server_info = `wget -qO - \'$Settings::TinderboxServerURL\'`;
> 
> run_shell_command()?
> 

The patch that allows grabbing output hasn't landed, I think it's too late in the 1505 release cycle for that now.

> Nitpicks: I'm not a huge fan of unless(), because it makes my brain always have
> to invert the logic.
> 
> Also, I would use "tinderbox.mozilla.org for the default value.

Ok, I inverted the first set of "if" statements, there are a few "unless" statements still but I think they make sense (unless you prefer "if not" for some reason :) )

I'll attach a patch in a sec.

Comment 6

[Robert Helmer [:rhelmer]]

Attachment: pull build date from tinderbox

Comment 7

[Robert Helmer [:rhelmer]]

Comment on attachment 227296 [details] [diff] [review]
pull build date from tinderbox 

needs better error checking, redoing.

Comment 8

[Robert Helmer [:rhelmer]]

Attachment: pull build date from tinderbox, better error checking

Note that this does a die() because tinderbox's error checking mechanism isn't available at this point in the build process. This info is needed for the build start email, and error checking does not begin until after the email has been sent.

Comment 9

[J. Paul Reed [:preed]]

Comment on attachment 227312 [details] [diff] [review]
pull build date from tinderbox, better error checking

Coalese Settings::FetchBuildInfo and Settings::DownloadBuild into Settings::TestOnlyTinderbox; use that variable to control execution of the logic; then use all the sub-variables.

use print_log instead of print() here.

One nit: if (0 != ($? >> 8)) { # please.

Other than that, r=preed.

Comment 10

[Robert Helmer [:rhelmer]]

Attachment: uses TestOnlyTinderbox and print_log

Comment 11

[J. Paul Reed [:preed]]

Comment on attachment 227474 [details] [diff] [review]
uses TestOnlyTinderbox and print_log

r=preed

Comment 12

[Robert Helmer [:rhelmer]]

landed on trunk
--
new revision: 1.322; previous revision: 1.321
done
Checking in tinder-defaults.pl;
/cvsroot/mozilla/tools/tinderbox/tinder-defaults.pl,v  <--  tinder-defaults.pl
new revision: 1.96; previous revision: 1.95
done

Comment 13

[Robert Helmer [:rhelmer]]

Attachment: pull build date from tinderbox only for graph server

In light of changing requirements :) we only currently need this for the graph server report, and not the tinderbox report.

Comment 14

[Robert Helmer [:rhelmer]]

Attachment: use previous start_time correctly

Comment 15

[Robert Helmer [:rhelmer]]

Attachment: only pull graph_time once, and fall back to start_time

Found one more wrinkle.. $graph_time should only be pulled once per run, at the beginning, and tinderbox should fall back to start_time (not time()!) if it cannot fetch anything from the server.

Comment 16

[Robert Helmer [:rhelmer]]

Attachment: address preed's comments on last patch

Comment 17

[J. Paul Reed [:preed]]

Comment on attachment 227763 [details] [diff] [review]
address preed's comments on last patch

Ship it.

Comment 18

[Robert Helmer [:rhelmer]]

(In reply to comment #17)
> (From update of attachment 227763 [details] [diff] [review] [edit])
> Ship it.
> 

Landed:

Checking in build-seamonkey-util.pl;
/cvsroot/mozilla/tools/tinderbox/build-seamonkey-util.pl,v  <--  build-seamonkey-util.pl
new revision: 1.324; previous revision: 1.323
done

Comment 19

[Andrew Schultz]

Comment on attachment 227763 [details] [diff] [review]
address preed's comments on last patch

>+my $graph_time;

...

>+            my $graph_time = get_build_time_from_server($start_time);

Doesn't this value get dropped on the floor because it's redeclared as a local variable?  Shouldn't it also be set for the non-test-only case?

Comment 20

[Robert Helmer [:rhelmer]]

(In reply to comment #19)
> (From update of attachment 227763 [details] [diff] [review] [edit])
> >+my $graph_time;
> 
> ...
> 
> >+            my $graph_time = get_build_time_from_server($start_time);
> 
> Doesn't this value get dropped on the floor because it's redeclared as a local
> variable?  Shouldn't it also be set for the non-test-only case?
> 

Good catch, thanks! Why would this be useful in the non-test-only case? If this is not a test-only tinderbox, then we want to use the real build start time when reporting to the graph server. 

Comment 21

[Robert Helmer [:rhelmer]]

Attachment: do not clobber global $graph_time override variable

Comment 22

[Andrew Schultz]

graph_time needs to be set for the non-test-only case because it still gets used for that case.

-    my $time = POSIX::strftime "%Y:%m:%d:%H:%M:%S", localtime;
+    my $time = POSIX::strftime("%Y:%m:%d:%H:%M:%S", localtime($graph_time));

My tbox scripts complain about use of uninitialized variable because of this.

Comment 23

[Robert Helmer [:rhelmer]]

(In reply to comment #22)
> graph_time needs to be set for the non-test-only case because it still gets
> used for that case.
> 
> -    my $time = POSIX::strftime "%Y:%m:%d:%H:%M:%S", localtime;
> +    my $time = POSIX::strftime("%Y:%m:%d:%H:%M:%S", localtime($graph_time));
> 
> My tbox scripts complain about use of uninitialized variable because of this.
> 

Hmm actually it shouldn't be used here at all, I'll submit a proper patch. Sorry about that.

Comment 24

[Robert Helmer [:rhelmer]]

Attachment: correct use of $graph_time

* only use $graph_time for TestOnlyTinderbox
* use $graph_time for submitting data

Comment 25

[J. Paul Reed [:preed]]

Comment on attachment 231955 [details] [diff] [review]
correct use of $graph_time

k; r=preed.

Comment 26

[Robert Helmer [:rhelmer]]

Landed on trunk:
/cvsroot/mozilla/tools/tinderbox/build-seamonkey-util.pl,v  <--  build-seamonkey-util.pl
new revision: 1.332; previous revision: 1.331
done