Bug 25517 – build-webkit script should print build time at end

• Component: Tools / Tests
• Status: VERIFIED FIXED

[Jeff Johnson]

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.

[Laurent Cerveau]

Created attachment 38883 [details]
Patch for bug 25517 : build-webkit script should print build time at end

N/A

[David Kilzer (:ddkilzer)]

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:

> [phone]  Laurent Cerveau  <[e-mail]>
> +
> +        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!

[Laurent Cerveau]

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?).

[Laurent Cerveau]

Created attachment 38923 [details]
Patch for bug 25517 revision 2: use of subroutine for time computation, coding guidelines

[David Kilzer (:ddkilzer)]

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!

[Laurent Cerveau]

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..

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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. :)

[Eric Seidel (no email)]

Welcome to WebKit, btw!

[David Kilzer (:ddkilzer)]

Comment on attachment 38940 [details]
Patch for bug 25517 revision 3: better coding guideline, nice formatting for build time

Great!  r=me

[Eric Seidel (no email)]

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>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Jeff Johnson]

Thanks! I've verified this patch. Now I know that I need to buy a much faster computer.