Skip to content

Conversation

@rchiodo
Copy link

@rchiodo rchiodo commented Sep 8, 2022

Fixes #17819

Fixes problems with windows paths when running node builds on windows machines.

@rchiodo
Copy link
Author

rchiodo commented Sep 8, 2022

This is also related to python/cpython#96509

CPython uses opendir to read files for import so it needs this to work in order for the node build to be portable.

@rchiodo rchiodo changed the title Use node's path.isAboslute when running with a node fs Use node's path.isAbsolute when running with a node fs Sep 9, 2022
@rchiodo
Copy link
Author

rchiodo commented Sep 9, 2022

Sorry I didn't add any tests because I wasn't sure how to detect if on windows or not. If there's a way to do so, I think the src/test/test_readdir.c would be a good spot.

@tiran
Copy link
Contributor

tiran commented Sep 11, 2022

node's os module perhaps?

> require('os').platform()
'linux'

@rchiodo
Copy link
Author

rchiodo commented Sep 12, 2022

Sorry I meant in the c code. Maybe it doesn't need to do that though. I can have the python code detect os and only run certain tests on windows.

@rchiodo
Copy link
Author

rchiodo commented Sep 13, 2022

@tiran do you know who I might ping to get this reviewed? I managed to figure out how to add a windows only test.

@rchiodo
Copy link
Author

rchiodo commented Sep 13, 2022

@kripken @sbc100 can either of you take a look? Thanks.

@kleisauke
Copy link
Collaborator

I just opened an alternative PR for this at #17849.

@rchiodo
Copy link
Author

rchiodo commented Sep 14, 2022

Closing in favor of @kleisauke's #17849

@rchiodo rchiodo closed this Sep 14, 2022
kripken pushed a commit that referenced this pull request Nov 7, 2022
Ensures that absolute paths are handled correctly on Windows as well. Using these helpers
should be safe, since binaries linked with -sNODERAWFS can only be used on Node.js, see:

emscripten/src/library_noderawfs.js

Line 28 in e98554f
       throw new Error("NODERAWFS is currently only supported on Node.js environment.") 

Context: #16296
Resolves: #17360.
Resolves: #17819.
Supersedes: #17821.
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.

opendir is not portable to windows for NODERAWFS

3 participants