Bug 124581 – Modify webkitdirs to reuse checkForArgumentAndRemoveFromARGV

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Nick Diego Yamane (diegoyam)]

Modify webkitdirs do reuse checkForArgumentAndRemoveFromARGV

[Nick Diego Yamane (diegoyam)]

Created attachment 217307 [details]
Patch

[Nick Diego Yamane (diegoyam)]

Created attachment 217308 [details]
Patch

[Tim Horton]

Comment on attachment 217308 [details]
Patch

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

> Tools/ChangeLog:9
> +        functions instead of reusing them as it's being done by all other functions.

s/as it's/as is/, I think?

> Tools/ChangeLog:17
> +        (checkForArgumentAndRemoveFromArrayRef): Fixes a bug in elements removal.

what kind of bug! more explanation would be nice :)

> Tools/Scripts/webkitdirs.pm:846
> +sub checkForArgumentAndRemoveFromARGVGettingValue($$)

maybe getArgumentAndRemoveFromARGV? 'get' prefix is closer to 'checkFor'. I dunno.

> Tools/Scripts/webkitdirs.pm:850
> +    return 0 unless ($#matchingIndices != 1);

why 0 instead of undef or whatever?

[Nick Diego Yamane (diegoyam)]

(In reply to comment #3)
> (From update of attachment 217308 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217308&action=review

Thanks for the review.

> 
> > Tools/ChangeLog:9
> > +        functions instead of reusing them as it's being done by all other functions.
> 
> s/as it's/as is/, I think?
ok

> 
> > Tools/ChangeLog:17
> > +        (checkForArgumentAndRemoveFromArrayRef): Fixes a bug in elements removal.
> 
> what kind of bug! more explanation would be nice :)
> 
ok

> > Tools/Scripts/webkitdirs.pm:846
> > +sub checkForArgumentAndRemoveFromARGVGettingValue($$)
> 
> maybe getArgumentAndRemoveFromARGV? 'get' prefix is closer to 'checkFor'. I dunno.
> 
I've just followed the pattern the other functions were already using

> > Tools/Scripts/webkitdirs.pm:850
> > +    return 0 unless ($#matchingIndices != 1);
> 
> why 0 instead of undef or whatever?

In this case, the return value is checked in a boolean expression, so '0' or 'undef' has the same effect (in perl)

[Nick Diego Yamane (diegoyam)]

Created attachment 217313 [details]
Patch

[Daniel Bates]

Comment on attachment 217313 [details]
Patch

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

This patch looks good. I'm r-'ing this patch because of the lack of a test.

> Tools/Scripts/webkitdirs.pm:393
> +    if (checkForArgumentAndRemoveFromARGVGettingValue('--sdk', \$sdk)) {

Nit: ' => "

That is, we use double quotes for string literals unless we explicitly don't want string interpolation.

> Tools/Scripts/webkitdirs.pm:395
> +    } elsif (checkForArgumentAndRemoveFromARGV('--device')) {

Ditto.

> Tools/Scripts/webkitdirs.pm:397
> +    } elsif (checkForArgumentAndRemoveFromARGV('--sim') ||

Ditto.

> Tools/Scripts/webkitdirs.pm:398
> +        checkForArgumentAndRemoveFromARGV('--simulator')) {

Ditto.

> Tools/Scripts/webkitdirs.pm:614
> +    if (checkForArgumentAndRemoveFromARGV('--debug')) {

Ditto.

> Tools/Scripts/webkitdirs.pm:616
> +    } elsif(checkForArgumentAndRemoveFromARGV('--release')) {

Ditto.

> Tools/Scripts/webkitdirs.pm:618
> +    } elsif (checkForArgumentAndRemoveFromARGV('--profile') || checkForArgumentAndRemoveFromARGV('--profiling')) {

Ditto.

> Tools/Scripts/webkitdirs.pm:653
> +    if (checkForArgumentAndRemoveFromARGV('--32-bit')) {

Ditto.

> Tools/Scripts/webkitdirs.pm:881
> +    my $removeOffset = 0;
>      foreach my $index (@indicesToRemove) {
> -        splice(@$arrayRef, $index, 1);
> +        splice(@$arrayRef, $index - $removeOffset++, 1);

Can we write a test for this change?

You may find it beneficial to look at the tests in <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitperl/> when writing such a test, including <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitperl/VCSUtils_unittest/removeEOL.pl>.

[Nick Diego Yamane (diegoyam)]

(In reply to comment #6)
> (From update of attachment 217313 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217313&action=review
> 
> This patch looks good. I'm r-'ing this patch because of the lack of a test.
> 
> > Tools/Scripts/webkitdirs.pm:393
> > +    if (checkForArgumentAndRemoveFromARGVGettingValue('--sdk', \$sdk)) {
> 
> Nit: ' => "
> 
> That is, we use double quotes for string literals unless we explicitly don't want string interpolation.
> 
> > Tools/Scripts/webkitdirs.pm:395
> > +    } elsif (checkForArgumentAndRemoveFromARGV('--device')) {
> 
> Ditto.
> 
> > Tools/Scripts/webkitdirs.pm:397
> > +    } elsif (checkForArgumentAndRemoveFromARGV('--sim') ||
> 
> Ditto.
> 
> > Tools/Scripts/webkitdirs.pm:398
> > +        checkForArgumentAndRemoveFromARGV('--simulator')) {
> 
> Ditto.
> 
> > Tools/Scripts/webkitdirs.pm:614
> > +    if (checkForArgumentAndRemoveFromARGV('--debug')) {
> 
> Ditto.
> 
> > Tools/Scripts/webkitdirs.pm:616
> > +    } elsif(checkForArgumentAndRemoveFromARGV('--release')) {
> 
> Ditto.
> 
> > Tools/Scripts/webkitdirs.pm:618
> > +    } elsif (checkForArgumentAndRemoveFromARGV('--profile') || checkForArgumentAndRemoveFromARGV('--profiling')) {
> 
> Ditto.
> 
> > Tools/Scripts/webkitdirs.pm:653
> > +    if (checkForArgumentAndRemoveFromARGV('--32-bit')) {
> 
> Ditto.
> 

Ok for all above.

> > Tools/Scripts/webkitdirs.pm:881
> > +    my $removeOffset = 0;
> >      foreach my $index (@indicesToRemove) {
> > -        splice(@$arrayRef, $index, 1);
> > +        splice(@$arrayRef, $index - $removeOffset++, 1);
> 
> Can we write a test for this change?
> 
> You may find it beneficial to look at the tests in <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitperl/> when writing such a test, including <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitperl/VCSUtils_unittest/removeEOL.pl>.

Ok, I'll add some unit test cases for it :)

thanks

[Nick Diego Yamane (diegoyam)]

Created attachment 217337 [details]
Patch

[Daniel Bates]

Comment on attachment 217337 [details]
Patch

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

This patch is close. I'm r-'ing this patch since we should extract the bug fix into a separate bug and patch. See my reasoning below for more details.

> Tools/ChangeLog:20
> +        (checkForArgumentAndRemoveFromArrayRef): Bugfix: It was removing wrong
> +        elements when there were more then one occurrence of that argument.
> +        E.g: Checking for 'a' in {a, b, a, c}, the resulting array would be
> +        {b, a}, when it should be {b, c}.

Please extract this fix into a separate bug and patch. In general, a patch should represent a single bug fix. The advantage of this approach is that it makes it straightforward to track down a regression and/or revert a change set without inadvertently blaming or reverting code that wasn't the cause of a regression.

> Tools/Scripts/webkitperl/webkitdirs_unittest/checkForArgumentAndRemoveFromArrayRef.pl:1
> +#!/usr/bin/perl

Please add the -w argument to enable warning diagnostics: #!/usr/bin/perl -w

> Tools/Scripts/webkitperl/webkitdirs_unittest/checkForArgumentAndRemoveFromArrayRef.pl:26
> +use strict;

Please include the warnings module: use warnings;

> Tools/Scripts/webkitperl/webkitdirs_unittest/checkForArgumentAndRemoveFromArrayRef.pl:27
> +use Test::Simple tests => 4;

I suggest we use module Test::More instead of Test::Simple because it exposes additional assertion routines, such as is_deeply(). The assertion routine is_deeply() can compare complex structures, including arrays, and print where the structures differ (if they do). Then we don't need to implement a custom array equality function. See <http://search.cpan.org/~rjbs/Test-Simple-1.001002/lib/Test/More.pm> for more details.

I didn't mean to imply that you should use Test::Simple when I suggested looking at the test <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitperl/VCSUtils_unittest/removeEOL.pl> as an example.

> Tools/Scripts/webkitperl/webkitdirs_unittest/checkForArgumentAndRemoveFromArrayRef.pl:47
> +sub equalArrays(\@\@)
> +{
> +    my @array1 = @{ shift() };
> +    my @array2 = @{ shift() };
> +    return 0 if ($#array1 != $#array2);
> +
> +    for(my $i = 0; $i <= $#array1; ++$i) {
> +        if ($array1[$i] ne $array2[$i]) {
> +            return 0;
> +        }
> +    }
> +    return 1;
> +}

As aforementioned above, I suggest we use Test::More::is_deeply() instead of implemented our own array equality function as the former provides fails with verbose output when there is a difference between the specified arrays.

[Nick Diego Yamane (diegoyam)]

Created attachment 217476 [details]
Patch

[Nick Diego Yamane (diegoyam)]

(In reply to comment #8)
> Created an attachment (id=217337) [details]
> Patch

(In reply to comment #9)
> (From update of attachment 217337 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217337&action=review
> 
> This patch is close. I'm r-'ing this patch since we should extract the bug fix into a separate bug and patch. See my reasoning below for more details.
> 
> > Tools/ChangeLog:20
> > +        (checkForArgumentAndRemoveFromArrayRef): Bugfix: It was removing wrong
> > +        elements when there were more then one occurrence of that argument.
> > +        E.g: Checking for 'a' in {a, b, a, c}, the resulting array would be
> > +        {b, a}, when it should be {b, c}.
> 
> Please extract this fix into a separate bug and patch. In general, a patch should represent a single bug fix. The advantage of this approach is that it makes it straightforward to track down a regression and/or revert a change set without inadvertently blaming or reverting code that wasn't the cause of a regression.
> 

Just sent a new patch only without the bug fix and filed a new bug (https://bugs.webkit.org/show_bug.cgi?id=124676) to address the bug separately as you suggested.

> > Tools/Scripts/webkitperl/webkitdirs_unittest/checkForArgumentAndRemoveFromArrayRef.pl:1
> > +#!/usr/bin/perl
> 
> Please add the -w argument to enable warning diagnostics: #!/usr/bin/perl -w
> 
> > Tools/Scripts/webkitperl/webkitdirs_unittest/checkForArgumentAndRemoveFromArrayRef.pl:26
> > +use strict;
> 
> Please include the warnings module: use warnings;
> 
> > Tools/Scripts/webkitperl/webkitdirs_unittest/checkForArgumentAndRemoveFromArrayRef.pl:27
> > +use Test::Simple tests => 4;
> 
> I suggest we use module Test::More instead of Test::Simple because it exposes additional assertion routines, such as is_deeply(). The assertion routine is_deeply() can compare complex structures, including arrays, and print where the structures differ (if they do). Then we don't need to implement a custom array equality function. See <http://search.cpan.org/~rjbs/Test-Simple-1.001002/lib/Test/More.pm> for more details.
> 
> I didn't mean to imply that you should use Test::Simple when I suggested looking at the test <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitperl/VCSUtils_unittest/removeEOL.pl> as an example.
> 
> > Tools/Scripts/webkitperl/webkitdirs_unittest/checkForArgumentAndRemoveFromArrayRef.pl:47
> > +sub equalArrays(\@\@)
> > +{
> > +    my @array1 = @{ shift() };
> > +    my @array2 = @{ shift() };
> > +    return 0 if ($#array1 != $#array2);
> > +
> > +    for(my $i = 0; $i <= $#array1; ++$i) {
> > +        if ($array1[$i] ne $array2[$i]) {
> > +            return 0;
> > +        }
> > +    }
> > +    return 1;
> > +}
> 
> As aforementioned above, I suggest we use Test::More::is_deeply() instead of implemented our own array equality function as the former provides fails with verbose output when there is a difference between the specified arrays.

Sounds good, I'll do that.
thanks

[Daniel Bates]

Comment on attachment 217476 [details]
Patch

Thank you Nick for iterating on this patch.

[WebKit Commit Bot]

Comment on attachment 217476 [details]
Patch

Clearing flags on attachment: 217476

Committed r159599: <http://trac.webkit.org/changeset/159599>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

After this patch, I'm getting errors like the one below everywhere:
server:git-webkit rniwa$ Tools/Scripts/run-webkit-tests --debug LayoutTests/fast/dom LayoutTests/fast/html LayoutTests/fast/forms LayoutTests/svg LayoutTests/html5lib/
Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851.
Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852.
Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851.
Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852.
Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851.
Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852.
Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851.
Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852.
Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851.
Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852.
Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851.
Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852.

We're also seeing them on various builders on build.webkit.org.

Please fix it ASAP.

[Csaba Osztrogonác]

(In reply to comment #15)
> After this patch, I'm getting errors like the one below everywhere:
> server:git-webkit rniwa$ Tools/Scripts/run-webkit-tests --debug LayoutTests/fast/dom LayoutTests/fast/html LayoutTests/fast/forms LayoutTests/svg LayoutTests/html5lib/
> Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851.
> Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852.
> Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851.
> Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852.
> Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851.
> Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852.
> Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851.
> Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852.
> Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851.
> Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852.
> Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 851.
> Use of uninitialized value in splice at /Volumes/Data/git-webkit/Tools/Scripts/webkitdirs.pm line 852.
> 
> We're also seeing them on various builders on build.webkit.org.
> 
> Please fix it ASAP.

fixed in https://trac.webkit.org/changeset/159616