[PHP-wasm] File mounting in NODEFS

[bgrgicak]

Motivation for the change, related issues

We accidentally added support for mounting files to the Playground CLI by creating a directory with the file path and mounting the file into that node.

This PR removes the accidental mount implementation and adds file mounting support to createNodeFsMountHandler by detecting the mount type and creating the correct node type (symlink, file, directory).

Because Emscripten allows only the mounting of directories, we had to remove the directory check from FS.mount during PHP-wasm compile time.

To ensure mounting works as expected for files, directories, and symlinks, this PR also adds mount tests that include file system operations like reading files, modifying files, deleting, moving, and unmounting.

While writing tests I found two bugs and addressed them in this PR:

• When attempting to remove a mountpoint node FSHelpers.rmdir would first remove all files and directories from the mountpoint and fail to remove the mountpoint in the end. To address this, we now detect if a directory we are attempting to delete is a mount point and throw an error early. The same applies for moving a mount point.
• FSHelpers.copyRecursive copied files and directories, but skipped symlinks. Now the function will also copy symlinks.

Testing Instructions (or ideally a Blueprint)

• CI

[adamziel]

Interesting!

It gets more interesting:

	fs.writeFileSync(
		import.meta.dirname + '/file.txt',
		'Hello, world!'
	);
	php.mkdirTree('/tmp/file.txt');
	await php.mount(
		'/tmp/file.txt',
		createNodeFsMountHandler(import.meta.dirname + '/file.txt'),
	);
	console.log(php.readFileAsText('/tmp/file.txt'));
	php.unlink('/tmp/file.txt');

throws:

Error: Could not unlink "/tmp/file.txt": There is a directory under that path.

Apparently we can read that path as a file, but we can't delete it. I bet the reading is somehow routed to NODEFS while deleting is routed to MEMFS. Interesting!

[adamziel]

I went through the filesystem implementation and it seems like supporting file mounts may actually be easy.

If we comment out the isDir check:

                                if (!FS.isDir(node.mode)) {
-                                       throw new FS.ErrnoError(54);
+                                       // throw new FS.ErrnoError(54);
                                }

And create an empty file first, e.g. php.writeFile('/tmp/file.txt', ''); here's what happens:

• We get a FS node with /tmp as a parent and MEMFS node_ops and stream_ops.
• We also a FS node with null parent and NODEFS node_ops and stream_ops.
• The first node has a mounted.root property set to the second node
• All FS.lookupPath('/tmp/file.txt') calls find the NODEFS node thanks to this part of FS.lookupPath():

					if (
						FS.isMountpoint(current) &&
						(!islast || opts.follow_mount)
					) {
						current = current.mounted.root;
					}

• Undefined operations such as unlinking, moving, etc. of /tmp/file.txt are blocked by ample checks baked into Emscripten (if (FS.isMountpoint(node)) { throw new FS.ErrnoError( ERRNO_CODES.EBUSY ); }).
• Both php.writeFile('/tmp/file.txt', 'Updated from PHP'); and fs.writeFileSync(root + '/file.txt', 'Updated via Node.js fs module'); change the underlying file in the host filesystem.
• Moving the parent directory (php.mv('/tmp', '/tmp2');) doesn't seem to break anything. Reading and writing from the file continues to work.
• It seems to work even if I mount a symlink

---

Let's try to comment out that check via replace.sh in Dockerfile and come up with a very aggressive test suite that tries to break the minutest details of the filesystem ops when such a mount is used. Then let's also try mounting the sqlite database as a single file. If we can't break it, maybe that's it!

[bgrgicak]

I commented out that check via replace.sh in the Dockerfile and added support for creating file and directory nodes inside createNodeFsMountHandler.

My next step is to remove the node after unmounting and add tests.

Thanks for the research @adamziel!

[adamziel]

Should we automatically remove the node? I'm thinking about preserving it. It's a fairly low level api, being explicit is more useful than being implicit

[bgrgicak]

Should we automatically remove the node? I'm thinking about preserving it. It's a fairly low level api, being explicit is more useful than being implicit

Good timing, I just added node removal to unmount d18f5fa.
It deletes only the node created by mount, if the node existed, it won't remove it.

[adamziel]

I wonder – could we just borrow some of the Emscripten test suite for files to test this?

[bgrgicak]

I wonder – could we just borrow some of the Emscripten test suite for files to test this?

Here are the Emscripten mount tests, they seem basic and I think that we could do a bit more with our tests.

Here's a list of tests I had in mind.

• File
  • Should mount file
  • Should throw an error when mounting to an existing file
  • Should exist
  • Should be editable
  • Should not be deletable
  • Should not be movable
  • Should not exist after unmounting
  • Should remount after unmounting
• Directory
  • Should mount directory
  • Should throw an error when mounting to an existing directory
  • Should exist
  • Should be listable
  • Should be editable
    • Create dir
    • Move dir (rename)
    • Remove dir
    • Add file
    • Edit file
    • Remove file
  • Should not be deletable
  • Should not be movable
  • Should not exist after unmounting if it didn't exist before mounting
  • Should exist after unmounting if if existed before mounting
  • Should remount after unmounting
• Symlinked file
  • Same as File tests
• Symlinked directory
  • Same as directory tests

[adamziel]

That's a great list of tests @bgrgicak, let's do it! I'm kind of on the fence on File should not be deletable but I like it as a starting point. It's a stricter rule than the alternative. If it's ever a problem, we can relax it.

[bgrgicak]

That's a great list of tests @bgrgicak, let's do it! I'm kind of on the fence on File should not be deletable but I like it as a starting point. It's a stricter rule than the alternative. If it's ever a problem, we can relax it.

I will compare all of my tests with POSIX to match the spec, or start a thread about differences.

[adamziel]

Are we certain this only affects that one instance? Maybe let's add a "--exactly-one-match" or similar flag to replace-across-lines.sh to make sure we're not replacing too much.

[bgrgicak]

Are we certain this only affects that one instance?

Today yes, but that might change in the future.

I tried a few things to add --exactly-one-match to replace-across-lines.sh but it seemed overly complicated, especially compared to removing the g argument from the regex like I ended up doing in cbdc543.

[bgrgicak]

Copy recursive didn't copy symlinks.

[bgrgicak]

@adamziel this is now ready for review. I'm just waiting on PHP-wasm Node to recompile.

[brandonpayton]

👋 Hi @bgrgicak, I plan to review this yet today.

[brandonpayton]

I left some comments and questions, but this looks good to me. The biggest question I have is whether we should be mounting files at all, but it seems useful. And I am not aware of a downside.

[adamziel]

Good question! It is a natural enough interaction that people keep trying to mount plugin files or wp-config and get confused when it doesn't work. If it's easy for us to support it, let's support it.

Feel free to proceed here once you feel it's ready :)

[bgrgicak]

Thanks for the feedback! I think this PR is in a good place after addressing it.

[bgrgicak]

I'm not sure how I missed this while testing, but there was a condition in createNodeFsMountHandler that would never be used, so I removed it. #2379