Bug 135400 – [Win] Modify version numbering scheme to support 5-tuple versions

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Brent Fulgham]

Way back in <http://trac.webkit.org/changeset/49026>, we switched versioning so that “537.22.3” became “5.37.22” in our versioning. This is causing us some grief with the new 5-tuple version numbers we would like to use.

Consequently, we will use the following scheme to cover the likely version numbers we might need, while still being readable and providing proper sort ordering of version numbers.

Windows provides a version structure consisting of four signed 4-byte values (Major, Minor, Tiny, Build). Of these, only the Major, Minor, and Tiny values are considered by the installer system when deciding whether to upgrade a DLL or executable. Consequently, we really only have three uint32 values to work with.

Because of the way we do versioning, it is unlikely that our Minor, Tiny, or Micro values will go above 50 or so. Consequently, we can partition the valid 65,535 numbers in our three tuples as follows:

First uint32:
    Major: 0-65,535 
Second uint32:
    Minor: 0-64
    Tiny: 0-999
Third uint32:
    Micro: 0-64
    Nano: 0-999

Therefore what appears in strings as “600.1.2.3.67” would show up numerically on Windows as version “600.1002.3067”. Likewise, “60023.64.687.55.982” would appear as “60023.64687.55982”; and “214747.999.999.999.999” would blow with an error.

[Radar WebKit Bug Importer]

<rdar://problem/17849033>

[Brent Fulgham]

Created attachment 235719 [details]
Patch

[Brent Fulgham]

Created attachment 235720 [details]
Patch

[Brent Fulgham]

Created attachment 235722 [details]
Patch

[Build Bot]

Comment on attachment 235722 [details]
Patch

Attachment 235722 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6197564969844736

New failing tests:
media/media-fragments/TC0001.html

[Build Bot]

Created attachment 235726 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[David Kilzer (:ddkilzer)]

Comment on attachment 235722 [details]
Patch

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

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> (From update of attachment 235722 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235722&action=review
> 
> > 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/.)

And that special thing is calling exitStatus() from VCSUtils.pm and passing in the return value from the system() call.

[Brent Fulgham]

Comment on attachment 235722 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235722&action=review

>> Tools/Scripts/test-webkitperl:55
>> +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.

Yikes! I missed that :-(

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

Not only that, but I used $first when I should have used $second. Sloppy :-(. The reason none of the 999 tests failed was that it was comparing $first in both tests. I don't have any tests that willfully violate the maximum sizes because the test harness isn't sophisticated enough to recover if the program fails due to bad input, so the test cases are all 'expected good' inputs. I.e., we don't test to confirm that known failures continue to be known failures -- it's only looking for regressions.

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

Actually, this is in svn as lower-case (i.e., this is WebKitLibraries/win/tools, not <root>/Tools).

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

OK!

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

OK!

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

OK!

>> WebKitLibraries/win/tools/scripts/version-stamp.pl:76
>> +        chomp($line);
> 
> Are we guaranteed that there are no "//" or "/* */" comments inline after the #define?

Yes, because we auto-generate the input file, and it does not have any comments (inline or otherwise).

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

I thought the typical use was for environment variables, file handles, and 'constants'. I'll change $versionStamper -> $VERSION_STAMPER to be more consistent.

>>> 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/.)
> 
> And that special thing is calling exitStatus() from VCSUtils.pm and passing in the return value from the system() call.

Done!

[Brent Fulgham]

Committed r171798: <http://trac.webkit.org/changeset/171798>