Summary: | [Win] Modify version numbering scheme to support 5-tuple versions | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||
Component: | Tools / Tests | Assignee: | Brent Fulgham <bfulgham> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bfulgham, buildbot, commit-queue, dbates, ddkilzer, mrowe, rniwa, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Brent Fulgham
2014-07-29 15:46:35 PDT
Created attachment 235719 [details]
Patch
Created attachment 235720 [details]
Patch
Created attachment 235722 [details]
Patch
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 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
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/.) (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. 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! Committed r171798: <http://trac.webkit.org/changeset/171798> |