Skip to content

Conversation

juj
Copy link
Collaborator

@juj juj commented Aug 24, 2025

Windows does not support fifos, so the test expectation needs to look different.

@@ -5868,7 +5868,7 @@ def test_fs_nodefs_readdir(self):
self.cflags += ['-lnodefs.js']
suffix = ''
if self.get_setting('WASMFS'):
suffix = '.wasmfs'
suffix = '.wasmfs_win' if WINDOWS else '.wasmfs'
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIUC the issue here is the order in which directories are listed?

I wonder if we can instead have either:

  1. Have wasmfs list directories in a deterministic / alphabetical order
  2. Update the test to avoid depending on specific ordering.

Adding another separate copy of the expectations seems less desirable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not an ordering problem, but a problem that Windows does not support FIFO objects on the filesystem, so the test is unable to add one. And hence fails to readdir() it in the filesystem on Windows.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps add a comment as to why these three different output are needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

check

@juj juj force-pushed the fix_wasmfs_test_fs_nodefs_readdir_windows branch from 02feecb to 99173ae Compare August 25, 2025 11:14
@juj
Copy link
Collaborator Author

juj commented Aug 26, 2025

Ping, any further thoughts here?

@sbc100
Copy link
Collaborator

sbc100 commented Aug 26, 2025

In general, I don't love the solution of having get more different output expectation file, since it makes it hard to see how/why the different configs have different output.

But I don't see any other great options here.

@juj juj merged commit f540c49 into emscripten-core:main Aug 26, 2025
2 of 14 checks passed
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.

2 participants