Tinderbox should only block out the time on the chart that the build actually took

RESOLVED FIXED

Status

Webtools Graveyard
Tinderbox
--
blocker
RESOLVED FIXED
12 years ago
4 years ago

People

(Reporter: justdave, Assigned: morgamic)

Tracking

Details

Attachments

(8 attachments, 6 obsolete attachments)

1.82 KB, text/plain
Details
1.33 KB, patch
cls
: review+
Details | Diff | Splinter Review
940 bytes, patch
cls
: review-
Details | Diff | Splinter Review
714 bytes, patch
cls
: review-
Details | Diff | Splinter Review
1.08 KB, patch
cls
: review-
Details | Diff | Splinter Review
9.61 KB, patch
bear
: review+
Details | Diff | Splinter Review
2.85 KB, patch
bear
: review+
Details | Diff | Splinter Review
1.53 KB, patch
bear
: review+
Details | Diff | Splinter Review
Currently, tinderbox will block out the entire time between the start of one build and the start of the next one in the same tree as a single build on the build charts.  Instead, it should only block out the time the build actually took (some of the machines will build 3 or 4 different trees before getting back to the same one again), and leave a gray box on the timespan that the box wasn't actually building that tree.

This is already implemented in production, but it's never been checked into CVS, and I can't find a bug for it.

Marking as a blocker, because the patch is huge, and its existence in production is preventing us from doing any other upgrades to production because of the number of conflicts with other patches.
I also can't post a clean patch right now because there's a bunch of other things that have been applied on top of it in production already, and they all need to be sorted out from each other first.

Comment 2

12 years ago
This bug slipped by me when it was first filed.  Would you like my help to fix it?
(In reply to comment #2)
> This bug slipped by me when it was first filed.  Would you like my help to fix
> it?

Please do. It's blocking bug 342170 (and others) from being fixed.

Comment 4

12 years ago
(In reply to comment #3)
> Please do. It's blocking bug 342170 (and others) from being fixed.

To fix it I need access to the Tinderbox installation on mecha.  I used to have root there, though I probably don't anymore.  Maybe I still have it, I don't know, I haven't tried to get on mecha since my last day at MoCo.  It would make things go a lot faster if I had that capability to access the system.  Can someone check and let me know?
heh, reed's being a bit presumptuous there, but yeah, if you want to help with it, that'd be awesome.  I've got a copy of it on chameleon, and I think you already have an account there, except chameleon's down for a power reroute right now - I'll get it copied where you can get at it and post it here as soon as it's up again.
OK, chameleon is up, you indeed do still have an account there (I thought you did), and I just dropped a copy of production (minus some of the bigger trees' data to save on disk space) into your home directory.
(Assignee)

Comment 7

12 years ago
Chase, any progress on this?  I'm working on 342170 to try to get things merged w/ CVS -- let me know what I can do to help. :)

Comment 8

12 years ago
I have 10 patches separated out of production Tinderbox.  I'll be attaching them here.

Comment 9

12 years ago
Created attachment 228364 [details]
CVS/Entries from production Tinderbox

All patches to be attached apply cleanly against the revisions described in this Entries file.

Comment 10

12 years ago
Created attachment 228365 [details] [diff] [review]
display real end times for builds in the waterfall display

I authored this.  Set $new_fangled_display to 0 to enable the old display behavior.

Comment 11

12 years ago
Created attachment 228366 [details] [diff] [review]
prophylactic fixes for secure Tinderbox rigging

I authored this.  Notes:

  * scrubs supplied tree names of unexpected characters
  * allows graceful error handling of bad supplied tree names
  * allows +'s in usernames to be escaped correctly by JS
  * while we're touching the pop-up, implement a suggestion from Hixie for
    opacity

Review should focus on making scrubbing as rigorous as possible, especially.

Comment 12

12 years ago
Created attachment 228367 [details] [diff] [review]
implement wayback links for Tinderbox (checked in)

I authored this.  It adds links at the bottom of each Tinderbox tree's page that allow viewers to jump farther back in time than just 12/24 hours -- I found that extremely useful for forensic investigation.

The original code contained a pointer to mail me with comments and suggestions about Tinderbox, hence the liberty with the graffiti.

Comment 13

12 years ago
Created attachment 228368 [details] [diff] [review]
require only one tree

I didn't author this.  Justdave recalls hearing Timeless seek reviews, suggesting Timeless authored it.

I'll let Timeless describe it.

Comment 14

12 years ago
Created attachment 228369 [details] [diff] [review]
point to gzip's correct location (for mecha?)

I didn't author this.  I recall hearing Justdave mention he made this change.

Comment 15

12 years ago
Created attachment 228371 [details] [diff] [review]
(guess) fix static page creation

I didn't author this.  My guess is that it fixes static page creation for some (or all) cases.  I hated static page creation because it wouldn't work 100% of the time.  Maybe this fixes the broken cases.  Maybe I can stop hating.

Comment 16

12 years ago
Created attachment 228372 [details] [diff] [review]
point at correct CVSROOT location

I didn't author this, but the change was probably long overdue.

Comment 17

12 years ago
Created attachment 228373 [details] [diff] [review]
take more care with new notes (modified fix by bug 223304)

I didn't author this.  It adds grace to Tinderbox's notetaking.

Comment 18

12 years ago
Created attachment 228374 [details] [diff] [review]
fix nuke 'feature' (fixed by bug 340448)

I didn't author this.  This allows the nuke admin feature to remove gzipped log files, too.  Before this patch, nuke would only remove entries from build.dat, leaving the relatively huge gzipped log files!

Comment 19

12 years ago
Created attachment 228375 [details] [diff] [review]
leftover - set query_branch_head (fixed by bug 323369)

I didn't author this.  I suspect it's the beginning of someone's larger multi-axial Tinderbox display (eg. view this branch, view this platform).  It appears to have no effect in production.

Comment 20

12 years ago
Now that all 10 patches are out of production, the process of merging them back into CVS begins.  First, we'll wait for reviews.

Comment 21

12 years ago
Note: it's possible/likely some of the later patches are already in CVS, so we should check that out, too.  Otherwise we'll waste time reviewing patches that are already checked in but were applied to production by hand.

Updated

12 years ago
Attachment #228374 - Attachment description: fix nuke 'feature' → fix nuke 'feature' (fixed by bug 340448)
Attachment #228374 - Attachment is obsolete: true

Updated

12 years ago
Attachment #228375 - Attachment description: leftover - set query_branch_head → leftover - set query_branch_head (fixed by bug 323369)
Attachment #228375 - Attachment is obsolete: true

Updated

12 years ago
Attachment #228373 - Attachment description: take more care with new notes → take more care with new notes (modified fix by bug 223304)
Attachment #228373 - Attachment is obsolete: true

Updated

12 years ago
Attachment #228372 - Flags: review+

Comment 22

12 years ago
Comment on attachment 228369 [details] [diff] [review]
point to gzip's correct location (for mecha?)

We should really be using the Makefile to substitute those values.
Attachment #228369 - Flags: review+

Updated

12 years ago
Attachment #228367 - Flags: review+

Comment 23

12 years ago
Comment on attachment 228367 [details] [diff] [review]
implement wayback links for Tinderbox (checked in)

For wayback links:

Checking in showbuilds.cgi;
/cvsroot/mozilla/webtools/tinderbox/showbuilds.cgi,v  <--  showbuilds.cgi
new revision: 1.190; previous revision: 1.189
done
Attachment #228367 - Attachment description: implement wayback links for Tinderbox → implement wayback links for Tinderbox (checked in)

Comment 24

12 years ago
Created attachment 228429 [details] [diff] [review]
endtimes v1.1 (checked in)

Updated end-times patch against the current trunk.  Changed $use_new_fangled_display to $display_accurate_build_end_times and added a description.
Attachment #228365 - Attachment is obsolete: true
Attachment #228429 - Flags: review?(bear)

Comment 25

12 years ago
Comment on attachment 228429 [details] [diff] [review]
endtimes v1.1 (checked in)

>Index: webtools/tinderbox/tbglobals.pl
>@@ -80,9 +84,10 @@
> 
> sub print_time {
>   my ($t) = @_;
>-  my ($minute,$hour,$mday,$mon);
>-  (undef,$minute,$hour,$mday,$mon,undef) = localtime($t);
>-  sprintf("%02d/%02d&nbsp;%02d:%02d",$mon+1,$mday,$hour,$minute);
>+  my ($sec,$minute,$hour,$mday,$mon);
>+  ($sec,$minute,$hour,$mday,$mon,undef) = localtime($t);
>+  sprintf("%02d/%02d&nbsp;%02d:%02d:%02d",$mon+1,$mday,$hour,$minute,$sec);
>+  #sprintf("%02d/%02d&nbsp;%02d:%02d",$mon+1,$mday,$hour,$minute);

I didn't catch this in my work yesterday: the commented line should be removed in the new revision.

Comment 26

12 years ago
Comment on attachment 228429 [details] [diff] [review]
endtimes v1.1 (checked in)

+1 with the change that Chase references in comment #25 above
Attachment #228429 - Flags: review?(bear) → review+

Comment 27

12 years ago
Comment on attachment 228429 [details] [diff] [review]
endtimes v1.1 (checked in)

Endtimes v1.1 checked in with recommended change:

Checking in webtools/tinderbox/showbuilds.cgi;
/cvsroot/mozilla/webtools/tinderbox/showbuilds.cgi,v  <--  showbuilds.cgi
new revision: 1.191; previous revision: 1.190
done
Checking in webtools/tinderbox/tbglobals.pl;
/cvsroot/mozilla/webtools/tinderbox/tbglobals.pl,v  <--  tbglobals.pl
new revision: 1.36; previous revision: 1.35
done
Attachment #228429 - Attachment description: endtimes v1.1 → endtimes v1.1 (checked in)

Comment 28

12 years ago
Comment on attachment 228371 [details] [diff] [review]
(guess) fix static page creation

Afaict, this patch does nothing as showbuilds.cgi does not parse @ARGV.  ENV{QUERY_STRING} is used by split_cgi_args() in tbglobals.pl to parse the args if showbuilds.cgi is not called by HTTP POST request.
Attachment #228371 - Flags: review-
Hey guys,

What is left? Are there still patches that need review?

Let me know if I can help out. I have a really small change I need to get in to get the test-only boxes going.

Comment 30

12 years ago
The prophylactic fixes and the one tree patches are all that are left but they have some major conflicts against the tip of the trunk.  I'm still not sure how necessary the one tree patch is.  The gzip & CVSROOT patches aren't necessary if you're using 'make install' to install tinderbox which is now required.  

Comment 31

12 years ago
Created attachment 229600 [details] [diff] [review]
onetree v1.1 (checked in)

Per timeless, there's one mode of showbuilds.cgi, quickparse, that supports passing in multiple trees.  The rest of the modes take a single tree.  This patch brings up the "chooser" if you try to pass multiple trees to anything but quickparse.
Attachment #228368 - Attachment is obsolete: true
Attachment #229600 - Flags: review?(bear)

Comment 32

12 years ago
(In reply to comment #31)
> Created an attachment (id=229600) [edit]
> onetree v1.1
> 
> Per timeless, there's one mode of showbuilds.cgi, quickparse, that supports
> passing in multiple trees.  The rest of the modes take a single tree.  This
> patch brings up the "chooser" if you try to pass multiple trees to anything but
> quickparse.
> 

My concern is for the rdf version as it is most likely going to be consumed by a feed reader and not a person.  Will popping up a form that requires input break expected behaviour?

Comment 33

12 years ago
(In reply to comment #32)
> My concern is for the rdf version as it is most likely going to be consumed by
> a feed reader and not a person.  Will popping up a form that requires input
> break expected behaviour?

cls on IRC just answered my question - the current behaviour would be returning an error page already for a rdf request so no change in behaviour will happen. 

Updated

12 years ago
Attachment #229600 - Flags: review?(bear) → review+

Comment 34

12 years ago
Comment on attachment 229600 [details] [diff] [review]
onetree v1.1 (checked in)

onetree v1.1:

/cvsroot/mozilla/webtools/tinderbox/showbuilds.pl,v  <--  showbuilds.pl
new revision: 1.3; previous revision: 1.2
done
Attachment #229600 - Attachment description: onetree v1.1 → onetree v1.1 (checked in)

Comment 35

12 years ago
Created attachment 229608 [details] [diff] [review]
misc cleanup v1.0

With the onetree patch, there's no need for the additional checks in the prophylactic patch.  The $form{tree} has already been sanitized by the time tb_load_data & friends are called.  Here are the remaining misc changes from that patch.
Attachment #228366 - Attachment is obsolete: true
Attachment #229608 - Flags: review?(bear)

Updated

12 years ago
Attachment #229608 - Flags: review?(bear) → review+

Comment 36

12 years ago
That's it.  We're done here.  The path changes will break 'make install' (so I should probably cancel their reviews).  At some point, someone will need to make a copy of the live tinderbox dir, 'cvs up' it, remove the conflicts and then run 'make install'.


misc cleanup v1.0:

Checking in webtools/tinderbox/showbuilds.pl;
/cvsroot/mozilla/webtools/tinderbox/showbuilds.pl,v  <--  showbuilds.pl
new revision: 1.4; previous revision: 1.3
done
Checking in webtools/tinderbox/tbglobals.pl;
/cvsroot/mozilla/webtools/tinderbox/tbglobals.pl,v  <--  tbglobals.pl
new revision: 1.38; previous revision: 1.37
done
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Attachment #228369 - Flags: review+ → review-

Updated

12 years ago
Attachment #228372 - Flags: review+ → review-
Product: Webtools → Webtools Graveyard
You need to log in before you can comment on or make changes to this bug.