Bug 133102 – Add option to run-jsc-stress-tests to use installed jsc

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

[Michael Saboff]

Typically run-jsc-stress-tests is used to test jsc and a JavaScriptCore built the user.  We should have an option to use a system installed jsc.

[Michael Saboff]

Created attachment 231745 [details]
Patch

[Geoffrey Garen]

Comment on attachment 231745 [details]
Patch

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

> Tools/Scripts/run-jsc-stress-tests:111
> +    puts "--jsc                (-j)   Path to JavaScriptCore."
> +    puts "--system-jsc                Use jsc installed on the system."

Why is this better than "--jsc /usr/local/bin/jsc"? It seems like you've conflated two things behind this option: (1) a built-in --jsc; (2) a --no-copy option for --remote.

[Michael Saboff]

(In reply to comment #2)
> (From update of attachment 231745 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231745&action=review
> 
> > Tools/Scripts/run-jsc-stress-tests:111
> > +    puts "--jsc                (-j)   Path to JavaScriptCore."
> > +    puts "--system-jsc                Use jsc installed on the system."
> 
> Why is this better than "--jsc /usr/local/bin/jsc"? It seems like you've conflated two things behind this option: (1) a built-in --jsc; (2) a --no-copy option for --remote.

There are three reasons for the two different options, 1) I don't want figure out that the path provided is for the system jsc in order to not package up and send it (and it's framework).  2) the path for jsc might be different or not existent on the local device compared to the remote device when --remote is used.  3) I could see where we might want to specify both "use the system jsc (--system-jsc)"  and "here is it's path (--jsc)", since the script may not know where the system jsc happens to be.

I think it is reasonable to change --system-jsc to --no-copy-jsc and require that that --jsc always needs to be given.  What I posted eliminates the need for two options and assumes /usr/local/bin/jsc.  That suites the needs we currently have.

[Geoffrey Garen]

Comment on attachment 231745 [details]
Patch

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

> Tools/Scripts/run-jsc-stress-tests:128
> +               ['--system-jsc', GetoptLong::NO_ARGUMENT],

Let's call this --no-copy, and have it modify the meaning of --jsc to be the final location where the framework already exists.

> Tools/Scripts/run-jsc-stress-tests:145
> +    when '--system-jsc'
> +        $jscPath = "/usr/local/bin/jsc"

Let's use the jsc inside the framework, even in the system case. That removes a special case.

> Tools/Scripts/run-jsc-stress-tests:433
> +        if !$systemJsc
> +            script += "export DYLD_FRAMEWORK_PATH=$(cd ../#{$frameworkPath.dirname}; pwd)\n"

I think we can remove this special case by using the jsc inside the framework.

[Michael Saboff]

Comment on attachment 231745 [details]
Patch

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

>> Tools/Scripts/run-jsc-stress-tests:433
>> +            script += "export DYLD_FRAMEWORK_PATH=$(cd ../#{$frameworkPath.dirname}; pwd)\n"
> 
> I think we can remove this special case by using the jsc inside the framework.

We'll still need some special case, since the existing code is assuming a relative path and with the "--no-copy" option we'll now have an absolute path.  That special case could be moved to the generation of frameworkPath and then eliminate the "../" everywhere it is used.

[Michael Saboff]

Created attachment 231905 [details]
Updated patch with agreed upon changes

[Geoffrey Garen]

Comment on attachment 231905 [details]
Updated patch with agreed upon changes

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

r=me

> Tools/Scripts/run-jsc-stress-tests:110
>      puts "--jsc                (-j)   Path to JavaScriptCore. This option is required."

Let's be clearer here: "Path to JavaScriptCore build product."

> Tools/Scripts/run-jsc-stress-tests:111
> +    puts "--no-copy                   Use the existing JavaScriptCore specified by the --jsc option."

Let's be clearer here: "Do not copy the JavaScriptCore build product before testing. --jsc specifies an already present JavaScriptCore to test."

> Tools/Scripts/run-jsc-stress-tests:125
> +jscPathArg = ""

I'd call this "jscArg", since the argument is named "jsc" and not "jscPath".

> Tools/Scripts/run-jsc-stress-tests:178
> +        $jscPath = Pathname.new(jscPathArg).realpath
> +    else
> +        $jscPath = Pathname.new(jscPathArg)

Why do these disagree about realpath?

> Tools/Scripts/run-jsc-stress-tests:1040
> +        if !$remote
> +            $testingFrameworkPath = frameworkFromJSCPath($jscPath).realpath
> +            $jscPath = Pathname.new($jscPath).realpath
> +        else
> +            $testingFrameworkPath = frameworkFromJSCPath($jscPath)

Why do these disagree about realpath?

[Michael Saboff]

(In reply to comment #7)
> (From update of attachment 231905 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231905&action=review
> 
> r=me
> 
> > Tools/Scripts/run-jsc-stress-tests:110
> >      puts "--jsc                (-j)   Path to JavaScriptCore. This option is required."
> 
> Let's be clearer here: "Path to JavaScriptCore build product."

Done.

> > Tools/Scripts/run-jsc-stress-tests:111
> > +    puts "--no-copy                   Use the existing JavaScriptCore specified by the --jsc option."
> 
> Let's be clearer here: "Do not copy the JavaScriptCore build product before testing. --jsc specifies an already present JavaScriptCore to test."

Done.
 
> > Tools/Scripts/run-jsc-stress-tests:125
> > +jscPathArg = ""
> 
> I'd call this "jscArg", since the argument is named "jsc" and not "jscPath".

Done.
 
> > Tools/Scripts/run-jsc-stress-tests:178
> > +        $jscPath = Pathname.new(jscPathArg).realpath
> > +    else
> > +        $jscPath = Pathname.new(jscPathArg)
> 
> Why do these disagree about realpath?

realpath will canonicalize the path by turning relative paths into absolute paths and resolving symbolic links.  For example, providing "WebKitBuild/Release/JavaScriptCore/jsc"  might become "/home/me/src/webkit/WebKitBuild/Release/JavaScriptCore.framework/Versions/A/Resources/jsc"

This kind of full path breaks frameworkFromJSCPath() in the script and also generally doesn't work when the .realpath processing is done on the local machine and the --remote option was provided.

> > Tools/Scripts/run-jsc-stress-tests:1040
> > +        if !$remote
> > +            $testingFrameworkPath = frameworkFromJSCPath($jscPath).realpath
> > +            $jscPath = Pathname.new($jscPath).realpath
> > +        else
> > +            $testingFrameworkPath = frameworkFromJSCPath($jscPath)
> 
> Why do these disagree about realpath?

See above.

[Michael Saboff]

Committed r169217: <http://trac.webkit.org/changeset/169217>