Bug 74836 – Replace webkitperl/run-leaks_unittest/RunLeaks.pm with webkitperl/LoadAsModule.pm

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Kentaro Hara]

webkitperl/run-leaks_unittest/RunLeaks.pm can be used for unit-testing of run-leaks only. Instead we can create more generalized webkitperl/LoadAsModule.pm, which can be used for unit-testing of other Perl scripts. We are planning to use it for unit-testing of prepare-ChangeLog.

[Kentaro Hara]

Created attachment 119810 [details]
Patch

[Kentaro Hara]

Comment on attachment 119810 [details]
Patch

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

> Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v1.0.pl:161
> +my $actualOutput = LoadAsModule::parseLeaksOutput(\@input);

By the way, I am not sure why we need to replace @input with \@input.


Reduced test case:

sub f(\@){ my $p = shift; print "@$p\n";}
my @v=(1, 2, 3);
f(@v);  # This works.

eval 'sub { sub ff(\@){ my $p = shift; print "@$p\n";}}';
my @vv=(1, 2, 3);
ff(@vv);  # This does not work.
ff(\@vv);  # This works instead. Why???

[Kentaro Hara]

Created attachment 119811 [details]
Patch

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> (From update of attachment 119810 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119810&action=review
> 
> > Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v1.0.pl:161
> > +my $actualOutput = LoadAsModule::parseLeaksOutput(\@input);
> 
> By the way, I am not sure why we need to replace @input with \@input.
> 
> Reduced test case:
> 
> sub f(\@){ my $p = shift; print "@$p\n";}
> my @v=(1, 2, 3);
> f(@v);  # This works.
> 
> eval 'sub { sub ff(\@){ my $p = shift; print "@$p\n";}}';
> my @vv=(1, 2, 3);
> ff(@vv);  # This does not work.
> ff(\@vv);  # This works instead. Why???

It's because Perl doesn't see a prototype for ff(), so it defaults to the no-prototype behavior.  You can fix it by doing this (although I'm not sure this is a good solution for generalizing the LoadAsModule.pm code (will look at the patch next):

sub ff(\@);
eval 'sub { sub ff(\@){ my $p = shift; print "@$p\n";}}';
my @vv=(1, 2, 3);
ff(@vv);  # This now works.
# ff(\@vv);  # This now fails with a compilation error (Type of arg 1 to main::ff must be array (not reference constructor))

See the "Prototypes" section of "man perlsub" for more details.

[Kentaro Hara]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 119810 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=119810&action=review
> > eval 'sub { sub ff(\@){ my $p = shift; print "@$p\n";}}';
> > my @vv=(1, 2, 3);
> > ff(@vv);  # This does not work.
> > ff(\@vv);  # This works instead. Why???
> 
> It's because Perl doesn't see a prototype for ff(), so it defaults to the no-prototype behavior.  You can fix it by doing this (although I'm not sure this is a good solution for generalizing the LoadAsModule.pm code (will look at the patch next):
> 
> sub ff(\@);
> eval 'sub { sub ff(\@){ my $p = shift; print "@$p\n";}}';
> my @vv=(1, 2, 3);
> ff(@vv);  # This now works.
> # ff(\@vv);  # This now fails with a compilation error (Type of arg 1 to main::ff must be array (not reference constructor))

ddkilzer: Thanks. However, actually, we cannot write the prototype like 'sub ff(\@)' in LoadAsModule.pm, because we cannot easily know what prototypes will appear in the loading Perl script. So the possible options would be

[a] Use \@input instead of @input (i.e. this patch).
[b] Leave RunLeaks.pm as it is, and we create PrepareChangeLog.pm. PrepareChangeLog.pm will be almost a copy of RunLeaks.pm.

> although I'm not sure this is a good solution for generalizing the LoadAsModule.pm code

Do you prefer [b]?

[David Kilzer (:ddkilzer)]

Comment on attachment 119811 [details]
Patch

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

> Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v1.0.pl:36
> -use RunLeaks;
>  use Test::More;
> +use lib "..";
> +use LoadAsModule;
> +
> +LoadAsModule::load("run-leaks");

I think this is definitely a step in the right direction (sharing code to load Perl scripts as modules), but this implementation has a couple of drawbacks:

1. You need a separate LoadAsModule::load() call for each script.
2. Scripts to test are in the LoadAsModule package, not their own package.
3. If two scripts had to be loaded, any methods with the same name would collide.

I think LoadAsModule should work like this where the first argument is the package name, and the second is the script name:

use lib "..";
use LoadAsModule qw(RunLeaks run-leaks);

There is a special "import" method that you may define that gets called when there are arguments to the 'use' statement (see lib.pm for an example).

Maybe we can put the whole sub-package in a big eval statement?

package LoadAsModule;

[...]

sub import
{
    my ($self, $package, $script) = @_;

    eval q{
        package } . $package . q{;

        use [...];

        sub readFile($);

        my $runLeaksPath = File::Spec->catfile(sourceDir(), "Tools", "Scripts", "} . $script . q{");
        eval "sub {" . readFile($runLeaksPath) . "}";

        sub readFile($) {
        [...]
    };
}

I'm not sure yet if this would work or not, though.

Maybe we can just do something more straight-forward without using a giant eval statement.  I'm going to play with this a bit.

> Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v2.0-new.pl:128
> -is_deeply($actualOutput, $expectedOutput, "leaks Report Version 2.0 (old)");
> +is_deeply($actualOutput, $expectedOutput, "leaks Report Version 2.0 (new)");

Nice catch!

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> use LoadAsModule qw(RunLeaks run-leaks);

I've got this working!

> Maybe we can just do something more straight-forward without using a giant eval statement.  I'm going to play with this a bit.

I'll post a patch or the new LoadAsModule.pm in a few minutes.

[David Kilzer (:ddkilzer)]

Created attachment 119929 [details]
How to use import and eval in LoadAsModule.pm

[David Kilzer (:ddkilzer)]

Comment on attachment 119811 [details]
Patch

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

>> Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v1.0.pl:36
>> +LoadAsModule::load("run-leaks");
> 
> I think this is definitely a step in the right direction (sharing code to load Perl scripts as modules), but this implementation has a couple of drawbacks:
> 
> 1. You need a separate LoadAsModule::load() call for each script.
> 2. Scripts to test are in the LoadAsModule package, not their own package.
> 3. If two scripts had to be loaded, any methods with the same name would collide.
> 
> I think LoadAsModule should work like this where the first argument is the package name, and the second is the script name:
> 
> use lib "..";
> use LoadAsModule qw(RunLeaks run-leaks);
> 
> There is a special "import" method that you may define that gets called when there are arguments to the 'use' statement (see lib.pm for an example).
> 
> Maybe we can put the whole sub-package in a big eval statement?
> 
> package LoadAsModule;
> 
> [...]
> 
> sub import
> {
>     my ($self, $package, $script) = @_;
> 
>     eval q{
>         package } . $package . q{;
> 
>         use [...];
> 
>         sub readFile($);
> 
>         my $runLeaksPath = File::Spec->catfile(sourceDir(), "Tools", "Scripts", "} . $script . q{");
>         eval "sub {" . readFile($runLeaksPath) . "}";
> 
>         sub readFile($) {
>         [...]
>     };
> }
> 
> I'm not sure yet if this would work or not, though.
> 
> Maybe we can just do something more straight-forward without using a giant eval statement.  I'm going to play with this a bit.

See the attached patch for how to do this.

[Kentaro Hara]

Created attachment 119935 [details]
rebased patch for review

[David Kilzer (:ddkilzer)]

Comment on attachment 119935 [details]
rebased patch for review

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

r=me

> Tools/Scripts/webkitperl/LoadAsModule.pm:35
> +use lib File::Spec->catdir($FindBin::Bin, "..", "..");

Are you sure you need two ".." arguments here?  It worked with just one ".." argument for me running:

./Tools/Scripts/test-webkitperl

[Kentaro Hara]

(In reply to comment #11)
> (From update of attachment 119935 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119935&action=review
> 
> r=me
> 
> > Tools/Scripts/webkitperl/LoadAsModule.pm:35
> > +use lib File::Spec->catdir($FindBin::Bin, "..", "..");
> 
> Are you sure you need two ".." arguments here?  It worked with just one ".." argument for me running:
> 
> ./Tools/Scripts/test-webkitperl

One ".." is enough to run ./Tools/Scripts/test-webkitperl, but two ".."s are needed to run ./Tools/Scripts/test-webkitperl and 'perl ./Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v1.0.pl' (although I am not sure we need to support the latter).

Are there any concern with two ".."s?

[David Kilzer (:ddkilzer)]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 119935 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=119935&action=review
> > 
> > r=me
> > 
> > > Tools/Scripts/webkitperl/LoadAsModule.pm:35
> > > +use lib File::Spec->catdir($FindBin::Bin, "..", "..");
> > 
> > Are you sure you need two ".." arguments here?  It worked with just one ".." argument for me running:
> > 
> > ./Tools/Scripts/test-webkitperl
> 
> One ".." is enough to run ./Tools/Scripts/test-webkitperl, but two ".."s are needed to run ./Tools/Scripts/test-webkitperl and 'perl ./Tools/Scripts/webkitperl/run-leaks_unittest/run-leaks-report-v1.0.pl' (although I am not sure we need to support the latter).
> 
> Are there any concern with two ".."s?

As long as ./Tools/Scripts/test-webkitperl works with two ".." arguments, that's fine.

[WebKit Review Bot]

Comment on attachment 119935 [details]
rebased patch for review

Clearing flags on attachment: 119935

Committed r103338: <http://trac.webkit.org/changeset/103338>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.