Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SINGLE_FILE html and worker fixes + tests #5736

Merged
merged 7 commits into from
Nov 7, 2017

Conversation

buu700
Copy link
Contributor

@buu700 buu700 commented Nov 2, 2017

While further investigating this as requested, I found some SINGLE_FILE bugs that impacted html output and workers.

Specifically:

  1. SINGLE_FILE with html output would fail to compile, due to embedding the wasm in the js and deleting it, then afterwards trying to embed the deleted wasm in the html.

  2. In the case of both html and workers, multiple files would be produced/required (respectively, test.html + test.js and test.js + test.worker.js).

Most of the lines changed in emcc.py are just indentation, so this will be easier to view with ?w=1.

@kripken
Copy link
Member

kripken commented Nov 3, 2017

  1. You can pass , also_proxied=True to btest to get a test to run in both main thread and proxied worker mode, to save some writing in the second test.
  2. It should be ok to avoid the binaryen interpreter, and assume the browser has native support.
  3. Looks like this makes us emit all the JS in the html when in SINGLE_FILE mode and emitting html? That sounds good, but it is a change from before, is that correct?

Otherwise looks good, thanks!

@buu700
Copy link
Contributor Author

buu700 commented Nov 3, 2017

Thanks!

re: 1 and 2, got it, will go ahead and make those changes.

re: 3, yep, now using SINGLE_FILE with html output will produce just a single html file with all JS included as inline scripts. It is a change from the previous behavior, but I don't remember if we'd decided one way or the other how this option should work with html or documented anything about that combination anywhere, so the fact that it left the JS separate from the html was probably just an oversight.

@kripken
Copy link
Member

kripken commented Nov 3, 2017

Sounds good about 3. Please also add a test for it (just a few extra lines in an existing test, that no .js file is emitted).

@buu700
Copy link
Contributor Author

buu700 commented Nov 3, 2017

That is covered in the existing tests (see the assertion at the end of each one).

@buu700
Copy link
Contributor Author

buu700 commented Nov 3, 2017

Well, also_proxied didn't seem to like SINGLE_FILE (died with an error about trying to copy a non-existent test.js), so for now I just left the tests as-is aside from switching to native wasm as you suggested.

@kripken
Copy link
Member

kripken commented Nov 6, 2017

Yeah, some old debug code there assumed both html and js files existed. I fixed that on incoming now, so you should be able to merge that and then use that option here.

@buu700
Copy link
Contributor Author

buu700 commented Nov 7, 2017

Cool, thanks; just pushed the updated test.

@kripken
Copy link
Member

kripken commented Nov 7, 2017

Great, thanks!

@kripken kripken merged commit 7802705 into emscripten-core:incoming Nov 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants