Proper STANDALONE_WASM testing, using WASM_ENGINES by kripken · Pull Request #9499 · emscripten-core/emscripten

kripken · 2019-09-25T16:43:24Z

The initial landing of STANDALONE_WASM support had testing, but it was pretty hackish, just enough to get us going. This adds more proper support.

This defines WASM_ENGINES in the settings file, parallel to JS_ENGINES. Adds support for running in those runtimes - in particular, we run the wasm and not the JS.

Adds also_with_standalone_wasm to core, which allows us to add files to be tested in this mode. So far I added two tests.

Ignore temp file leaks on /tmp/wasmer/ as it appears to add a bunch of temp files there sometimes.

sbc100 · 2019-09-25T16:58:35Z

          command: |
            curl https://get.wasmer.io -sSfL | sh
+            mkdir ~/vms
+            cp ~/.wasmer/bin/wasmer ~/vms


How about just ln -s ~/.wasmer/bin/wasmer ~/vms/. Binaries don't always work standalone when you move them like this.

(I responded to this earlier, as it's under the general topic of why having a vms/ dir is useful.)

sbc100 · 2019-09-25T16:59:17Z

            wget https://github.com/CraneStation/wasmtime/releases/download/dev/wasmtime-dev-x86_64-linux.tar.xz
            tar -xf wasmtime-dev-x86_64-linux.tar.xz
-            cp wasmtime-dev-x86_64-linux/wasmtime ~/
+            cp wasmtime-dev-x86_64-linux/wasmtime ~/vms


ln -s wasmtime-dev-x86_64-linux/wasmtime ~/vms/.

sbc100 · 2019-09-25T17:00:11Z

            echo "V8_ENGINE = '`pwd`/upstream/bin/d8'" >> ~/.emscripten
            echo "JS_ENGINES = [NODE_JS, V8_ENGINE]" >> ~/.emscripten
+            echo "WASM_ENGINES = []" >> ~/.emscripten
+            test -f ~/vms/wasmtime && echo "WASMTIME = os.path.expanduser(os.path.join('~', 'vms', 'wasmtime')) ; WASM_ENGINES.append(WASMTIME)" >> ~/.emscripten || true


If ~/.wasmer/bin is the default location why not just hardcode that here?

Same with wasmtime-dev-x86_64-linux/wasmtime.. why copy them around?

I thought it might be nice to have a vms/ directory. Just one place we need to care about, and also we can avoid persisting to the CircleCI image anything but the necessary files.

sbc100 · 2019-09-25T17:06:32Z

+    command_flags += ['run']
+  if is_wasmer or is_wasmtime:
+    # in a wasm runtime, run the wasm, not the js
+    filename = filename[:-3] + '.wasm'


So we are still geneating the .js file in all cases? But then running the wasm on its own at this late stage?

I'd kind of rather push this logic back in the do_run method of the test runner, but I'm OK with cleaning this up later if you prefer?

We still build js+wasm normally. We then test the js, and separately also test the wasm without the js. Basically, when we ask to run the app in a js vm we run the js, and in wasm vm the wasm.

I actually think this makes sense to do here. This way we can call run from anywhere, and if the vm we run in happens to be a wasm vm, it will just work. That may be useful in other, browser, etc. eventually, and not just in do_run which is mainly for core.

sbc100 · 2019-09-25T17:09:03Z

 # JS_ENGINES = [NODE_JS] # add V8_ENGINE or SPIDERMONKEY_ENGINE if you have them installed too.
 #
+# WASMER = os.path.expanduser(os.path.join('~', '.wasmer', 'bin', 'wasmer'))
+# WASMTIME = os.path.expanduser(os.path.join('~', 'wasmtime'))


Do we need these two variables? Can we just add WASM_ENGINES and be done with it? It looks like we are detecting that type of the engine based on the filename anyway in jsrun.py

This basically just parallels the above code for SPIDERMONKEY_ENGINE and V8_ENGINE.

I think it makes sense there, and here, because we do need to know the identities of these engines. For example, v8 requires -- before the program args, and wasmer requires run before the wasm name.

sbc100 · 2019-09-25T17:09:21Z

    'JAVA',
    'JS_ENGINES',
+    'WASMER',
+    'WASMTIME',


I don't think these two are referenced anywhere.

I think they could be used in jsrun, so

is_wasmer = 'wasmer' in jsengine => is_wasmer = engine == shared.WASMER

The only reason I didn't do that is the JS engine detection doesn't work that way. But I could refactor them all that way, what do you think?

Also, some tests eventually may only run in wasmer etc., like we have tests that only run in NODE_JS etc., so having the variable is useful I think.

sbc100 · 2019-09-25T17:10:29Z


 # EM_CONFIG stuff
-if JS_ENGINES is None:
-  JS_ENGINES = [NODE_JS]


You don't like defaulting to running the tests in NODE_JS?

These two lines allow normal users to avoid having JS_ENGINES in their config files, and still be able to run tests.

Oh, I think this is just dead code. Earlier in the file we have

JS_ENGINES = [] [...] JS_ENGINES = [listify(engine) for engine in JS_ENGINES]

so it could never be None AFAICT.

sbc100 · 2019-09-25T17:11:51Z

Could I be a pain and ask you to wrap you commit messages at 80 chars? I know this might seem silly/trivial but it makes git log a lot more readable command line users.

kripken · 2019-09-25T17:41:06Z

About git commit messages, I usually copy-paste what is in the github description here... which is not wrapped. Sounds like I'd need to use a tool to wrap it? (is there an easy one?)

sunfishcode · 2019-09-25T17:46:52Z

Can you talk about the long-term vision here? Do you expect Emscripten's JS APIs will be desirable as an important foundation for the non-Web wasm ecosystem? Or do you expect Emscripten will grow to support more of WASI and ultimately be an alternative to wasi-sdk?

sbc100 · 2019-09-25T18:07:06Z

I think the goal is the latter. For low level APIs like filesystem it I think it makes sense for emscripten and WASI to agree wherever possible. The short term goal is for emscripten binaries that don't use any weby stuff to be WASI compliant. So yes, emscripten would be an alternative to wasi-sdk for such users. Using emscripten might be overkill for some such users, but for those that also want to run on the web it might be useful because of the JS runtime support that comes built-in.

sbc100 · 2019-09-25T18:10:33Z

About git commit messages, I usually copy-paste what is in the github description here... which is not wrapped. Sounds like I'd need to use a tool to wrap it? (is there an easy one?)

Right but the github description is populated by you git commits initially isn't it? If you prefer to type
your decirptions into the web-UI I would have hoped that github's editor would do this for you.. but maybe it doesn't. I'll see if I can find a way.

But configuring the editor you use for git commit to wrap at 80 will probably solve the problem in at least some cases I think?

kripken · 2019-09-25T18:17:49Z

@sbc100 I often rewrite the text in the github web UI here to improve it. Seems like I'd need to change what I do, but I'll try to find a convenient way.

kripken · 2019-09-25T19:47:32Z

@sunfishcode

do you expect Emscripten will grow to support more of WASI

Yes - we'd like to support standard APIs like wasi where possible, and where there is no downside. (An example of a downside is wasi may provide a "get the time" API, but on the Web it may be better to use an existing API; this is also why we avoid using musl's time code.)

and ultimately be an alternative to wasi-sdk?

I wouldn't put it like that - each will be better at certain things - but I guess the question is "alternative for what use case"?

Here's how I see it: There are 3 "big" use cases for wasm,

Web - e.g. Unity game, small kernel module used by JS, a library like Bullet, etc.
Server - console apps, etc.
Custom embedding - wasm running inside a larger application, where the wasm is a plugin or a dynamic library or such.

(there's also blockchain and others of course)

For the Web, Emscripten wants to emit Web API calls. We'll use wasi where possible though, just to have fewer "oddities", as mentioned earlier.

For the server, wasi APIs makes the most sense obviously. As Emscripten emits more wasi calls, perhaps it can be useful here too.

For the custom embedding case, wasi is a good basis, but each embedding will add its own extra stuff, either because wasi doesn't support them yet, or because it just needs custom things - say an image manipulation program that uses wasm plugins would need different things than an audio editor (each will probably have imports/exports to run the plugin on an image/audio, but the parameters etc. would vary widely). But basic stuff like printing may as well use wasi, and that's a big main reason I think emscripten should support wasi.

Overall, custom embeddings have the option to add support for nonstandard stuff that they need, like their own APIs, or emscripten APIs, or other stuff if that's useful. Emscripten's STANDALONE_WASM mode is designed to help there, as it'll emit as standalone a wasm as possible, but may still emit non-wasi calls if there is no wasi option, and the JS for the wasm will use those - a custom embedding would have the option to do so as well.

Do you expect Emscripten's JS APIs will be desirable as an important foundation for the non-Web wasm ecosystem?

Overall, probably not. But non-Web is pretty wide, so as mentioned earlier, custom embeddings may support some stuff. But I don't have a prediction about this being widespread or not.

Since we are guaranteed that `NODE_JS` is part of the config (since all users need this) we can set a sensible default for `JS_ENGINES`. This default was supposed to be part of #9469, but a bug meant the code was dead and it was removed in #9499. `JS_ENGINES` is needed for any users that want to run test code. Fix `gen_struct_info.py` to explicitly use NODE_JS since this is the only engine we expect users to have installed and the only one we officially support outside of test code.

Since we are guaranteed that `NODE_JS` is part of the config (since all users need this) we can set a sensible default for `JS_ENGINES`. This default was supposed to be part of #9469, but a bug meant the code was dead and it was removed in #9499. `JS_ENGINES` is needed for any users that want to run test code.

) The initial landing of STANDALONE_WASM support had testing, but it was pretty hackish, just enough to get us going. This adds more proper support. This defines WASM_ENGINES in the settings file, parallel to JS_ENGINES. Adds support for running in those runtimes - in particular, we run the wasm and not the JS. Adds also_with_standalone_wasm to core, which allows us to add files to be tested in this mode. So far I added two tests. Ignore temp file leaks on /tmp/wasmer/ as it appears to add a bunch of temp files there sometimes.

Since we are guaranteed that `NODE_JS` is part of the config (since all users need this) we can set a sensible default for `JS_ENGINES`. This default was supposed to be part of emscripten-core#9469, but a bug meant the code was dead and it was removed in emscripten-core#9499. `JS_ENGINES` is needed for any users that want to run test code.

kripken and others added 14 commits September 24, 2019 16:23

wip [ci skip]

684e12c

setup

a9a60fc

yolo

a3a0cc3

expected fail

6dcc681

flake8

8842adc

fix

952761a

debugging

3b7acb6

yolo

900a71a

hope

95dad8e

restore

7be3e86

fix

7aceb93

warn [ci skip]

6b4ab68

add a test

8603e4b

ignore wasmer temp files

79ae811

kripken requested a review from sbc100 September 25, 2019 16:43

sbc100 approved these changes Sep 25, 2019

View reviewed changes

review feedback [ci skip]

76fbc3d

Merge remote-tracking branch 'origin/incoming' into sttest

d01e51b

sbc100 approved these changes Sep 26, 2019

View reviewed changes

kripken merged commit e881201 into incoming Sep 26, 2019

kripken deleted the sttest branch September 26, 2019 22:43

sbc100 mentioned this pull request Sep 30, 2019

Add a default for JS_ENGINES #9542

Merged

Conversation

kripken commented Sep 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 commented Sep 25, 2019

Uh oh!

kripken commented Sep 25, 2019

Uh oh!

sunfishcode commented Sep 25, 2019

Uh oh!

sbc100 commented Sep 25, 2019

Uh oh!

sbc100 commented Sep 25, 2019

Uh oh!

kripken commented Sep 25, 2019

Uh oh!

kripken commented Sep 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kripken Sep 25, 2019 •

edited

Loading