Bug 25517 - build-webkit script should print build time at end
Summary: build-webkit script should print build time at end
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-02 01:17 PDT by opendarwin
Modified: 2010-03-10 21:33 PST (History)
3 users (show)

See Also:


Attachments
Patch for bug 25517 : build-webkit script should print build time at end (2.07 KB, patch)
2009-09-01 13:32 PDT, Laurent Cerveau
ddkilzer: review-
Details | Formatted Diff | Diff
Patch for bug 25517 revision 2: use of subroutine for time computation, coding guidelines (2.33 KB, patch)
2009-09-02 04:47 PDT, Laurent Cerveau
ddkilzer: review-
Details | Formatted Diff | Diff
Patch for bug 25517 revision 3: better coding guideline, nice formatting for build time (2.41 KB, patch)
2009-09-02 13:37 PDT, Laurent Cerveau
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description opendarwin 2009-05-02 01:17:15 PDT
Unless you have a supercomputer, WebKit takes a long time to build. Thus, it would be both interesting and useful if the build-webkit script finished by printing the amount of time it took to run.
Comment 1 Laurent Cerveau 2009-09-01 13:32:17 PDT
Created attachment 38883 [details]
Patch for bug 25517 : build-webkit script should print build time at end

N/A
Comment 2 David Kilzer (:ddkilzer) 2009-09-01 21:11:52 PDT
Comment on attachment 38883 [details]
Patch for bug 25517 : build-webkit script should print build time at end

This is a great first patch!  Some comments below:

> +2009-09-01  Laurent Cerveau  <lcerveau@me.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +         <http://webkit.org/b/25517> build-webkit script should print build time at end
> +
> +        * Scripts/build-webkit:
> +		Added startTime and endTime variable so that the build time is computed and printed as
> +		part of the build message. Proper display formatting (hours, min, secs).

Lines should be indented by exactly 8 spaces.  There appears to be 9 spaces or 2 tabs used above.

> +my $endTime = time;
> +my $buildTime = $endTime - $startTime;
> +
> +my $buildHours = ($buildTime - $buildTime % 3600)/3600;
> +my $buildMins = (($buildTime - $buildHours*3600) - ($buildTime - $buildHours*3600) %60)/60;
> +my $buildSecs = $buildTime - $buildHours*3600 - $buildMins*60;

This logic would be best in it's own function that takes ($endTime - $startTime) as input and returns a formatted string.  Maybe call it formatBuildTime().

I would also leave off hour calculations and only calculate minutes and seconds.  Most modern hardware builds WebCore in much less than an hour.

Please add spaces before and after all operators (*, %, /, +, -).

> +
> +

There should only be one blank line here.

>  print "\n";
>  print "===========================================================\n";
> -print " WebKit is now built. To run $launcherName with this newly-built\n";
> -print " code, use the \"$launcherPath\" script.\n";
> +print " WebKit is now built ($buildHours:$buildMins:$buildSecs). \n";
> +print " To run $launcherName with this newly-built code, use the\n";
> +print " \"$launcherPath\" script.\n";
>  print "===========================================================\n";

This is fine, but a single $buildTime string from formatBuildTime() would replace $buildHours:$buildMins:$buildSecs.

r- to fix the above issues.  Thanks!
Comment 3 Laurent Cerveau 2009-09-02 04:45:21 PDT
Here is a second version of the patch. A few comments
- I did keep the possibility to display the hour but only if it is superior to 1 (on my Macbook , it namely is sometimes at the limit)
- I did put the subRoutine in the build-webkit script. Not sure however that this is where it makes the most sense (maybe a separate utilities file?).
Comment 4 Laurent Cerveau 2009-09-02 04:47:19 PDT
Created attachment 38923 [details]
Patch for bug 25517 revision 2: use of subroutine for time computation, coding guidelines
Comment 5 David Kilzer (:ddkilzer) 2009-09-02 07:10:19 PDT
Comment on attachment 38923 [details]
Patch for bug 25517 revision 2: use of subroutine for time computation, coding guidelines

The ChangeLog looks good!  I think one more patch should do it.  Most of the issues are simply following coding style used in other Perl scripts in the WebKit project.  (Sorry, I missed some of these on the first review.)

> +my $startTime = time;

Since 'time' here is a subroutine call, please add empty parenthesis:  time()

> +# Formatting routine to display build time

The comment isn't necessary; I think the name of the subroutine is self-explanatory.

> +sub formatBuildTime
> +{

New subroutines added to Perl script should have the number of parameters declared:

sub formatBuildTime($)
{

The subroutine itself should appear (alphabetically--although this would be the first one) after the  last "exit 0;" statement in the file.

Also, this subroutine should be declared at the top of the script with its parameter list (it should come after the 'use' statements but before any global variables):

 use POSIX;
 
+sub formatBuildTime($);
 
 my $originalWorkingDirectory = getcwd();


> +    my $buildTime = $_[0];

This is normally written using the @_ array in other Perl scripts:

    my ($buildTime) = @_;

> +    my $buildHours = ($buildTime - $buildTime % 3600) / 3600;

This would be easier to read using:

    my $buildHours = int($buildTime / 3600);

> +    my $buildMins = (($buildTime - $buildHours * 3600) - ($buildTime - $buildHours*3600) % 60) / 60;

Similarly:

    my $buildMins = int(($buildTime - $buildHours * 3600) / 60)

> +    if ($buildHours) {
> +        my $result = $buildHours,':',$buildMins,':',$buildSecs;
> +    } else {
> +        my $result = $buildMins,':',$buildSecs;
> +    }

A few items here:

- WebKit usually prefers to use an "early return" instead of an if/else block in these cases.
- WebKit prefers explicit return statements for values.  This code uses implicit return values, e.g., the last statement executed, and declares a $result variable that's never really used.
- The formatBuildTime() subroutine should really return a string instead of a list of strings.
- The minutes and seconds values should be zero-padded so you have times like "1:01:05" instead of "1:1:5".
- I think it would be nice to add "h", "m" and "s" suffixes to the time values to make them easier to read:  "18m:02s", "1h:01m:05s".

Here's how that code might look:

    if ($buildHours) {
        return sprintf("%dh:%02dm:%02ds", $buildHours, $buildMins, $buildSecs);
    }
    return sprintf("%02dm:%02ds", $buildMins, $buildSecs);

> +my $endTime = time;

This should use "time()" since it's a subroutine call.

> -print " WebKit is now built. To run $launcherName with this newly-built\n";
> -print " code, use the \"$launcherPath\" script.\n";
> +print " WebKit is now built (", &formatBuildTime($endTime - $startTime), "). \n";
> +print " To run $launcherName with this newly-built code, use the\n";
> +print " \"$launcherPath\" script.\n";

Instead of calling the subroutine inside the print statement, let's set a variable outside the "print" statements (no need to use the '&' modifier here to invoke the subroutine):

my $buildTime = formatBuildTime($endTime - $startTime);

Then we can just use the variable in the print statement (like the others already do):

> +print " WebKit is now built ($buildTime). \n";

r- for the remaining issues.  Thanks!
Comment 6 Laurent Cerveau 2009-09-02 13:37:56 PDT
Created attachment 38940 [details]
Patch for bug 25517 revision 3: better coding guideline, nice formatting for build time

Thanks for the comments! (Very useful learning experience). All should be here now..
Comment 7 Eric Seidel (no email) 2009-09-02 14:01:54 PDT
Comment on attachment 38940 [details]
Patch for bug 25517 revision 3: better coding guideline, nice formatting for build time

Patch was not marked as a patch (checkbox). :)  In the future you might consider using tools like 'bugzilla-tool post-diff 25517' which will automatically post diffs to bugs and mark them as patches and mark them as needing review.
Comment 8 Eric Seidel (no email) 2009-09-02 14:03:45 PDT
Comment on attachment 38940 [details]
Patch for bug 25517 revision 3: better coding guideline, nice formatting for build time

it seems a little silly that we have support for printing hours.  If a build ever takes an hour, I'd be concerned. :)  If it did, it might be just as clear to say "61 minutes" instead of 1 hr 1 minute. :)
Comment 9 Eric Seidel (no email) 2009-09-02 14:08:25 PDT
Welcome to WebKit, btw!
Comment 10 David Kilzer (:ddkilzer) 2009-09-02 19:18:53 PDT
Comment on attachment 38940 [details]
Patch for bug 25517 revision 3: better coding guideline, nice formatting for build time

Great!  r=me
Comment 11 Eric Seidel (no email) 2009-09-02 19:31:49 PDT
Comment on attachment 38940 [details]
Patch for bug 25517 revision 3: better coding guideline, nice formatting for build time

Clearing flags on attachment: 38940

Committed r48004: <http://trac.webkit.org/changeset/48004>
Comment 12 Eric Seidel (no email) 2009-09-02 19:31:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 opendarwin 2009-09-03 19:13:49 PDT
Thanks! I've verified this patch. Now I know that I need to buy a much faster computer.