Initial NODERAWFS implementation #6008

saschanaz · 2017-12-31T07:36:14Z

This removes the need for emscripten-compiled CLI tools to manually mount NODEFS and map all file paths.

-s NODERAWFS=1 overwrites on FS object and works as an in-place replacement when on Node.js, otherwise the existing virtual filesystem is provided.

Node.js supports integer flags but it depends on native system constants, which are different from values on struct_info.compiled.json

kripken · 2018-01-03T17:54:53Z

Nice! This is simpler than I thought it would need to be.

More testing (of all the file operations that are practical to test) and documentation would be good here.

kripken · 2018-01-03T17:56:15Z

Also we should test it in core, so all compilation modes get checked.

juj · 2018-01-04T11:51:41Z

Should this disable --embed-file and --preload-file to be used in conjunction, since this actually backs on a physical filesystem that is already persistently prepopulated, and not on to a virtual filesystem that would need to be loaded up at every startup. Therefore virtual filesystem packages would end up unpacking the contents of the .data files again and again to a physical location on disk. Perhaps a mutual incompatibility check would be good here?

saschanaz · 2018-01-05T12:22:19Z

It seems the truncate test does not work with root permission.

juj · 2018-01-10T14:04:03Z

tests/unistd/dup.c

-  f = open("/", O_RDONLY);
-  f2 = open("/", O_RDONLY);
+  f = open(".", O_RDONLY);
+  f2 = open(".", O_RDONLY);


Could we leave a form that checks against / in non-NODERAWFS form? I think we'd want to retain that, it's an important invariant of MEMFS that / root is predictable.

juj · 2018-01-10T14:07:27Z

tests/fs/test_nodefs_cloexec.c

    );
-    assert(open("/working/test.txt", O_RDONLY | O_CLOEXEC) != -1);
+    assert(open("test.txt", O_RDONLY | O_CLOEXEC) != -1);


The removal of /working/ could be seen as changing the test, although searching through the tests/ subdirectory, it does look like there's still a couple of places where we explicitly test the /working/test.txt form. I think we'd like to ensure that this is tested, since this does change absolute to relative, which sometimes has different behavior. Perhaps a

#ifdef NODERAWFS #define ROOT "" #else #define ROOT "/" #endif ... assert(open(ROOT "working/test.txt", O_RDONLY | O_CLOEXEC) != -1);

or similar, to keeping testing behavior of absolute paths, while retaining the relative test for NODERAWFS?

(I'm not sure how important specifically the absolute root path property was when the original test was being crafted, but I think it's good to keep it as close to original as possible, perhaps only adding new cases)

kripken · 2018-01-12T18:17:14Z

tests/test_core.py

@@ -4582,20 +4599,43 @@ def process(filename):
      }
    ''', 'at Object.write', js_engines=[NODE_JS], post_build=post) # engines has different error stack format

+  def test_noderawfs(self):


tests like these two that don't depend on the compiler mode should be in other (otherwise we run the same test in each mode)

kripken · 2018-01-12T18:18:23Z

src/settings.js

@@ -382,6 +382,10 @@ var NO_FILESYSTEM = 0; // If set, does not build in any filesystem support. Usef
 var FORCE_FILESYSTEM = 0; // Makes full filesystem support be included, even if statically it looks like it is not
                          // used. For example, if your C code uses no files, but you include some JS that does,
                          // you might need this.
+var NODERAWFS = 0; // This dynamically disables virtual filesystem when the build result runs on Node.js


perhaps this should just make the build node.js-only, that seems simpler? is there a benefit to letting it run on non-node with different behavior?

NODERAWFS only replaces low-level APIs, otherwise it depends on original FS methods including FS.read/writeFile. FS.ensureErrnoError, FS.nextfd, etc. I think there is no value to make it node.js-only when we still depend on original FS.

I think it might avoid some possible confusion, though. To be clear, I'm suggesting that when NODERAWFS is enabled, the build will throw at runtime if it is loaded in anything else than node.js.

Otherwise, the possible confusion that worries me is that the build is run in node and in another shell, and they produce very different results. That's currently something that should not happen, which shell you are in should not matter.

which shell you are in should not matter

Yeah... it can be confusing in other shell environments. We may make it throw first and see what's going on.

Updated but probably we cannot run a failure test on CI as 1. we only have Node.js 2. AFAIK the browser test does not support failure test.

Thanks, and yeah, makes sense about testing.

This PR looks good to me now, just one last thing, please update the comment here in settings.js to say it will throw if not in Node.js.

saschanaz · 2018-01-16T05:02:10Z

src/settings.js

@@ -385,7 +385,7 @@ var FORCE_FILESYSTEM = 0; // Makes full filesystem support be included, even if
 var NODERAWFS = 0; // This dynamically disables virtual filesystem when the build result runs on Node.js
                   // and directly depends on behaviors of Node.js File System API.
                   // The initial working directory will be same as process.cwd() instead of VFS root directory.
-                   // The VFS becomes only available for non-Node.js environment.
+                   // The build result will throw when it runs on non-Node.js environment.


This one? @kripken

Yeah, but sorry, I should have been more clear. It's the first statement I think is confusing,

"This dynamically disables virtual filesystem when the build result runs on Node.js"

How about text like "This mode is intended for use with Node.js (and will throw if the build is run in another engine)" at the start, removing "when the build runs on Node.js" (since that is the only intended use).

Great, just pushed a commit for that 😃

kripken · 2018-01-16T18:41:01Z

Great, thanks!

kripken · 2018-01-17T02:48:31Z

I think I see why we missed the test failures here. The last commits are ci skip, so perhaps the breakage starts in them, and also, it looks like github looks at the last commit for showing the build status, it doesn't look back past any ci skips, which makes it harder to see what's going on.

saschanaz · 2018-01-17T03:03:01Z

Oops, are some tests failing on the bots? Strange, I didn't thought the last ci-skipping commits can break anything... (e473771 added else branch for non-Node.js and 1574770 only changed comments)

saschanaz · 2018-01-17T03:12:02Z

test_fs_stream_proto is failing but it seems it's rather related with #5967 rather than this one, as I think this PR has never touched it.

kripken · 2018-01-17T03:17:56Z

Hmm, I bisected test_fs_stream_proto to this commit, but maybe I did something wrong...

I also bisected the other failures (partial list here) to this commit. The error on them is TypeError: Cannot read property 'O_APPEND' of undefined on NODEFS so it does seem related.

saschanaz · 2018-01-17T03:32:00Z

Git blame says the last change in that line is from 9fc522a, which definitely causes type error on Python 3. I wonder why the change. cc: @juj

(A fix is on #6098)

juj · 2018-01-17T12:54:22Z

Git blame says the last change in that line is from 9fc522a, which definitely causes type error on Python 3. I wonder why the change. cc: @juj

Ops, very sorry about this breakage - that was one of those "this is just way too trivial to need a review" so I pushed it in direct. The reason for that change is that the test was failing on Windows where it was writing \r\n newlines to the output, but then tested the file size of the result, which differed from expected on Windows because of those \rs adding in the size. Writing in binary mode causes Windows to omit \r\n but do \n newlines instead. Sorry, I was not familiar with the typedness of these writes on Python 3, that's very good to know.

aheejin · 2018-01-19T23:19:19Z

This is currently breaking out waterfall Mac bots:
asm2wasm log
wasm backend log

Doesn't look like the CI has mac bots. Could you test them on mac?

kripken · 2018-01-19T23:52:24Z

Sadly I don't think travis has mac bots...

Looking at the last error in the second link, it says it's node 8.9.3 so it's probably not a version issue. Likely it really is an OS difference, and NODERAWFS is sensitive to such things. We'd need to debug it on mac I guess. If no one thinks they can get to that soon, we could disable the NODERAWFS tests on mac for now, and maybe update the docs to warn.

aheejin · 2018-01-20T00:18:40Z

I disabled the tests. Not sure which doc to update though.

dschuff · 2018-01-20T00:24:13Z

Side note, Travis actually does have mac bots: see e.g. https://github.com/WebAssembly/wabt/blob/master/.travis.yml

saschanaz · 2018-01-20T06:38:24Z

Looking at the logs, it seems append mode doesn't work on macOS. I don't have any macOS access but how about filing issues on Node.js too?

kripken · 2018-01-22T19:35:59Z

Not sure which doc to update though.

I think the best we have is the settings.js description. I'll update that.

saschanaz added 4 commits December 31, 2017 16:29

initial NODERAWFS implementation

f13f113

fix Ubuntu bug

65c0229

use permission mapper

20b67c4

Node.js supports integer flags but it depends on native system constants, which are different from values on struct_info.compiled.json

test both file read/write

e071850

saschanaz added 11 commits January 4, 2018 11:42

shorter postset

37ab5cf

apply append mode fix

3a95bbf

Merge branch 'incoming' into noderawfs

99f147c

also_with_noderawfs + fs_nodefs_rw

a1eeaf0

nodefs_cloexec

3e074a6

fix flags mapping

1fa114b

fix llseek positioning bug

b9cf0c1

support chdir

20bb015

test_unistd_access

29014e9

short lookupPath, fixes access test

6df292b

fix closure error

97f2d2d

saschanaz added 5 commits January 5, 2018 10:07

add tty property, fix emptyPath test

6c6eec6

move noderawfs test

c537992

fix fclose

65230b9

fallback to VFS for PIPEFS

39059ea

return stats

ffcd591

saschanaz added 6 commits January 5, 2018 21:55

split truncate test

62f3d59

unlink test

94312f3

skip access tests on root

c0a6c63

read in outside

32ba4e9

cleanup

8368f03

return from readlinkSync

e5043ff

juj reviewed Jan 10, 2018

View reviewed changes

Merge branch 'incoming' into noderawfs

1e948c7

saschanaz force-pushed the noderawfs branch from a7bfbd2 to 1e948c7 Compare January 11, 2018 03:56

conditional directories

0869a2a

kripken reviewed Jan 12, 2018

View reviewed changes

saschanaz added 2 commits January 13, 2018 14:35

move to test_other

a7e445c

throw when not node.js [ci skip]

e473771

saschanaz commented Jan 16, 2018

View reviewed changes

revise documentation [ci skip]

1574770

kripken merged commit 99e4fc3 into emscripten-core:incoming Jan 16, 2018

saschanaz deleted the noderawfs branch January 17, 2018 02:58

aheejin mentioned this pull request Jan 20, 2018

Disable NODERAWFS tests on Mac #6121

Closed

aheejin mentioned this pull request Jan 25, 2018

Remove unused OS macros from test_unistd_unlink #6142

Merged

kulp mentioned this pull request Sep 15, 2020

Reduce dependence on abspath kulp/tenyr#62

Merged

kleisauke mentioned this pull request Nov 9, 2022

Ensure that stdin/stdout/stderr are not mapped 1:1 under NODERAWFS #18163

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial NODERAWFS implementation #6008

Initial NODERAWFS implementation #6008

saschanaz commented Dec 31, 2017 •

edited

Loading

kripken commented Jan 3, 2018

kripken commented Jan 3, 2018

juj commented Jan 4, 2018

saschanaz commented Jan 5, 2018

juj Jan 10, 2018

juj Jan 10, 2018

kripken Jan 12, 2018

kripken Jan 12, 2018

saschanaz Jan 13, 2018

kripken Jan 13, 2018

saschanaz Jan 14, 2018

saschanaz Jan 14, 2018 •

edited

Loading

kripken Jan 16, 2018

saschanaz Jan 16, 2018

kripken Jan 16, 2018

saschanaz Jan 16, 2018

kripken commented Jan 16, 2018

kripken commented Jan 17, 2018

saschanaz commented Jan 17, 2018

saschanaz commented Jan 17, 2018

kripken commented Jan 17, 2018

saschanaz commented Jan 17, 2018 •

edited

Loading

juj commented Jan 17, 2018

aheejin commented Jan 19, 2018 •

edited

Loading

kripken commented Jan 19, 2018

aheejin commented Jan 20, 2018

dschuff commented Jan 20, 2018

saschanaz commented Jan 20, 2018

kripken commented Jan 22, 2018

Initial NODERAWFS implementation #6008

Initial NODERAWFS implementation #6008

Conversation

saschanaz commented Dec 31, 2017 • edited Loading

kripken commented Jan 3, 2018

kripken commented Jan 3, 2018

juj commented Jan 4, 2018

saschanaz commented Jan 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saschanaz Jan 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented Jan 16, 2018

kripken commented Jan 17, 2018

saschanaz commented Jan 17, 2018

saschanaz commented Jan 17, 2018

kripken commented Jan 17, 2018

saschanaz commented Jan 17, 2018 • edited Loading

juj commented Jan 17, 2018

aheejin commented Jan 19, 2018 • edited Loading

kripken commented Jan 19, 2018

aheejin commented Jan 20, 2018

dschuff commented Jan 20, 2018

saschanaz commented Jan 20, 2018

kripken commented Jan 22, 2018

saschanaz commented Dec 31, 2017 •

edited

Loading

saschanaz Jan 14, 2018 •

edited

Loading

saschanaz commented Jan 17, 2018 •

edited

Loading

aheejin commented Jan 19, 2018 •

edited

Loading