-
Notifications
You must be signed in to change notification settings - Fork 3.4k
When building with -sENVIRONMENT=web, do not produce WASI'isms in the MINIMAL_RUNTIME build. #22502
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -106,7 +106,13 @@ var imports = { | |
'a': wasmImports, | ||
#else // MINIFY_WASM_IMPORTED_MODULES | ||
'env': wasmImports, | ||
|
||
// 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Also, won't those that care about code size already fall into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This only happens if METADCE is being run, which it isn't if ASYNCIFY is enabled. |
||
#endif | ||
|
||
#endif // MINIFY_WASM_IMPORTED_MODULES | ||
}; | ||
|
||
|
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 implementexit
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.