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

Remove current_path define guard for WASM platform #230

Closed
wants to merge 1 commit into from

Conversation

DSchroer
Copy link

@DSchroer DSchroer commented Mar 6, 2022

Some WASM runtimes support the operations that are hidden behind these guards. Expose the functionality to the WASM platform.

@DSchroer DSchroer changed the title Remove define guards for WASM platform Remove current_path define guard for WASM platform Mar 6, 2022
@Lastique
Copy link
Member

Lastique commented Mar 6, 2022

The change was made as part of #144 by @whitequark. There, it was reported that WASI does not support an absolute current path.

Some WASM runtimes support...

If there are multiple different runtimes, some supporting the current path, others not, then this change is not correct. The library should work on all runtimes, so it must detect if the runtime supports the operation. If this is not possible then a user-defined config macro could be added. I don't know anything about WASI, so I can't pick a reasonable default for such a macro, but I lean towards "disabled by default" as that would make the library more portable by default.

@DSchroer
Copy link
Author

DSchroer commented Mar 7, 2022

The issue here is that the compiler toolchains like emscripten have already decided to support current path. So if you compile code using std::filesystem::current_path it will work as intended. However if I use boost::filesystem::current_path it will fail unexpectedly.

WASM is a very strange platform because really the platform is lightweight and software defined. If you prefer to hide it behind another flag I am happy to do so but locking it off because WASI does not support it while WASM can easily support it does not make a ton of sense to me.

@Lastique
Copy link
Member

Lastique commented Mar 7, 2022

My understanding is that WASI to WASM is similar to libc to C. I really have no idea how "standard" WASI is in the WASM ecosystem.

My rationale is that Boost.Filesystem should work in the "standard" WASM environment, for whatever definition of "standard" there is in the WASM ecosystem. If there are extensions to the standard provided by some environments, those extensions must be detected (at compile or run time) or hidden behind the user-definable switch. Making them mandatory, effectively breaking compatibility with some valid implementations, is not the way to go.

@DSchroer
Copy link
Author

DSchroer commented Mar 7, 2022

To me there is another layer between. WASI is the OS that libc runs on. There is already a WASI-libc library that exists to run in the WASI ecosystem. And that lib has support for current directory: WebAssembly/wasi-libc#214

@Lastique
Copy link
Member

Lastique commented Mar 7, 2022

Is there a way to detect if these APIs are supported by libc?

@DSchroer
Copy link
Author

DSchroer commented Mar 7, 2022

Is there a way to detect if these APIs are supported by libc?

Not that I know of but someone more well versed in this domain might know. From my understanding the only way to know if an API is available in WASM is at runtime. Its a user defined sandbox environment so the runtime env and the compiletime env are often very different.

If the build succeeds using emcc and there are no warnings regarding exported APIs being missing, its a pretty good guess that everything will be available.

@DSchroer
Copy link
Author

DSchroer commented Mar 7, 2022

Keep in mind the WASM is always statically linked. So the risk of backwards compatibility is much less on this platform.

@Lastique
Copy link
Member

Lastique commented Mar 7, 2022

So, even a configure-time check won't work?

@DSchroer
Copy link
Author

DSchroer commented Mar 7, 2022

Not really. You only know if the platform can support it when the final file is run. I can take a .wasm file and run it on two completely different WASI runtimes. For example NodeJS and Chrome. The wasm binary will have no way to detect that. If the symbol is missing the system will refuse to run the wasm binary but the user can then define the symbol and try again without recompiling anything.

@DSchroer
Copy link
Author

DSchroer commented Mar 7, 2022

This PR does not even define or import any new symbols. Its just making sure that _wasm does not go directly to:

emit_error(BOOST_ERROR_NOT_SUPPORTED

On WASI platforms that it does not work for it will continue to not work.
On WASI platforms that it does work for it will now start to work.

@Lastique
Copy link
Member

Lastique commented Mar 7, 2022

The difference is that with this patch the file will fail to load due to a missing symbol, even if it isn't used.

If there is no way to detect support for current directory API then I'm inclined to leave it unsupported by default. An opt-in macro to enable it on user's request seems to be the only way forward, but this PR is not that.

@DSchroer
Copy link
Author

DSchroer commented Mar 7, 2022

Ill close it for now however I suspect that this missing symbol still wont occur on any of the WASI runtimes now. Until then we will just leave the patch in our project. https://github.com/DSchroer/openscad-wasm

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