Bug 25101 - run-webkit-tests should launch DumpRenderTree and the image diff tool in a way compatible with threads.
Summary: run-webkit-tests should launch DumpRenderTree and the image diff tool in a wa...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on:
Blocks: 10906
  Show dependency treegraph
 
Reported: 2009-04-08 14:22 PDT by David Levin
Modified: 2009-04-09 09:15 PDT (History)
4 users (show)

See Also:


Attachments
Proposed fix. (7.21 KB, patch)
2009-04-08 14:45 PDT, David Levin
ddkilzer: review-
Details | Formatted Diff | Diff
Proposed fix. (6.77 KB, patch)
2009-04-08 22:20 PDT, David Levin
ddkilzer: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2009-04-08 14:22:53 PDT
run-webkit-tests sets up ENV correctly and launches these programs, but on a perl threads use the main thread's environment instead of the current thread.
Comment 1 David Levin 2009-04-08 14:45:06 PDT
Created attachment 29346 [details]
Proposed fix.
Comment 2 Eric Seidel (no email) 2009-04-08 14:59:09 PDT
Yeah, this is something ddkilzer or bdash or darin adler would want to review (as our local perl gurus).
Comment 3 David Kilzer (:ddkilzer) 2009-04-08 18:03:59 PDT
Comment on attachment 29346 [details]
Proposed fix.

Overall this patch looks great, but I have two concerns:

1. The serialization of %ENV using regular expressions is very clever, but I think it would be more straight-forward if Data::Dumper were used to do the serializing, and "eval" to do the deserializing.  The Data::Dumper module is present in all "modern" installations of Perl and it would simplify the code in run-webkit-tests and the utility script (ExecAppWithEnv.pm).  Try this:

$ perl -e 'use Data::Dumper; my $d = Data::Dumper->new([\%ENV], [qw(ENV)]); $d->Indent(0); $d->Purity(1); print $d->Dump();'

Also, instead of using "escape" and "unescape", I think it's clearer to use "serialize" and "deserialize" when describing the process of moving %ENV to the subprocess.

2. Please rename "ExecAppWithEnv.pm" to something that resembles a script name like "execAppWithEnv".  Naming it "ExecAppWithEnv.pm" made me think it was a Perl module, but it's not.  It's a Perl script that is invoked by run-webkit-tests.

r- to resolve the above issues.  Thanks!
Comment 4 David Levin 2009-04-08 22:20:27 PDT
Created attachment 29358 [details]
Proposed fix.

Addressed feedback.
Comment 5 David Kilzer (:ddkilzer) 2009-04-09 06:10:18 PDT
Comment on attachment 29358 [details]
Proposed fix.

r=me  Thanks!
Comment 6 David Kilzer (:ddkilzer) 2009-04-09 06:11:25 PDT
Comment on attachment 29358 [details]
Proposed fix.

>+        * Scripts/execAppWithEnv.pm: Added.

Oops!  This should just be "Scripts/execAppWithEnv".  :)
Comment 7 David Levin 2009-04-09 09:15:48 PDT
Committed <http://trac.webkit.org/changeset/42359>