[webkit-reviews] review granted: [Bug 135400] [Win] Modify version numbering scheme to support 5-tuple versions : [Attachment 235722] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 135400: [Win] Modify version numbering scheme to support 5-tuple versions
https://bugs.webkit.org/show_bug.cgi?id=135400

Attachment 235722: Patch
https://bugs.webkit.org/attachment.cgi?id=235722&action=review

------- Additional Comments from David Kilzer (:ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=235722&action=review


r=me with a few fixes per the notes.

> Tools/Scripts/test-webkitperl:55
> -my $pattern = "Tools/Scripts/webkitperl/*_unittest/*.pl";
> +my $pattern = "Tools/Scripts/webkitperl/auto-version_unittest/*.pl";

I don't think you want to check this in.  You probably made this change just to
run the auto-version tests while you were fixing the patch.

> WebKitLibraries/win/tools/scripts/auto-version.pl:123
> +    die "Second version component ($second) is too large. Must be between 0
and 999" if ($first > 99);

The ($first > 99) check here does not match the error text.  Why did none of
the tests fail assuming it should be ($first > 999)?  Do we need another test
case?

> WebKitLibraries/win/tools/scripts/version-stamp.pl:31
> +my $versionStamper = File::Spec->catfile($WEBKIT_LIBRARIES, 'tools',
'VersionStamper', 'VersionStamper.exe');

Nit: Windows probably doesn't care but technically this should be 'Tools'
instead of 'tools', right?

> WebKitLibraries/win/tools/scripts/version-stamp.pl:43
> +    my @arguments = split(/\s/, $ARGV[0]) or die "Couldn't parse $ARGV[0]";

You probably want to split on /\s+/ here so that passing two spaces between
arguments in a quoted string doesn't make $arguments[1] empty/undefined.

> WebKitLibraries/win/tools/scripts/version-stamp.pl:48
> +# Make sure we don't have any leading or trailing quote

Nit: "quotes" or "quote characters", plus a period to end the sentence.

> WebKitLibraries/win/tools/scripts/version-stamp.pl:66
> +open(VERSIONINFO, '<', $VERSION_FILE) or die "Unable to open $VERSION_FILE:
$!";

Nit: Please rename VERSIONINFO to VERSION_INFO.

> WebKitLibraries/win/tools/scripts/version-stamp.pl:76
> +	   $line =~ s/#define $componentKey//;
> +	   $line =~ s/^\s*(.*)\s*$/$1/;
> +	   $line =~ s/^"(.*)"$/$1/;
> +	   chomp($line);

Are we guaranteed that there are no "//" or "/* */" comments inline after the
#define?

> WebKitLibraries/win/tools/scripts/version-stamp.pl:85
> +my $TARGET_PATH = File::Spec->canonpath($target);

Nit: Seems kind of arbitrary that we use $TARGET_PATH (all caps, underscore)
here, but $versionStamper (camelCase, no underscore) above.

I'm not sure what the rule is for ALL_CAPS variables; maybe only environment
variables and file handles (or "constants")?

Not necessary to fix to land the patch.

> WebKitLibraries/win/tools/scripts/version-stamp.pl:87
> +system($versionStamper, '--verbose', $TARGET_PATH, '--fileMajor',
$components{'__VERSION_MAJOR__'},

We should check the return status here in case $versionStamper fails  (I think
there is a special thing you have to do to the return value of system() to
check whether it was successful or not; see other perl code in Tools/Scripts/.)