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

Add support for WASI #825

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for WASI #825

wants to merge 1 commit into from

Conversation

fredldotme
Copy link

Disables shared libraries for WASI only and makes sure compat doesn't redefine builtins.

Disables shared libraries for WASI only and makes sure
compat doesn't redefine builtins.
@fredldotme
Copy link
Author

Sidenote: I haven't checked whether json_c_snprintf is actually used anywhere, actually. But I made it a possibly unused function due to the assumption that, just because it's not used in one source file might not mean much.

@hawicz
Copy link
Member

hawicz commented Sep 3, 2023

I haven't read more than the first few lines of the diff, but already I can tell you I'm not going to merge this. Please don't litter the code with ifdef __wasi__ lines. Instead, figure out why the regular symbol/feature detection isn't working, and fix that.

@fredldotme
Copy link
Author

Is at least the CMake part kosher enough or are there complaints about that approach too?

@hawicz
Copy link
Member

hawicz commented Sep 3, 2023

Sure, I guess that's ok, though it's not clear to me why you're disabling shared libs and tests, so a comment explaining that would be nice.

Also, is "WASI" in this case "Web Assembly System Interface"? Please at least mention that full name in the commit message, and ideally add a "Building on WASI" section to README.md (assuming there are some special steps needed beyond the Building on Unix section)

Finally, the duplication in snprintf_compat.h is bugging me. Can you try to consolidate with the MSC/MINGW case, perhaps with a #define _vsnprintf vsnprintf for the __wasi__ case?
(does WASI really have the same lack-of-termination bug? argh!)

@fredldotme
Copy link
Author

Sure can add some more commentary.

Yes WASI is "WebAssembly System Interface", and it has no concept of shared libraries at the moment, so everything must be statically linked together.

I'll take a look at the compat parts in a bit.

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.

None yet

2 participants