Closed Bug 932940 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: dminor, Assigned: dminor)

References

Details

Attachments

(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/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 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.
Attachment #824853 - Flags: review?(terrence)
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.
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 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.
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/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.
Attachment #825891 - Flags: review?(terrence) → review+
My pleasure, thanks for the review.

With comments addressed: https://hg.mozilla.org/integration/mozilla-inbound/rev/caef6eaabec2
https://hg.mozilla.org/mozilla-central/rev/caef6eaabec2
Status: NEW → RESOLVED
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.