Skip to content

Add nodefs readdir handling for directories that contain exotic entries #22925

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

Merged
merged 9 commits into from
Dec 4, 2024

Conversation

hoodmane
Copy link
Collaborator

Resolves #22924.

@sbc100
Copy link
Collaborator

sbc100 commented Nov 21, 2024

Looks like this perhaps requires a wasmfs change to match? wasmfs.test_fs_nodefs_readdir is now failing. Looks like maybe wasmfs is reporting "." and ".." but the old FS is not ? Maybe we want go back to the old way of verifying the results to avoid having to fix these decrpencies as part of this PR?

@hoodmane
Copy link
Collaborator Author

Yeah maybe the best thing to do would be to grab the output and make a series of assertions that it contains or does not contain certain strings? It seems that the order is not stable but the set of lines should be.

@sbc100
Copy link
Collaborator

sbc100 commented Nov 21, 2024

Yeah maybe the best thing to do would be to grab the output and make a series of assertions that it contains or does not contain certain strings? It seems that the order is not stable but the set of lines should be.

Sure. That sounds reasonable. I was a little surprised to see who different the wasmfs output was though. I guess there is stuff we need to address there:

--- expected
+++ actual
@@ -1,12 +1,13 @@
 listing contents of dir=/
 .
 ..
+dev
 tmp
-home
-dev
-proc
 listing contents of dir=/working
+.
+..
 existing
+named_pipe
 stdout
 test_nodefs_readdir.js
 test_nodefs_readdir.wasm

Specifically it looks like wasmfs supports . and .. but the old FS doesn't? Also, the home and proc directories should match too. These are all separate issues though.

dev
proc
listing contents of dir=/working
existing
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange that we have . and .. at the root but not in the subdirectory. I guess that just a bug.

@sbc100
Copy link
Collaborator

sbc100 commented Nov 27, 2024

Rebasing could fix the code size failures.

@sbc100 sbc100 enabled auto-merge (squash) November 27, 2024 20:18
@sbc100
Copy link
Collaborator

sbc100 commented Nov 27, 2024

Looks like the wasm2js failure is real. Might be worth switching to assertions rather then comparing output verbatim?

auto-merge was automatically disabled November 27, 2024 23:10

Head branch was pushed to by a user without write access

@hoodmane
Copy link
Collaborator Author

hoodmane commented Dec 4, 2024

@sbc100 can this be merged? Only test failure is test_nodejs_sockets_echo_subprotocol which looks to me like a flake.

@sbc100 sbc100 merged commit 81b7179 into emscripten-core:main Dec 4, 2024
26 of 28 checks passed
@hoodmane hoodmane deleted the nodefs-exotic-files branch December 4, 2024 20:41
@dschuff
Copy link
Member

dschuff commented Dec 5, 2024

This change (at least I suspect it's this change) seems to be failing on Windows:
https://ci.chromium.org/ui/p/emscripten-releases/builders/try/win/b8729460472173187201/overview
Is that expected? Should we disable it there?

@hoodmane
Copy link
Collaborator Author

hoodmane commented Dec 5, 2024

I think that may be caused by #23025? Looks more related to that than to this one.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 5, 2024

Can you make a followup to #23025 that marks test_fs_enotdir as @crossplatform?

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.

NodeFS fails on (folders with) named pipes
3 participants