All users were logged out of Bugzilla on October 13th, 2018

ability for tinderbox client to pull build date from quickparse output

RESOLVED FIXED

Status

RESOLVED FIXED
13 years ago
4 years ago

People

(Reporter: rhelmer, Assigned: rhelmer)

Tracking

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Assignee)

Description

13 years ago
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.
(Assignee)

Comment 1

13 years ago
Created attachment 227118 [details] [diff] [review]
pull build date from tinderbox
Assignee: morgamic → rhelmer
Status: NEW → ASSIGNED
Attachment #227118 - Flags: review?(preed)

Comment 2

13 years ago
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.
Attachment #227118 - Flags: review?(preed) → review-

Comment 3

13 years ago
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).
(Assignee)

Comment 4

13 years ago
(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?
(Assignee)

Comment 5

13 years ago
(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.
(Assignee)

Comment 6

13 years ago
Created attachment 227296 [details] [diff] [review]
pull build date from tinderbox
Attachment #227118 - Attachment is obsolete: true
Attachment #227296 - Flags: review?(preed)
(Assignee)

Comment 7

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

needs better error checking, redoing.
Attachment #227296 - Flags: review?(preed)
(Assignee)

Comment 8

13 years ago
Created attachment 227312 [details] [diff] [review]
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.
Attachment #227296 - Attachment is obsolete: true
Attachment #227312 - Flags: review?(preed)

Comment 9

13 years ago
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.
Attachment #227312 - Flags: review?(preed) → review+
(Assignee)

Comment 10

13 years ago
Created attachment 227474 [details] [diff] [review]
uses TestOnlyTinderbox and print_log
Attachment #227474 - Flags: review?(preed)
(Assignee)

Updated

13 years ago
Attachment #227312 - Attachment is obsolete: true
Comment on attachment 227474 [details] [diff] [review]
uses TestOnlyTinderbox and print_log

r=preed
Attachment #227474 - Flags: review?(preed) → review+
(Assignee)

Comment 12

13 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Updated

13 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 13

13 years ago
Created attachment 227608 [details] [diff] [review]
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.
Attachment #227474 - Attachment is obsolete: true
Attachment #227608 - Flags: review?(preed)
(Assignee)

Comment 14

13 years ago
Created attachment 227644 [details] [diff] [review]
use previous start_time correctly
Attachment #227608 - Attachment is obsolete: true
Attachment #227644 - Flags: review?(preed)
Attachment #227608 - Flags: review?(preed)
(Assignee)

Comment 15

13 years ago
Created attachment 227732 [details] [diff] [review]
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.
Attachment #227644 - Attachment is obsolete: true
Attachment #227732 - Flags: review?(preed)
Attachment #227644 - Flags: review?(preed)
(Assignee)

Comment 16

13 years ago
Created attachment 227763 [details] [diff] [review]
address preed's comments on last patch
Attachment #227732 - Attachment is obsolete: true
Attachment #227763 - Flags: review?(preed)
Attachment #227732 - Flags: review?(preed)
Comment on attachment 227763 [details] [diff] [review]
address preed's comments on last patch

Ship it.
Attachment #227763 - Flags: review?(preed) → review+
(Assignee)

Comment 18

13 years ago
(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
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED

Comment 19

12 years ago
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?
(Assignee)

Comment 20

12 years ago
(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. 
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 21

12 years ago
Created attachment 231923 [details] [diff] [review]
do not clobber global $graph_time override variable
Attachment #231923 - Flags: review?(preed)

Comment 22

12 years ago
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.
(Assignee)

Comment 23

12 years ago
(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.
(Assignee)

Comment 24

12 years ago
Created attachment 231955 [details] [diff] [review]
correct use of $graph_time

* only use $graph_time for TestOnlyTinderbox
* use $graph_time for submitting data
Attachment #227763 - Attachment is obsolete: true
Attachment #231923 - Attachment is obsolete: true
Attachment #231955 - Flags: review?(preed)
Attachment #231923 - Flags: review?(preed)
Comment on attachment 231955 [details] [diff] [review]
correct use of $graph_time

k; r=preed.
Attachment #231955 - Flags: review?(preed) → review+
(Assignee)

Comment 26

12 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago12 years ago
Resolution: --- → FIXED
Product: Webtools → Webtools Graveyard
You need to log in before you can comment on or make changes to this bug.