Followup to #8438 - let NO_FILESYSTEM still work even if code uses files #8464
Conversation
…e filesystem. that flag means we manually want to not include filesystem support, even if the code seems to use it.
|
Why not make this a compiler time failure? Shouldn't we always prefer compiler time errors. Imagine you are working on a bug project that sets NO_FILESYSTEM. Someone adds some new code that using "fopen", but in some rare case. As a developer I'd like to know right away that my new code is not compatible with the project and needs modification. |
|
Yeah, that makes sense in it's own way - we'd need a different flag, though. The current flag just means to include the minimal FS support possible, assuming it won't actually be called. I broke that in the other PR mentioned, so this just restores the previous behavior and unbreaks people. In the longer term, with ASMFS we can maybe avoid this complexity entirely (there would be no JS dependencies for the filesystem). |
|
@sbc100 I'm having a "use-case" where it's convenient that NO_FILESYSTEM doesn't fail even when the code contains calls to fopen() etc, but never calls it: Dear ImGui contains code to serialize the current UI state (windows positions, sizes, etc) to disc, so on next start, the UI can be restored, but this code isn't useful on emscripten anyway, since it's all synchronous. This code can be disabled at runtime, but cannot be removed at compile time with a define. Compiling with NO_FILESYSTEM on emscripten still makes sense, since it reduces the download size. Similar situations may also exist in other libraries (image loaders etc...) Those often have code to either load image data from memory (useful for emscripten), or directly through the CRT file functions (not useful for emscripten, since this is synchronous code). But often the file IO code cannot be removed from compilation because the library authors don't specifically had the emscripten situation in mind. So if emscripten would make such situations a compile error, I would either have to remove NO_FILESYSTEM, increasing download size just for code that isn't called anyway... or pester a number of library authors with emscripten-specific PRs, which includes arguing about whether adding such fixes just for a single platform makes sense ;) |
|
Oh, actually I got this wrong - my original testcase here didn't pass even before that PR. The issue is much narrower - if the filesystem is disabled, then syscalls should not depend on it. That fixes a refined testcase that I pushed with that proper fix. @floooh , I think this should fix the regressions from that PR, does it fix your case? |
|
@kripken Yes, the PR fixes my samples. I'm having some weird closure errors, but maybe I did something wrong when test-merging the PR locally. But switching back and forth between incoming and this PR, and closure |
|
Building with |
|
This is the closure error output I get when linking with -g1 (including the linker command line), further down there are indeed FS-related undeclared variables: |
|
The |
|
The ERRNO_CODE error is gone, but the others are still there: |
|
PS: just to make sure I didn't mess up anything in my previous tests, I tested the PR on a fresh emscripten install, and the errors are the same. |
|
Interesting. I think this has uncovered a bunch of missing dependencies in our JS libraries. I'll keep working on it in this PR. |
|
Ok, tests are passing - @sbc100 , PTAL. Detailed description of where this ended up:
I also verified the sockets tests pass, which we don't run on CI here. |
|
@floooh, please check if this works on your codebases too. |
|
Everything working now, tested on my sokol-samples and the tiny-emulators, both with NO_FILESYSTEM=1 and closure enabled. Thanks! |
tests/test_core.py
Outdated
| @@ -4468,6 +4468,8 @@ def clean(out, err): | |||
|
|
|||
| def test_mount(self): | |||
| self.set_setting('FORCE_FILESYSTEM', 1) | |||
| self.set_setting('INCLUDE_FULL_LIBRARY', 1) # uses constants from ERRNO_CODES | |||
| self.set_setting('ERROR_ON_UNDEFINED_SYMBOLS', 0) # avoid errors when linking in full library | |||
There was a problem hiding this comment.
This seems like a regression, no?
There was a problem hiding this comment.
The issue here is that the test depends on ERRNO_CODES - which is an internal API. So in a sense, FS not automatically including it is a regression and users might notice it. But I think the risk is worth it for emitting smaller code.
There was a problem hiding this comment.
This seems a little gross.. but I'll take your word for it that there are no obvious better ways to achieve this or to write this test.
There was a problem hiding this comment.
Another option is to hardcode the constants in the test. I pushed a commit with that now.
tests/test_other.py
Outdated
| #include <sys/time.h> | ||
| #include <stddef.h> | ||
| int main() { | ||
| extern int __syscall295(int); |
There was a problem hiding this comment.
Function declaration normally go at global scope.
|
|
tests/test_core.py
Outdated
| @@ -4468,6 +4468,8 @@ def clean(out, err): | |||
|
|
|||
| def test_mount(self): | |||
| self.set_setting('FORCE_FILESYSTEM', 1) | |||
| self.set_setting('INCLUDE_FULL_LIBRARY', 1) # uses constants from ERRNO_CODES | |||
| self.set_setting('ERROR_ON_UNDEFINED_SYMBOLS', 0) # avoid errors when linking in full library | |||
There was a problem hiding this comment.
This seems a little gross.. but I'll take your word for it that there are no obvious better ways to achieve this or to write this test.
|
Also pushed a commit with a fix after merging latest incoming. |
|
Merging to resolve the existing breakage (this also affected binaryen CI), but if you don't like the change I made to that one test in the last commit, I can revert that in a followup. |
|
Thanks for merging, I have recompiled my sokol-samples and emulators with the current incoming and it's all looking good: https://floooh.github.io/sokol-html5/ The most simple clear-sample (https://floooh.github.io/sokol-html5/wasm/clear-sapp.html) is now 8.0 KB for the WASM and 8.4 KB for the JS (both compressed download size in Chrome). I haven't checked for a while, but I think that's a new record. Also the compressed download size for the C64 emulator (https://floooh.github.io/tiny8bit/c64.html) now fits comfortably into under 64KBs (with closure enabled and all the latest size improvements). And this even includes the embedded C64 Kernal/BASIC ROMs, and 8-bit CPU machine code compresses a whole lot worse than even WASM ;) |
…created (emscripten-core#8464) They just surfaced because of it - we were lucky before that they were not noticeable, since we included too much code, which hid things. * Split up PATH into PATH and PATH_FS, where the latter depends on FS while the former does not. This allows code to use basic path functionality without getting the whole FS. * Fix missing PATH dependencies through the codebase - turns out we missed them because FS depended on it, and we assumed that covered things (which it mostly does). * Refactor syscall internals to move sockets code into the sockets syscall, so non-socket-using code doesn't get sockets + FS + all the support for that. * Optimize syscall internals to not depend on ERRNO_CODES.
…_FILESYSTEM` as 0 too (emscripten-core#8524) Otherwise, we incorrectly try to use the filesystem in some cases, like the testcase added here with libc++ code. This also affected binaryen which is how I noticed. Followup to emscripten-core#8438 and emscripten-core#8464.
…that PR created (emscripten-core#8464)" This reverts commit 6f080ae.
…that PR created (emscripten-core#8464)" This reverts commit 6f080ae.
…created (emscripten-core#8464) They just surfaced because of it - we were lucky before that they were not noticeable, since we included too much code, which hid things. * Split up PATH into PATH and PATH_FS, where the latter depends on FS while the former does not. This allows code to use basic path functionality without getting the whole FS. * Fix missing PATH dependencies through the codebase - turns out we missed them because FS depended on it, and we assumed that covered things (which it mostly does). * Refactor syscall internals to move sockets code into the sockets syscall, so non-socket-using code doesn't get sockets + FS + all the support for that. * Optimize syscall internals to not depend on ERRNO_CODES.
…_FILESYSTEM` as 0 too (emscripten-core#8524) Otherwise, we incorrectly try to use the filesystem in some cases, like the testcase added here with libc++ code. This also affected binaryen which is how I noticed. Followup to emscripten-core#8438 and emscripten-core#8464.
That flag means we manually want to not include filesystem support, even if the code seems to use it. That PR changed things to a compile-time error instead, which this fixes.
We should eventually avoid including the FS object entirely, see comment in the code.