Bug 124549 – run-jsc-stress-tests should be able to package its tests and move them places

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

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.

[Mark Hahnenberg]

s/run-javascriptcore-tests/run-jsc-stress-tests/g

[Mark Hahnenberg]

This feature will be enabled by the --tarball flag and will be disabled by default.

[Mark Hahnenberg]

Created attachment 217363 [details]
Patch

[Geoffrey Garen]

Comment on attachment 217363 [details]
Patch

r=me

[Filip Pizlo]

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?

[Mark Hahnenberg]

(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.

[Mark Hahnenberg]

Committed r159538: <http://trac.webkit.org/changeset/159538>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 124627

[Mark Hahnenberg]

(In reply to comment #8)
> Re-opened since this is blocked by bug 124627

Today is not my day :-(

[Mark Hahnenberg]

Created attachment 217439 [details]
Patch

[Mark Hahnenberg]

(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.

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Mark Hahnenberg]

(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.

[Mark Hahnenberg]

Created attachment 217452 [details]
Patch

[Mark Hahnenberg]

Comment on attachment 217452 [details]
Patch

After talking in person, this approach definitely still needs some work.

[Mark Hahnenberg]

Created attachment 217698 [details]
Patch

[Filip Pizlo]

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.

[Mark Hahnenberg]

Created attachment 217713 [details]
Patch

[Filip Pizlo]

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.

[Mark Hahnenberg]

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.

[Mark Hahnenberg]

Created attachment 217715 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 217715 [details]
Patch

Clearing flags on attachment: 217715

Committed r159752: <http://trac.webkit.org/changeset/159752>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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>'

[Mark Hahnenberg]

(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.

[Mark Hahnenberg]

> Fix is on the way.

Fix landed in http://trac.webkit.org/changeset/159754.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 124847

[Mark Hahnenberg]

(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.

[Csaba Osztrogonác]

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 )

[Mark Hahnenberg]

Created attachment 218202 [details]
Patch

[Mark Hahnenberg]

(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.

[Mark Hahnenberg]

(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

[WebKit Commit Bot]

Comment on attachment 218202 [details]
Patch

Clearing flags on attachment: 218202

Committed r159955: <http://trac.webkit.org/changeset/159955>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.