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.
Created attachment 39954 [details] proposed patch
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.
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.
Comment on attachment 39954 [details] proposed patch run-webkit-tests uses open3 instead of system, can you try that and see if it helps?
(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.
It seems run-javascriptcore-tests uses perl directly, maybe that's why the old regression tests used to work bdash?
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.
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.
(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".
Created attachment 40365 [details] updated patch
(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.)
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-
(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!
Created attachment 40375 [details] updated patch OK, I changed back the method.
Comment on attachment 40375 [details] updated patch r=me. I'll land this.
Committed r48931: <http://trac.webkit.org/changeset/48931>