Wasmtime GitHub notifications bot (Jun 17 2021 at 11:47): Wasmtime GitHub notifications bot (Jun 17 2021 at 11:47):akirilov-arm opened PR #2994 from aarch64_tests to main:
These changes are partially in support of #2960. With them the tests run on AArch64 should be the same as those executing on x86-64, except for a couple that are specific to the latter platform, 3 that fail under QEMU, and some CLIF tests missing the
target aarch64directive.
Wasmtime GitHub notifications bot (Jun 17 2021 at 14:27):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jun 17 2021 at 14:27):alexcrichton created PR review comment:
Oh we've actually already got another env var for indicating that qemu is in use and virtual memory should be limited as a result, perhaps that would suffice for this test too?
Wasmtime GitHub notifications bot (Jun 17 2021 at 14:28):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jun 17 2021 at 14:28):alexcrichton created PR review comment:
I think this can probably remove the
#[cfg]now? (it's probably best to keep a "know failing list" rather than a known passing list)
Wasmtime GitHub notifications bot (Jun 17 2021 at 14:38):akirilov-arm submitted PR review.
Wasmtime GitHub notifications bot (Jun 17 2021 at 14:38):akirilov-arm created PR review comment:
Since the CI tests are only on AArch64 and x86-64, that's what I did initially, but then I decided to make it easier for other platforms (and with QEMU it should be straightforward in principle to add s390x tests, for example).
I am happy to go for either approach (and what you say about a known failing list makes sense), but I think I will need to keep some of these tests disabled because right now they seem to fail due to memory exhaustion (everything passes when I run it natively on my AArch64 development machine).
Wasmtime GitHub notifications bot (Jun 17 2021 at 14:41):akirilov-arm edited PR review comment.
Wasmtime GitHub notifications bot (Jun 17 2021 at 14:43):akirilov-arm submitted PR review.
Wasmtime GitHub notifications bot (Jun 17 2021 at 14:43):akirilov-arm created PR review comment:
Thanks, checking it instead should be more robust in case we decide to test on other platforms with QEMU.
Wasmtime GitHub notifications bot (Jun 17 2021 at 14:53):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jun 17 2021 at 14:53):alexcrichton created PR review comment:
Personally I prefer to get failures rather than silent passing on new architectures, and most of the test suite will already fail if you try to test wasmtime on a mips machine for example. Now that being said if these get switched to an ignore for s390x that seems reasonable!
(it's relatively easy to grep for
TODO.*s390xor something similar whereas it's a bit more unclear to search for something that only runs for other platforms)
Wasmtime GitHub notifications bot (Jun 17 2021 at 15:03):akirilov-arm submitted PR review.
Wasmtime GitHub notifications bot (Jun 17 2021 at 15:03):akirilov-arm created PR review comment:
grep -R cfg.*ignorewas my approach :).
Wasmtime GitHub notifications bot (Jun 17 2021 at 15:25):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jun 17 2021 at 15:25):alexcrichton created PR review comment:
Heh true! Well either way doesn't matter too much to me, so I'm happy to do whatever you feel is best
Wasmtime GitHub notifications bot (Jun 17 2021 at 15:52):uweigand submitted PR review.
Wasmtime GitHub notifications bot (Jun 17 2021 at 15:52):uweigand created PR review comment:
According to the comment, the reason these were disabled are reference types, but we do in fact fully support reference types on s390x, so those should probably just work ...
Wasmtime GitHub notifications bot (Jun 17 2021 at 15:59):uweigand submitted PR review.
Wasmtime GitHub notifications bot (Jun 17 2021 at 15:59):uweigand created PR review comment:
... and I just verified the tests do indeed pass on s390x when enabled.
Wasmtime GitHub notifications bot (Jun 17 2021 at 16:07):akirilov-arm submitted PR review.
Wasmtime GitHub notifications bot (Jun 17 2021 at 16:07):akirilov-arm created PR review comment:
@uweigand What about the general question - should the tests be ignored or left to fail? E.g. if reference types weren't supported by the s390x backend, from your perspective would it have been preferable to keep a directive here to ignore the tests or not?
Wasmtime GitHub notifications bot (Jun 17 2021 at 16:18):uweigand submitted PR review.
Wasmtime GitHub notifications bot (Jun 17 2021 at 16:18):uweigand created PR review comment:
I think ignore by default is never a good idea. They should fail by default, and then can be set to ignore on a one-by-one basis per test/platform after analysis. (Then you can at least grep for your platform name to find a list of tests you still need to work on.)
Wasmtime GitHub notifications bot (Jun 18 2021 at 13:49):akirilov-arm updated PR #2994 from aarch64_tests to main.
Wasmtime GitHub notifications bot (Jun 18 2021 at 20:08):uweigand submitted PR review.
Wasmtime GitHub notifications bot (Jun 18 2021 at 20:08):uweigand created PR review comment:
I just ran into this problem on s390x under QEMU as well. Shouldn't we just unconditionally ignore the pooling tests on all platforms if platform_is_emulated() is true?
Wasmtime GitHub notifications bot (Jun 21 2021 at 13:13):akirilov-arm updated PR #2994 from aarch64_tests to main.
Wasmtime GitHub notifications bot (Jun 21 2021 at 13:55):akirilov-arm requested alexcrichton for a review on PR #2994.
Wasmtime GitHub notifications bot (Jun 21 2021 at 14:05):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jun 21 2021 at 14:05):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jun 21 2021 at 14:05):alexcrichton created PR review comment:
Could this use
(and)to group the latter condition here? I never remember the precedences of||and&&...
Wasmtime GitHub notifications bot (Jun 21 2021 at 14:05):alexcrichton created PR review comment:
Could this check get moved to a helper function in
tests/all/gc.rsto avoid duplicating the env var name too much?
Wasmtime GitHub notifications bot (Jun 21 2021 at 16:33):akirilov-arm updated PR #2994 from aarch64_tests to main.
Wasmtime GitHub notifications bot (Jun 21 2021 at 17:09):akirilov-arm requested alexcrichton for a review on PR #2994.
Wasmtime GitHub notifications bot (Jun 21 2021 at 17:26):alexcrichton merged PR #2994.
Last updated: Nov 22 2024 at 17:03 UTC