Bug 124549

Summary: run-jsc-stress-tests should be able to package its tests and move them places
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, fpizlo, gyuyoung.kim, ossy, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 124627, 124847    
Bug Blocks: 120809, 124551    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Mark Hahnenberg 2013-11-18 17:11:13 PST
Currently run-javascriptcore-tests runs files wherever it finds them. We want it to be able to run tests on remote machines, which will require moving those test files around. A first step to this is for run-jsc-tests to package everything it needs into a tar file.
Comment 1 Mark Hahnenberg 2013-11-18 17:11:59 PST
s/run-javascriptcore-tests/run-jsc-stress-tests/g
Comment 2 Mark Hahnenberg 2013-11-18 17:17:12 PST
This feature will be enabled by the --tarball flag and will be disabled by default.
Comment 3 Mark Hahnenberg 2013-11-19 17:10:13 PST
Created attachment 217363 [details]
Patch
Comment 4 Geoffrey Garen 2013-11-19 17:34:19 PST
Comment on attachment 217363 [details]
Patch

r=me
Comment 5 Filip Pizlo 2013-11-19 18:05:56 PST
Comment on attachment 217363 [details]
Patch

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

R=me too

> Tools/Scripts/run-jsc-stress-tests:318
> +        self.addLayoutTestFiles

These "self." things should be superfluous. Get rid of them if possible.

> Tools/Scripts/run-jsc-stress-tests:330
> +        system("mkdir", "-p", bundleTestPath.to_s) unless bundleTestPath.directory?

Why not use the ruby mkdir API?
Comment 6 Mark Hahnenberg 2013-11-19 18:16:14 PST
(In reply to comment #5)
> (From update of attachment 217363 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217363&action=review
> 
> R=me too
> 
> > Tools/Scripts/run-jsc-stress-tests:318
> > +        self.addLayoutTestFiles
> 
> These "self." things should be superfluous. Get rid of them if possible.

Will do.

> 
> > Tools/Scripts/run-jsc-stress-tests:330
> > +        system("mkdir", "-p", bundleTestPath.to_s) unless bundleTestPath.directory?
> 
> Why not use the ruby mkdir API?

You mean the FileUtils mkdir_p thingy? No reason really.
Comment 7 Mark Hahnenberg 2013-11-19 18:49:06 PST
Committed r159538: <http://trac.webkit.org/changeset/159538>
Comment 8 WebKit Commit Bot 2013-11-19 19:14:27 PST
Re-opened since this is blocked by bug 124627
Comment 9 Mark Hahnenberg 2013-11-19 19:16:19 PST
(In reply to comment #8)
> Re-opened since this is blocked by bug 124627

Today is not my day :-(
Comment 10 Mark Hahnenberg 2013-11-20 09:04:21 PST
Created attachment 217439 [details]
Patch
Comment 11 Mark Hahnenberg 2013-11-20 09:06:33 PST
(In reply to comment #10)
> Created an attachment (id=217439) [details]
> Patch

The main difference is that the default Bundle class now uses WEBKIT_PATH as the base for all paths passed to localPathToBundlePath. This prevents run-jsc-stress-tests from getting confused when using LAYOUTTESTS_RELATIVE_PATH to setup the helper files for layout tests.
Comment 12 Filip Pizlo 2013-11-20 09:29:21 PST
Comment on attachment 217439 [details]
Patch

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

How does this handle extra files for Kraken, Octane, etc?

> Tools/Scripts/run-jsc-stress-tests:289
> +    attr_accessor :baseForExtraFiles
> +    def add(localTestPath, testName)

I would put a blank line between attr_accessor and def add.

> Tools/Scripts/run-jsc-stress-tests:317
> +        self.addLayoutTestFiles

Remove self.

> Tools/Scripts/run-jsc-stress-tests:337
> +        self.add(LAYOUTTESTS_RELATIVE_PATH + "resources", "standalone-pre.js")

Remove self.

> Tools/Scripts/run-jsc-stress-tests:338
> +        self.add(LAYOUTTESTS_RELATIVE_PATH + "resources", "standalone-post.js")

Remove self.

> Tools/Scripts/run-jsc-stress-tests:344
> +            self.add(@baseForExtraFiles, file)

Remove self.
Comment 13 Filip Pizlo 2013-11-20 09:33:41 PST
Am I understanding right that this is engineered around having all tests in a subdirectory of the WebKit checkout?

That won't work for any of our internal tests.
Comment 14 Mark Hahnenberg 2013-11-20 09:55:09 PST
(In reply to comment #13)
> Am I understanding right that this is engineered around having all tests in a subdirectory of the WebKit checkout?
> 
> That won't work for any of our internal tests.

I don't think so. It's engineered around having all tests relative to the WebKit checkout, which I believe was already the case.
Comment 15 Mark Hahnenberg 2013-11-20 10:08:12 PST
Created attachment 217452 [details]
Patch
Comment 16 Mark Hahnenberg 2013-11-20 11:55:07 PST
Comment on attachment 217452 [details]
Patch

After talking in person, this approach definitely still needs some work.
Comment 17 Mark Hahnenberg 2013-11-22 10:38:08 PST
Created attachment 217698 [details]
Patch
Comment 18 Filip Pizlo 2013-11-22 12:56:54 PST
Comment on attachment 217698 [details]
Patch

We talked about it offline.  Looks really good but it would be even better if run-jsc-stress-tests didn't hard-code yaml filenames.
Comment 19 Mark Hahnenberg 2013-11-22 13:44:44 PST
Created attachment 217713 [details]
Patch
Comment 20 Filip Pizlo 2013-11-22 13:50:08 PST
Comment on attachment 217713 [details]
Patch

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

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

Change this to not be an IMPORTANT_ENV and handle it outside the loop.

> Tools/Scripts/run-jsc-stress-tests:611
> +    $extraFilesBaseDir = $extraFilesBaseDir.dirname unless $extraFilesBaseDir.basename == $benchmarkDirectory.basename

Don't change this here.
Comment 21 Mark Hahnenberg 2013-11-22 13:57:46 PST
I cc-ed some people responsible for other platforms since they'll need to make some adjustments if they want it to work for them.
Comment 22 Mark Hahnenberg 2013-11-22 14:08:14 PST
Created attachment 217715 [details]
Patch
Comment 23 WebKit Commit Bot 2013-11-25 09:26:51 PST
Comment on attachment 217715 [details]
Patch

Clearing flags on attachment: 217715

Committed r159752: <http://trac.webkit.org/changeset/159752>
Comment 24 WebKit Commit Bot 2013-11-25 09:26:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 Csaba Osztrogon√°c 2013-11-25 09:53:06 PST
(In reply to comment #23)
> (From update of attachment 217715 [details])
> Clearing flags on attachment: 217715
> 
> Committed r159752: <http://trac.webkit.org/changeset/159752>

FYI, it broke run-javascriptcore-tests script on all build.webkit.org bots:

Error: #<LoadError: cannot load such file -- highline>
Tools/Scripts/run-jsc-stress-tests:153:in `frameworkFromJSCPath': Unknown JSC path, cannot copy full framework: /Volumes/Data/slave/mavericks-release-tests-wk1/build/WebKitBuild/Release/jsc (RuntimeError)
	from Tools/Scripts/run-jsc-stress-tests:163:in `copyVMToBundle'
	from Tools/Scripts/run-jsc-stress-tests:822:in `prepareBundle'
	from Tools/Scripts/run-jsc-stress-tests:1056:in `<main>'
Comment 26 Mark Hahnenberg 2013-11-25 09:53:29 PST
(In reply to comment #25)
> (In reply to comment #23)
> > (From update of attachment 217715 [details] [details])
> > Clearing flags on attachment: 217715
> > 
> > Committed r159752: <http://trac.webkit.org/changeset/159752>
> 
> FYI, it broke run-javascriptcore-tests script on all build.webkit.org bots:
> 
> Error: #<LoadError: cannot load such file -- highline>
> Tools/Scripts/run-jsc-stress-tests:153:in `frameworkFromJSCPath': Unknown JSC path, cannot copy full framework: /Volumes/Data/slave/mavericks-release-tests-wk1/build/WebKitBuild/Release/jsc (RuntimeError)
>     from Tools/Scripts/run-jsc-stress-tests:163:in `copyVMToBundle'
>     from Tools/Scripts/run-jsc-stress-tests:822:in `prepareBundle'
>     from Tools/Scripts/run-jsc-stress-tests:1056:in `<main>'

Fix is on the way.
Comment 27 Mark Hahnenberg 2013-11-25 10:02:00 PST
> Fix is on the way.

Fix landed in http://trac.webkit.org/changeset/159754.
Comment 28 WebKit Commit Bot 2013-11-25 10:36:13 PST
Re-opened since this is blocked by bug 124847
Comment 29 Mark Hahnenberg 2013-11-25 10:38:03 PST
(In reply to comment #28)
> Re-opened since this is blocked by bug 124847

This broke the linux test bots. Ossy has agreed to help adjust the script so that it works for the linux bots as well.
Comment 30 Csaba Osztrogon√°c 2013-11-26 11:21:40 PST
I tried to debug r159752-r159754 changes on Linux, but it seems the problem
is bigger than I thought before. The whole "framework" is a Mac only thing,
it doesn't exist on Linux at all, that's why frameworkFromJSCPath() is broken,
and then setting $frameworkPath and $jscPath based on the this premise that
framework exists and jsc is placed in $frameworkPath/Resources/jsc.

archiveBuiltProduct() in Tools/BuildSlaveSupport/built-product-archive
would be a good start to bundle everything properly on all ports. Of
course it packs everything for layout testing which we don't need for
running only jsc tests.

But it seems it works only on GTK now (but packs jsc-stress-results unnecessarily!), EFL doesn't use it, because they have builder-tester bots only,
not separated builder and tester. But I still wondering how it works, because it
doesn't pack the monumental but necessary WebKitBuild/Dependencies.

It's more complex than I thought. :( I propose keeping the actual behaviour
of jsc-stress-tests script and create bundle if only --tarball is passed
until we find the proper way creating bundle on Linux. ( as Mark mentioned
in https://bugs.webkit.org/show_bug.cgi?id=124549#c2 )
Comment 31 Mark Hahnenberg 2013-12-02 11:43:32 PST
Created attachment 218202 [details]
Patch
Comment 32 Mark Hahnenberg 2013-12-02 11:44:32 PST
(In reply to comment #31)
> Created an attachment (id=218202) [details]
> Patch

I modified the script to artificially create the directory structure it expects, even on platforms that don't normally have a JavaScriptCore.framework directory.
Comment 33 Mark Hahnenberg 2013-12-02 11:52:46 PST
(In reply to comment #32)
> (In reply to comment #31)
> > Created an attachment (id=218202) [details] [details]
> > Patch
> 
> I modified the script to artificially create the directory structure it expects, even on platforms that don't normally have a JavaScriptCore.framework directory.

For those reviewing, the changes are in copyVMToBundle and prepareFramework
Comment 34 WebKit Commit Bot 2013-12-02 12:52:41 PST
Comment on attachment 218202 [details]
Patch

Clearing flags on attachment: 218202

Committed r159955: <http://trac.webkit.org/changeset/159955>
Comment 35 WebKit Commit Bot 2013-12-02 12:52:47 PST
All reviewed patches have been landed.  Closing bug.