Using wabt compiled to wasm

[andreaTP]

Here we move away from using wabt tools from the command line using a ProcessBuilder and we run the tools using Chicory itself.
Meanwhile, I updated wabt to 1.0.35.

A couple of observations:

• wasm-objdump fails in IntelliJ for me, looks like a problem with the classpath, I would be ok to remove its execution now that we pass the entire testsuite but open to suggestions
• performance are slightly worst as expected, it might be a motivation to start looking at them sooner, and they should not be completely terrible

Happy to receive early feedback!

[github-actions]

Test Results

   103 files  +1     103 suites  +1   18s ⏱️ -1s
27 811 tests +1  26 373 ✅ +1  1 438 💤 ±0  0 ❌ ±0 
28 271 runs  +1  26 750 ✅ +1  1 521 💤 ±0  0 ❌ ±0 

Results for commit e5ab525. ± Comparison against base commit 0ee6662.

This pull request removes 2 and adds 3 tests. Note that renamed tests count towards both.

com.dylibso.chicory.wat2wasm.Wat2WasmTest ‑ shouldRunWat2Wasm
com.dylibso.chicory.wat2wasm.Wat2WasmTest ‑ shouldThrowMalformedException

com.dylibso.chicory.wabt.Wast2JsonTest ‑ shouldRunWast2Json(Path)
com.dylibso.chicory.wabt.Wat2WasmTest ‑ shouldRunWat2Wasm
com.dylibso.chicory.wabt.Wat2WasmTest ‑ shouldThrowMalformedException

♻️ This comment has been updated with latest results.

[andreaTP]

The execution time seems to be about the same in CI! That's good news!

[andreaTP]

I just found out that even wat2wasm fails to be executed in IntelliJ.
Probably not a big issue but let me know if this hurts someone workflow.

I simplified a bunch of things and done some cleanup.

It would be great if @electrum can review the wabt module setup with shading and inclusions/exclusions 🙏

[andreaTP]

Additional note that this is much slower on my machine, likely the execution of the external process is better parallelized.
The difference is not observable in CI, probably, because there are only 2 cores.

[electrum]

I strongly advise against this shading approach. Using a shaded module as a dependency for another module within the same reactor build doesn't work as expected, because Maven doesn't understand shading and won't use the dependency-reduced POM for the dependent modules. Similarly, IntelliJ won't work with this, either. The exclusion hacks might make the Maven build work, but won't work with IntelliJ, and it feels fragile and hard to understand.

The only reliable approach that I've found is to do shading in a separate project. So you could have a chiocry-wabt project that depends on the latest release and produces a shaded artifact with a dependency-reduced POM. Then chicory would depend on this artifact, without the need for exclusions. We have several such sidecar shaded artifact projects for Trino. While it's annoying to manage these separate projects, they don't change frequently, and it brings the benefit of being completely reliable for the main project.

Alternatively, since this seems to only be used for tests, is to move the tests into a separate integration test module. This has the downside of the tests not being in the same module as the code, but is reliable and avoids all of the above problems. It would also allow running with the current version of Chicory, which is good for testing purposes, and will be important if a newer version of those tools requires a bug fix in Chicory (if using a release version, the tool couldn't be upgraded until a new version of Chicory is released).

[electrum]

I updated your PR in #341 that splits out the runtime tests into a separate module. The first commit is a squash of all commits from this PR, then more commits with my changes. Feel free to squash my commits as this is mostly your work.

[andreaTP]

Thanks a lot @electrum for your intervention and advice here!

I noticed that you went ahead and already proactively implemented the last option(effectively the only one that you could tackle on your own), this is excellent but slightly increases the complexity of this setup further.

The only reliable approach that I've found is to do shading in a separate project. So you could have a chiocry-wabt project

I think that this proposal is still the cleanest/most desirable, but we need @bhelx buy in to proceed or we have to host the additional repository on a personal account.

[electrum]

Having a separate repository that uses a shaded version the previous release seems more complicated than splitting out a runtime-tests module to run the WASM testsuite.

Shading and using the previous release version to avoid a circular dependency is an ugly workaround — it’s not a clean solution. It seems like a last resort when you have no better options.

If the concern here is being able to use text format — which isn’t an issue for users of the library as they won’t have a circular dependency — then we can just write the parser. I can look at that later today.

[andreaTP]

Ok, given your answer here and in the other PR, I think my mind is formed.
Let's move on with your fixes in #341 and iterate when necessary.

Thanks for bearing with me @electrum

[andreaTP]

If the concern here is being able to use #224

Thanks as always for offering your help @electrum .
I would say that it's really low priority at the moment, making the interpreter runtime safe by implementing the entire validation (I'm working on it in #340 e.g.) is on top of the priorities, if you ask me.