-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
When building with -sENVIRONMENT=web, do not produce WASI'isms in the MINIMAL_RUNTIME build. #22502
base: main
Are you sure you want to change the base?
When building with -sENVIRONMENT=web, do not produce WASI'isms in the MINIMAL_RUNTIME build. #22502
Conversation
|
||
// There does not currently exist a ENVIRONMENT_MAY_BE_WASI, so liken it to the shell environments. | ||
// (e.g. anyone building with -sENVIRONMENT=web won't be running in WASI) | ||
#if ENVIRONMENT_MAY_BE_NODE || ENVIRONMENT_MAY_BE_SHELL | ||
'{{{ WASI_MODULE_NAME }}}': wasmImports, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there are parts of the C library that depend on WASI-named syscalls.
We could perhaps see if any of the imports come from the WASI namespace and make this line conditional (for both minimal and regular runtime).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, won't those that care about code size already fall into MINIFY_WASM_IMPORTED_MODULES
above, avoiding this code block completely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, won't those that care about code size already fall into
MINIFY_WASM_IMPORTED_MODULES
above, avoiding this code block completely?
This only happens if METADCE is being run, which it isn't if ASYNCIFY is enabled.
|
||
// There does not currently exist a ENVIRONMENT_MAY_BE_WASI, so liken it to the shell environments. | ||
// (e.g. anyone building with -sENVIRONMENT=web won't be running in WASI) | ||
#if ENVIRONMENT_MAY_BE_NODE || ENVIRONMENT_MAY_BE_SHELL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems to imply a misunderstnading about how we use the WASI APIs in emscripten. The WASI APIs are like the regular __syscall_
functions that we implement in JS. They are just JS library functions like any other and can be used in any envrionment.
For example the __proc_exit
function which is used to implement exit
is a WASI API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I understand that in many cases the functions overlap. Although from the code size creep perspective that is not working out today, i.e. using that __proc_exit
as an example, I am getting that in a build even when I build with -sEXIT_RUNTIME=0
, so functionally that would warrant for a #if ENVIRONMENT_IS_WASI
construct if there is no other way to avoid leaking in the unused stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to emit this line if and only the wasm module requires WASI imports. I think we can just look that the metadata from the wasm module to determine this and set and internal settings such as REQUIRES_WASI_IMPORTS based on that.
No description provided.