Closed Bug 932940 Opened 6 years ago Closed 6 years ago

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


(Core :: JavaScript Engine, defect)

Not set





(Reporter: dminor, Assigned: dminor)




(1 file, 2 obsolete files)

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.
Attachment #824853 - Flags: review?(terrence)
Summary: Jit-test harness needs to set differentremote --js-cache → Jit-test harness needs to set different remote --js-cache
Instead of tweaking the --js-cache path later, can you just set it correctly up front in tests/lib/ (grep  JS_CACHE_DIR) or, if that's somehow not possible, in when we initially add --js-cache to 'prefix' (again, grep JS_CACHE_DIR)?
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/
@@ +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.
Attachment #824853 - Flags: review?(terrence)
Thanks! With comments addressed.

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

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.
Attachment #824853 - Attachment is obsolete: true
Attachment #825275 - Flags: review?(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 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.
Attachment #825275 - Flags: review?(terrence)
Depends on: 933431
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/
@@ +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.
Attachment #825891 - Flags: review?(terrence) → review+
My pleasure, thanks for the review.

With comments addressed:
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.