Stream: wasmtime

Topic: testing

Andrew Brown (Jun 02 2021 at 17:39):

@Alex Crichton, @Chris Fallin: I just merged https://github.com/bytecodealliance/wasmtime/pull/2957 since it seemed to make sense but I asked myself, "are these tests not being run on ARM in CI? Shouldn't we be seeing failures there?" IIRC, the S390 testing was pending some changes to QEMU, but aren't the tests being run in qemu-aarch64?

Chris Fallin (Jun 02 2021 at 17:40):

Yes, we do run aarch64 tests with qemu... I haven't looked closely at that PR yet but I should

Alex Crichton (Jun 02 2021 at 17:41):

yeah we're running the test, I don't think the PR was correct, it should probably only disable the tests when the x86_64 backend isn't built. On CI we build tests with all backends enabled

Chris Fallin (Jun 02 2021 at 17:42):

Ah, OK, so it's just building and then checking CLIF, it's not executing anything... the x86-64 is just a placeholder for "we need some backend"

Alex Crichton (Jun 02 2021 at 17:42):

the PR disables the tests unless it's on the x86_64 platform, but I think the right fix is to require that the backend is built in

Chris Fallin (Jun 02 2021 at 17:43):

I don't remember the crate dependency graph offhand but if we could use cranelift-native here we could just get a backend for the current machine

Andrew Brown (Jun 02 2021 at 19:38):

I think the right fix is to require that the backend is built in

Yeah, but isn't that what @Benjamin Bouvier is getting at in https://github.com/bytecodealliance/wasmtime/pull/2957#discussion_r643136838? In the absence of a way of knowing "is the x86 backend present?" we have to check the target architecture.

Andrew Brown (Jun 02 2021 at 19:49):

we could just get a backend for the current machine

I'm looking at those tests now and I think this makes sense to try... they assert a specific generated CLIF for memory operations so hopefully the emitted CLIF would be the same for different backends? If we think that is true and I can submit a PR to bring in cranelift-native as a dev dependency and use the current backend for the test.

Andrew Brown (Jun 02 2021 at 19:51):

Or perhaps @Alex Crichton there is some way to figure out incranelift-frontend which features were enabled for its cranelift-codegen dependency?

Alex Crichton (Jun 02 2021 at 19:55):

I think cranelift-codegen would have to expose a function of some kind. Or like you could catch the error when loading the target and "pass" the test because support wasn't compiled in

Andrew Brown (Jun 02 2021 at 19:58):

Yeah, that's not a bad idea... Just print out Skipping test foo because the x86 backend is not present or something like that?

Andrew Brown (Jun 02 2021 at 19:59):

It would be nicer to be able to ignore tests at runtime but I'm not sure that is possible...

Andrew Brown (Jun 02 2021 at 20:27):

Well, let's see how https://github.com/bytecodealliance/wasmtime/pull/2961 goes: basically I just made the tests use the host system's target ISA (pulled in by cranelift-codegen) which should have the effect that these tests would run for everyone regardless of host. I'm not sure if the emitted CLIF will always be the same but I don't really see why not so let's run it and see...

Chris Fallin (Jun 02 2021 at 20:31):

Thanks for tackling this! Yeah I don't think the CLIF should differ -- given that cranelift-frontend did not previously depend on cranelift-native to determine the platform

Andrew Brown (Jun 02 2021 at 20:34):

@Alex Crichton, when you get a chance: not sure why the verify-publish step is failing: https://github.com/bytecodealliance/wasmtime/pull/2961/checks?check_run_id=2731435666#step:5:868. Is there something different that has to happen for dev dependencies?

Alex Crichton (Jun 02 2021 at 21:38):

@Andrew Brown dependencies are topologically sorted in scripts/publish.rs and you'll probably need to reorder something

Andrew Brown (Jun 03 2021 at 17:03):

Ok, I used @bjorn3's suggestion to bring in the x86 backend as a dev dependency in https://github.com/bytecodealliance/wasmtime/pull/2961. I think that is a better solution than what we previously merged. Thoughts?

bjorn3 (Jun 03 2021 at 17:06):

Did you see my last suggestion of creating a FrontendConfig directly? https://github.com/bytecodealliance/wasmtime/pull/2961#issuecomment-853748761

Andrew Brown (Jun 03 2021 at 17:12):

Yeah, I just tried the first suggestion first... You think that's a way better approach?

Andrew Brown (Jun 03 2021 at 17:14):

Oh, I see... you mean in addition to what is currently there?

Andrew Brown (Jun 03 2021 at 17:15):

No, ok, I see what you mean... completely replace the use of TargetIsa

Last updated: Nov 22 2024 at 17:03 UTC