Bug 29656 – Make WebKitTools/Scripits/run-sunspider script work on Windows platform

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

[Csaba Osztrogonác]

If you build QtWebKit on Windows platform, you have to use 
cmd.exe as shell. It doesn't interpret the first line of 
scripts, eg #!/usr/bin/perl, but you have to run perl scripts
directly with "perl ./foo.pl" command instead of "./foo.pl"

It is the correct thing if you invoke external script from
a perl script with exec or system command.

[Csaba Osztrogonác]

Created attachment 39954 [details]
proposed patch

[Eric Seidel (no email)]

Comment on attachment 39954 [details]
proposed patch

It seems wrong that only this script would need special casing.  Either we already handle MSWIN Perl in all the other perl scripts, or we dont' support it at all and we shouldn't here.

[Mark Rowe (bdash)]

run-webkit-tests invokes build-dumprendertree in the same manner that run-sunspider currently invokes build-jsc.  We used to have a Qt Windows buildbot that ran regression tests, which suggests that what run-webkit-tests is doing does work.  Of course, we don't have that build bot at the moment so I can't say that conclusively.

[Tor Arne Vestbø]

Comment on attachment 39954 [details]
proposed patch

run-webkit-tests uses open3 instead of system, can you try that and see if it helps?

[Csaba Osztrogonác]

(In reply to comment #4)
I tried it with open3, but it didn't help.

The problem is same, because system, exec and open3 don't interpret the first line of scripts, and don't know which script interpreter should be called.

[Tor Arne Vestbø]

It seems run-javascriptcore-tests uses perl directly, maybe that's why the old regression tests used to work bdash?

[Csaba Osztrogonác]

Created attachment 40304 [details]
proposed patch

I refactored my patch, and modified sunspider-compare-results script too.
I added a bug to depends list because of modifying isWindows() function.

[David Kilzer (:ddkilzer)]

Comment on attachment 40304 [details]
proposed patch

Since both of these scripts are Perl scripts, they had to be invoked with a Perl interpreter originally.  We should use the same Perl interpreter when running subsequent scripts.

I would add a method like this to webkitdirs.pm (see "man perlvar" for details):

use Config;

[...]

sub currentPerlPath()
{
    my $thisPerl = $^X;
    if ($^O ne 'VMS') {
        $thisPerl .= $Config{_exe} unless $thisPerl =~ m/$Config{_exe}$/i;
    }
    return $thisPerl;
}

Then the following changes can be made:

> diff --git a/WebKitTools/Scripts/run-sunspider b/WebKitTools/Scripts/run-sunspider
> +my @perl;
> +push @perl, "perl" if isWindows();

These are no longer needed.

> -        my $buildResult = system "WebKitTools/Scripts/build-jsc", @ARGV;
> +        my $buildResult = system @perl, "WebKitTools/Scripts/build-jsc", @ARGV;

Replace @perl with currentPerlPath() here.

> -exec "./sunspider", @args;
> +exec @perl, "./sunspider", @args;

Ditto.

> diff --git a/WebKitTools/Scripts/sunspider-compare-results b/WebKitTools/Scripts/sunspider-compare-results
> +my @perl;
> +push @perl, "perl" if isWindows();

These are no longer needed.

> -        my $buildResult = system "WebKitTools/Scripts/build-jsc", "--" . $configuration;
> +        my $buildResult = system @perl, "WebKitTools/Scripts/build-jsc", "--" . $configuration;

Replace @perl with currentPerlPath().

> -exec "./sunspider-compare-results", @args, @ARGV;
> +exec @perl, "./sunspider-compare-results", @args, @ARGV;

Ditto.

r- to make the above changes.

[Csaba Osztrogonác]

(In reply to comment #8)
You are right, that we should use same Perl interpreter when invoke another Perl scripts. It is nicer solution than using just "perl".

I don't know perlvar before, I read it, and find it is very helpful, thx. ;)

> sub currentPerlPath()
> {
>     my $thisPerl = $^X;
>     if ($^O ne 'VMS') {
>         $thisPerl .= $Config{_exe} unless $thisPerl =~ m/$Config{_exe}$/i;
>     }
>     return $thisPerl;
> }

I don't see why we should use this method to determine the actual Perl interpreter. Why don't use simple $^X? I think all windows platform don't need .exe suffixes. And my ActiveState Perl 5.10.0 said that its interpreter is c:\perl\bin\perl.exe. It works with ...perl and ...perl.exe too.

Do you know a perl used by WebKit developers which needs this very complex method? I think we can use simple "return $^X".

[Csaba Osztrogonác]

Created attachment 40365 [details]
updated patch

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> > sub currentPerlPath()
> > {
> >     my $thisPerl = $^X;
> >     if ($^O ne 'VMS') {
> >         $thisPerl .= $Config{_exe} unless $thisPerl =~ m/$Config{_exe}$/i;
> >     }
> >     return $thisPerl;
> > }
> 
> I don't see why we should use this method to determine the actual Perl
> interpreter. Why don't use simple $^X? I think all windows platform don't need
> .exe suffixes. And my ActiveState Perl 5.10.0 said that its interpreter is
> c:\perl\bin\perl.exe. It works with ...perl and ...perl.exe too.
> 
> Do you know a perl used by WebKit developers which needs this very complex
> method? I think we can use simple "return $^X".

The problem is that the manpage doesn't say which platforms need the workaround (and I have no more knowledge that the manpage on this one).  You say you don't need it with ActiveState Perl, but have you tested all combinations of ActiveState Perl, Cygwin's Perl, "native" (DOS/Windows-only) Perl and invoking the script from both a DOS shell and a Cygwin shell?

I think it's best to follow the manpage on this one.  It certainly won't hurt to use the code, and it's guaranteed to work.  (It would probably make sense to add a comment to the subroutine that mentions the perlvar manpage, though.)

[David Kilzer (:ddkilzer)]

Comment on attachment 40365 [details]
updated patch

> diff --git a/WebKitTools/Scripts/webkitdirs.pm b/WebKitTools/Scripts/webkitdirs.pm
>  # used for scripts which are stored in a non-standard location
> +sub currentPerlPath()
> +{
> +    return $^X;
> +}

I really think you should use the method from Comment #8 due to the reasons I outlined in Comment #11:  it's guaranteed to work, and we don't know exactly which platforms it's needed on.

r-

[David Kilzer (:ddkilzer)]

(In reply to comment #12)
> (From update of attachment 40365 [details])
> > diff --git a/WebKitTools/Scripts/webkitdirs.pm b/WebKitTools/Scripts/webkitdirs.pm
> >  # used for scripts which are stored in a non-standard location
> > +sub currentPerlPath()
> > +{
> > +    return $^X;
> > +}
> 
> I really think you should use the method from Comment #8 due to the reasons I
> outlined in Comment #11:  it's guaranteed to work, and we don't know exactly
> which platforms it's needed on.

If you change the method back, I will r+ the patch.  Everything else looks good!

[Csaba Osztrogonác]

Created attachment 40375 [details]
updated patch

OK, I changed back the method.

[David Kilzer (:ddkilzer)]

Comment on attachment 40375 [details]
updated patch

r=me.  I'll land this.

[David Kilzer (:ddkilzer)]

Committed r48931: <http://trac.webkit.org/changeset/48931>