Use mainScriptUrlOrBlob if present under EXPORT_ES6 #23890
Conversation
|
Can we add a test for this. Otherwise lgtm. |
|
Can you mention that is a fix for a regression that was introduced #23171? (Assuming that is true?) |
|
Could you also perhaps mention (in a comment) why/when this might be needed /used? |
|
And it’s necessary to support running a threaded emscripten module from another origin, e.g. to distribute an emscripten module on a CDN (the script will load for the main thread but won’t be allowed to create workers). Using blobs instead allows a workaround. |
|
Can you put all those detail in the PR description? In terms of testing I think it should be fine to just extend the existing testing we have for |
|
I created a PR to increase the testing we have for mainScriptUrlOrBlob: #23891 |
|
Yes, cool! I am working on tests for pthread as blob and for webpack/vite, but I’m encountering something weird in my vite test. I want to instantiate the module twice (once with a string mainUrlOrBlob and once with a blob) within one test, but I’m getting two report_result outputs and the test harness doesn’t like it. Is there a way I can skip the generation of one or both of those |
|
As a quick update, I’m pretty sure I can revise this PR with tests tomorrow. There are some issues with the paths used by the web server giving 404s under some conditions but not others, but I’m confident I can resolve these. |
|
Instead of creating a new browser test can you just update the new |
test/test_browser.py
Outdated
| else: | ||
| copytree(test_file('webpack'), '.') | ||
| outfile = 'src/hello.js' | ||
| self.compile_btest('hello_world.c', ['-sEXIT_RUNTIME', '-sMODULARIZE', '-sENVIRONMENT=web,worker', '-o', outfile]) | ||
| self.run_process(shared.get_npm_cmd('webpack') + ['--mode=development', '--no-devtool']) | ||
| shutil.copy(outfile, 'dist/') |
There was a problem hiding this comment.
I think the idea is that the bundler should do this.. so you should not have to do this here. In fact, if you have to do this that would seem like a bug maybe? (Same with the .wasm file below I guess).
In any case, I think this bundler stuff is orthogonal the this change which I think we can test without modifying any bundler tests.
There was a problem hiding this comment.
Sorry, this one wasn't meant to be included here.
JoeOsborn
force-pushed
the
support-mainthreadurlorblob-es6modules
branch
from
f620b51 to
7616b71
Compare
|
Sorry my commits got messy there, I accidentally ran a commit -a without noticing. |
I ended up modifying a different existing browser test specifically about pthread blob support. |
BTW, |
|
Will do! It will only be for URLs though, not for blobs, since Node doesn’t seem to support blobs. |
Yes, the existing test in other just does |
JoeOsborn
force-pushed
the
support-mainthreadurlorblob-es6modules
branch
from
ec7aad1 to
47c54b5
Compare
Also fix an overly-complex Worker initialization.
Per @sbc100 review, reduce differences across conditions
Head branch was pushed to by a user without write access
JoeOsborn
force-pushed
the
support-mainthreadurlorblob-es6modules
branch
from
ce68912 to
472313d
Compare
|
Ah sorry, yeah that was something that bootstrap or some other script updated as far as I can tell. Do you need me to revert that and push another commit or will you handle it during merge? |
|
Yup, can you revert and push again. |
Spun out from #23804 .
This is a fix for #23769 which was introduced by #23171 , where the use of the bundler pattern hard-coding a script name was made to apply to all ES6 exports.
mainScriptUrlOrBlobis necessary under es6 exports to support running a threaded emscripten module from another origin, e.g. to distribute an emscripten module on a CDN (the script will load for the main thread but won’t be allowed to create workers). Using blobs is an effective workaround.