932940 - Jit-test harness needs to set different remote --js-cache

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Dan Minor [:dminor]]

As per luke's comment on bug 931882, testBullet.js and testCaching.js fail on Android due to an inappropriate --js-cache setting when run remotely.

Comment 1

[Dan Minor [:dminor]]

Attachment: Set --js-cache to remote test root

Comment 2

[Luke Wagner [:luke]]

Instead of tweaking the --js-cache path later, can you just set it correctly up front in tests/lib/jittests.py (grep  JS_CACHE_DIR) or, if that's somehow not possible, in jit_tests.py when we initially add --js-cache to 'prefix' (again, grep JS_CACHE_DIR)?

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 824853 [details] [diff] [review]
Set --js-cache to remote test root

Review of attachment 824853 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not pleased with |prefix| as a mechanism for storing parts of the command that changes, although I guess we are already doing that. I'd like to move all the thing in prefix to static members of Test, but in the meantime, I think Luke is right: adjust the path when it gets inserted into prefix.

::: js/src/tests/lib/jittests.py
@@ +662,5 @@
>  
> +    # Set --js-cache to be the remote test root.
> +    try:
> +        js_cache_idx = prefix.index('--js-cache')
> +        prefix[js_cache_idx + 1] = options.remote_test_root

I think we do not want to store these cache files in the main directory either. I guess it shouldn't be too hard to dm.mkdir for this.

Comment 4

[Dan Minor [:dminor]]

Attachment: Reset --js-cache when running remotely

Thanks! With comments addressed.

I don't think we can do this from jittests.py as we don't know we are running remotely until we parse arguments in jit_test.py.

I'm also setting HAVE_MULTIPROCESSING to False here for similar reasons. If it is preferable, I can remove this and pass -j 1 in from mozharness.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 825275 [details] [diff] [review]
Reset --js-cache when running remotely

Review of attachment 825275 [details] [diff] [review]:
-----------------------------------------------------------------

Much better, however modifying JS_CACHE_DIR, much less from another module, is still just too ugly to live. Could you add a static property to |class Test| called |CacheDir| which is initially set to JS_CACHE_DIR. Then remove the |prefix += ['--jscache-dir', JS_CACHE_DIR]| and add this part of the command to line generation directly to Test.command, using Test.CacheDir. Then in jit_tests.py you can make Test.CacheDir whatever you need if os.remote.

With that we can continue keeping os.remote relatively compact: in this case, just unconditionally create and remove the local cachedir, even if it isn't used.

Comment 6

[Dan Minor [:dminor]]

Attachment: Add CacheDir to Test, set appropriately for remote case

Try run here: https://tbpl.mozilla.org/?tree=Try&rev=6cfe09571bbe

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 825891 [details] [diff] [review]
Add CacheDir to Test, set appropriately for remote case

Review of attachment 825891 [details] [diff] [review]:
-----------------------------------------------------------------

Beautiful! Thanks for taking the time to do this the right way. r=me

::: js/src/jit-test/jit_test.py
@@ +204,5 @@
>          prefix += ['--js-cache-per-process']
>  
>      # Clean up any remnants from previous crashes etc
> +    shutil.rmtree(jittests.Test.CacheDir, ignore_errors=True)
> +    os.mkdir(jittests.Test.CacheDir)

These should remain jittests.JS_CACHE_DIR, since CacheDir may have been switched to something that is only on the remote side, not on the local side.

Comment 8

[Dan Minor [:dminor]]

My pleasure, thanks for the review.

With comments addressed: https://hg.mozilla.org/integration/mozilla-inbound/rev/caef6eaabec2

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/caef6eaabec2