Implement open() in library_wasmfs.js #18919

jameshu15869 · 2023-03-08T04:16:08Z

Implemented FS.open() in library_wasmfs.js
Added tests to test FS.open() with different flags.

Added FS.open() to library_wasmfs.js and wrote tests to make sure files are being opened with different flags.

tlively

Looks pretty good, thanks! A couple notes/questions:

It looks like test_open_wasmfs.out should at least have "success" in it.
Are there existing tests that use FS.open that we can already run with WasmFS enabled instead of adding a new test?

tlively · 2023-03-08T17:13:22Z

test/fs/test_open_wasmfs.c

@@ -0,0 +1,37 @@
+/*
+ * Copyright 2018 The Emscripten Authors.  All rights reserved.


Suggested change

* Copyright 2018 The Emscripten Authors. All rights reserved.

* Copyright 2023 The Emscripten Authors. All rights reserved.

tlively · 2023-03-08T17:20:42Z

src/library_wasmfs.js

+      // According to the existing File System docs, Emscripten does not
+      // support user permissions
+
+      // "The underlying implementation does not support user 
+      // or group permissions. The file permissions set in mode 
+      // are only used if the file is created. The caller is always 
+      // treated as the owner of the file, and only those permissions apply."
+


Suggested change

// According to the existing File System docs, Emscripten does not

// support user permissions

// "The underlying implementation does not support user

// or group permissions. The file permissions set in mode

// are only used if the file is created. The caller is always

// treated as the owner of the file, and only those permissions apply."

Quoting the docs here probably isn't necessary, and runs the risk of going stale.

I really appreciate your code review. Thank you. I have answered your questions below.

I remember I originally created test_open_wasmfs.out because of a "file not found" error when I was running a different test. It looks like the method self.do_runf() takes in an expected output that it checks on stdout without any need to create out files.

emscripten/test/common.py

Lines 1280 to 1281 in 496a4f4

def do_runf(self, filename, expected_output=None, **kwargs):

return self._build_and_run(filename, expected_output, **kwargs)

I think the .out file is necessary for running tests with self.do_run_in_out_file_test", so I deleted the .out file and the test still passed. I'm calling test_fs_open_wasmfs with self.do_runf() (The one above).

emscripten/test/common.py

Lines 1287 to 1298 in 496a4f4

def do_run_in_out_file_test(self, *path, **kwargs):

srcfile = test_file(*path)

out_suffix = kwargs.pop('out_suffix', '')

outfile = shared.unsuffixed(srcfile) + out_suffix + '.out'

if EMTEST_REBASELINE:

expected = None

else:

expected = read_file(outfile)

output = self._build_and_run(srcfile, expected, **kwargs)

if EMTEST_REBASELINE:

write_file(outfile, output)

return output

I did find some other test files that have FS.open(): test_llseek.c, test_nodefs.c, and test_write.cpp. All three tests call some fs functions that aren't yet in library_wasmfs.js yet so the tests all failed.

Note - I noticed that my pull request code fails the test-wasm64 in circleci due to

TypeError: Cannot convert 65488 to a BigInt at /tmp/emtest_89oyqghg/emscripten_test_wasm64l_4uf_hulx/test_open_wasmfs.js:909:22

From what I'm understanding, this seems to be an error with the way I wrote my test, rather than the wasmfs library. However, is it possible that my actual wasmfs code has some issues?

Thanks, adding a new test file sounds good to me, then.

I'm not sure off the top of my head what could be going wrong with the wasm64 test. It would be good to dig into that more. Can you at least reproduce the problem locally?

Edited comments in test_open_wasmfs.c and library_wasmfs.js Added additional test for test_write_wasmfs that currently will not pass

tlively · 2023-03-09T16:08:53Z

test/test_core.py

+  def test_fs_write_wasmfs(self):
+    self.emcc_args += ['-sWASMFS']
+    self.emcc_args += ['-sFORCE_FILESYSTEM']
+    self.do_run_in_out_file_test('fs/test_write.cpp')


There is an @also_with_wasmfs_js decorator you can attach to the previous test function instead of having to write a new test function here.

kripken

Nice work!

Looks good aside from the open comments and discussion.

kripken · 2023-03-09T19:23:24Z

test/fs/test_open_wasmfs.c

+
+int main() {
+  EM_ASM(
+    FS.writeFile('testfile', 'a=1\nb=2\n'); // writeFile already works with WasmFS


Suggested change

FS.writeFile('testfile', 'a=1\nb=2\n'); // writeFile already works with WasmFS

FS.writeFile('testfile', 'a=1\nb=2\n');

(that comment helps read this PR, but it doesn't seem like it would be useful after this PR lands, so it could've been in the PR as a comment and not in the source code)

I've removed this comment. I'll make sure to keep this in mind going forward!

jameshu15869 · 2023-03-10T04:03:26Z

@tlively @kripken
I think there might be more this pull request than just calling __syscall_open in library_wasmfs.js. It looks like the original FS.open() doesn't return a file descriptor like the __syscall_open() does. Instead, it looks like the JS FS.open() does some processing before finally returning a stream object (Code below, this object is returned at the end of the JS FS.open()).

emscripten/src/library_fs.js

Lines 1076 to 1086 in 2f70f77

    
           var stream = FS.createStream({ 
        
             node: node, 
        
             path: FS.getPath(node),  // we want the absolute path to the node 
        
             flags: flags, 
        
             seekable: true, 
        
             position: 0, 
        
             stream_ops: node.stream_ops, 
        
             // used by the file family libc calls (fopen, fwrite, ferror, etc.) 
        
             ungotten: [], 
        
             error: false 
        
           });

I found this after trying the @with_wasmfs_js flag you suggested, realizing that my asserts for FS.open() didn't work with the JS FS.open() because I was returning a file descriptor (An integer like __syscall_open() does) instead of a stream object. Based on my current understanding, we would have to do all the processing that the existing JS FS.open()does in library_wasmfs.js to make a new stream object to preserve compatibility, right?

I think this means that the FS.open() in library_wasmfs.js should be more complicated than it is now. For example, the existing JS FS.open() uses mknod and does things like modifying the flags using bit operations before returning a stream object. However, please let me know if I'm misunderstanding the problem.

tlively · 2023-03-10T16:54:36Z

@jameshu15869, good point. Exactly mimicking the stream objects from the legacy API would be a lot of work, but is also necessary for full compatibility for existing code. Maybe we can add a TODO about that and land this PR, then handle that work in a follow-up?

kripken · 2023-03-10T18:10:15Z

+1 for @tlively 's comment.

Also, I'd say that in general there is an open question here about how much we should match the old JS API. Emitting JS objects that look identical to before would seem like the obvious thing to do, but it might not be necessary and might also be very difficult to do (some might require more calls into wasm to get info, and the info might need reshaping perhaps). Seeing what the current tests require and doing the minimum for them might be ok. But we can also discuss each case specifically to find the best approach, and we'll be flexible here.

Cleaned up remaining discussion comment in test_open_wasmfs.c Combined writeFile tests for JS and WasmFS into 1 test using @also_with_wasmfs

jameshu15869 · 2023-03-11T04:43:36Z

@tlively @kripken
That sounds good. For now, I've made the changes to the commenting and test structure and committed to my branch.

I wasn't able to reproduce the CI error locally. Does CircleCI just run the testing files "as-is"? Or does it run against some kind of larger input or something? I've tried searching for 65488 (The integer that causes an error) in the code and there seems to be not relevant results.

tlively · 2023-03-13T15:41:49Z

Yeah, CircleCI runs the same tests you should be able to run locally. When you try to reproduce, are you using ./test/runner wasm64l.test_fs_open_wasmfs? The wasm64l is the key piece here, since this error is specific to wasm64. It's not surprising that you can't find 65488 in the code base; that's probably a pointer value.

To debug this further, I would run with ./test/runner --verbose wasm64l.test_fs_open_wasmfs, copy the build command from the output, and build and run the test manually, adding --profiling to the build command to make sure function names are preserved.

jameshu15869 · 2023-03-14T02:59:01Z

@tlively
Sounds good. My schedule for the next few days is currently a little busy, but I'll make sure to get back to you with the results as soon as I can.

Added to64 wrapper around buffer to solve problem with wasm64l testing locally.

jameshu15869 · 2023-03-18T15:46:18Z

@tlively @kripken
I found the error! I needed to wrap the buffer in {{{ to64 }}} before passing it to the js_api.cpp. All the CircleCI tests passed and I'm ready for another code review.

tlively

LGTM, thanks!

jameshu15869 · 2023-03-20T16:16:51Z

@tlively @kripken
Thank you both so much for helping me out through this pull request. I definitely learned a lot and really enjoyed the process. Should I just delete my branch now?

sbc100 · 2023-03-20T16:50:20Z

test/test_core.py

    self.emcc_args += ['-sWASMFS']
    self.emcc_args += ['-sFORCE_FILESYSTEM']
-    self.do_run_in_out_file_test('fs/test_writeFile.cpp')
+    self.do_runf(test_file('fs/test_open_wasmfs.c'), 'success')


@tlively @kripken as we increase support for the JS API does it make sense to keep adding more tests like this? Shouldn't we be enabling existing tests that use the JS API?

There is nothing wasmfs specific about this test, right? Are we missing existing tests of the JS open API?

James wrote,

I did find some other test files that have FS.open(): test_llseek.c, test_nodefs.c, and test_write.cpp. All three tests call some fs functions that aren't yet in library_wasmfs.js yet so the tests all failed.

So we have a bootstrapping problem with our tests. Once we have better API coverage we will be able to stop writing new tests and will be able to consolidate our existing tests.

We had/have similar test bootstrapping problems with the WasmFS syscall API as well.

tlively · 2023-03-20T18:35:48Z

@jameshu15869, thanks for your work on this! I'm glad you enjoyed it. Yes, you can go ahead and delete the branch.

jameshu15869 added 2 commits March 7, 2023 23:13

Add open() to library_wasmfs.js

dc49256

Added FS.open() to library_wasmfs.js and wrote tests to make sure files are being opened with different flags.

Delete settings.json

4004253

jameshu15869 mentioned this pull request Mar 8, 2023

WasmFS JS API #15976

Open

tlively reviewed Mar 8, 2023

View reviewed changes

Edit code commenting/syntax

e0520a3

Edited comments in test_open_wasmfs.c and library_wasmfs.js Added additional test for test_write_wasmfs that currently will not pass

tlively reviewed Mar 9, 2023

View reviewed changes

kripken reviewed Mar 9, 2023

View reviewed changes

Clean Up Comments and Combine Tests

40e73b1

Cleaned up remaining discussion comment in test_open_wasmfs.c Combined writeFile tests for JS and WasmFS into 1 test using @also_with_wasmfs

jameshu15869 and others added 2 commits March 17, 2023 23:15

Add "to64" wrapper around buffer in open()

413e426

Added to64 wrapper around buffer to solve problem with wasm64l testing locally.

Merge branch 'main' into library-wasmfs-open

bc432a1

tlively approved these changes Mar 20, 2023

View reviewed changes

tlively enabled auto-merge (squash) March 20, 2023 16:14

tlively merged commit 5db9a87 into emscripten-core:main Mar 20, 2023

sbc100 reviewed Mar 20, 2023

View reviewed changes

jameshu15869 deleted the library-wasmfs-open branch March 29, 2023 01:08

		@@ -0,0 +1,37 @@
		/*
		* Copyright 2018 The Emscripten Authors. All rights reserved.

	* Copyright 2018 The Emscripten Authors. All rights reserved.
	* Copyright 2023 The Emscripten Authors. All rights reserved.

	def do_runf(self, filename, expected_output=None, **kwargs):
	return self._build_and_run(filename, expected_output, **kwargs)

	def do_run_in_out_file_test(self, path, *kwargs):
	srcfile = test_file(*path)
	out_suffix = kwargs.pop('out_suffix', '')
	outfile = shared.unsuffixed(srcfile) + out_suffix + '.out'
	if EMTEST_REBASELINE:
	expected = None
	else:
	expected = read_file(outfile)
	output = self._build_and_run(srcfile, expected, **kwargs)
	if EMTEST_REBASELINE:
	write_file(outfile, output)
	return output

	FS.writeFile('testfile', 'a=1\nb=2\n'); // writeFile already works with WasmFS
	FS.writeFile('testfile', 'a=1\nb=2\n');

Implement open() in library_wasmfs.js #18919

Implement open() in library_wasmfs.js #18919

Uh oh!

Conversation

jameshu15869 commented Mar 8, 2023

Uh oh!

tlively left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jameshu15869 Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jameshu15869 commented Mar 10, 2023

Uh oh!

tlively commented Mar 10, 2023

Uh oh!

kripken commented Mar 10, 2023

Uh oh!

jameshu15869 commented Mar 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlively commented Mar 13, 2023

Uh oh!

jameshu15869 commented Mar 14, 2023

Uh oh!

jameshu15869 commented Mar 18, 2023

Uh oh!

tlively left a comment

Choose a reason for hiding this comment

Uh oh!

jameshu15869 commented Mar 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlively commented Mar 20, 2023

Uh oh!

Uh oh!

jameshu15869 Mar 9, 2023 •

edited

Loading

jameshu15869 commented Mar 11, 2023 •

edited

Loading