[nodefs] Fix execute permission bit on Windows when retrieving mode attribute #21902
Conversation
| @@ -127,6 +127,11 @@ addToLibrary({ | |||
| if (NODEFS.isWindows && !stat.blocks) { | |||
| stat.blocks = (stat.size+stat.blksize-1)/stat.blksize|0; | |||
| } | |||
| if (NODEFS.isWindows) { | |||
There was a problem hiding this comment.
Can you combine this with the block above so we only check isWindows once?
There was a problem hiding this comment.
I applied this suggestion in ef0ec92.
test/test_other.py
Outdated
| def test_windows_nodefs_execution_permission(self): | ||
| if self.get_setting('WASMFS'): | ||
| self.set_setting('FORCE_FILESYSTEM') | ||
| delete_dir('new-dir') |
There was a problem hiding this comment.
You don't need to do this here since tests are guaranteed to start with an empty dir
test/test_other.py
Outdated
| } | ||
| '''.replace('{{WORKING_DIR}}', Path(self.working_dir).as_posix()) | ||
| self.emcc_args += ['-lnodefs.js'] | ||
| self.do_run(src) |
There was a problem hiding this comment.
You can combine these two lines into self.do_run(src, emcc_args=['-lnodefs.js'])
test/test_other.py
Outdated
| int main(int argc, char * argv[]) { | ||
| EM_ASM( | ||
| FS.mkdir('/working'); | ||
| FS.mount(NODEFS, { root: '{{WORKING_DIR}}' }, '/working'); |
There was a problem hiding this comment.
I think you can just do { root: '.' } here? Unless I'm missing something?
There was a problem hiding this comment.
Ah, true. I think I introduced this early on when I was running the test without using the test runner. Now using the test runner, I confirmed that using { root: '.' } works as expected. I'll update it.
| FS.mount(NODEFS, { root: '{{WORKING_DIR}}' }, '/working'); | ||
| FS.mkdir('/working/new-dir'); | ||
| FS.writeFile('/working/new-dir/test.txt', 'test'); | ||
| ); |
There was a problem hiding this comment.
Can you put this EM_ASM block into a separate setup() function?
test/test_other.py
Outdated
| @only_windows('Check that directory permissions are properly retrieved on Windows') | ||
| @requires_node | ||
| def test_windows_nodefs_execution_permission(self): | ||
| if self.get_setting('WASMFS'): |
There was a problem hiding this comment.
I think this will always be false unless you add @also_with_wasmfs. I'm not sure if you want to do that here though.
There was a problem hiding this comment.
I added this part based on what I saw in other tests (example). Not exactly sure if it's needed but we could remove it if it's not 👍.
test/test_other.py
Outdated
| @requires_node | ||
| def test_windows_nodefs_execution_permission(self): | ||
| if self.get_setting('WASMFS'): | ||
| self.set_setting('FORCE_FILESYSTEM') |
There was a problem hiding this comment.
Is this needed? Maybe just remove these two lines?
There was a problem hiding this comment.
As shared here, probably not. I'll remove it.
test/test_other.py
Outdated
| err = stat("/working/new-dir/test.txt", &s); | ||
| assert((s.st_mode & S_IXUSR) == S_IXUSR); | ||
| assert((s.st_mode & S_IXGRP) == S_IXGRP); | ||
| assert((s.st_mode & S_IXOTH) == S_IXOTH); |
There was a problem hiding this comment.
I think you can just do assert(s.st_mode & S_IXOTH) since each of those defines is just a single bit
src/library_nodefs.js
Outdated
| if (NODEFS.isWindows) { | ||
| // Node.js on Windows never represents permission bit 'x', so | ||
| // propagate read bits to execute bits. | ||
| stat.mode = stat.mode | (stat.mode & 292) >> 2; |
There was a problem hiding this comment.
Can you put brackets around the (stat.mode & 292) >> 2 so its clear that the shift only applies the part of this expression?
There was a problem hiding this comment.
I noticed you updated the code where this is based in #21904. I'll apply the same here. Thanks 🙇 !
I noticed this while reviewing emscripten-core#21902.
I noticed this while reviewing #21902.
|
@sbc100 thank you so much for reviewing the PR so quickly 🙇 ! I've checked the feedback and pushed changes accordingly. Let me know if you could take another look, thanks! |
sbc100
changed the title
# Conflicts: # src/library_nodefs.js
On Windows, files and directories created within a mounted
NODEFSinstance consistently have the execution permission bitxdisabled, even though they are actually accessible. The permissions returned consistently indicate0666. If this value is used to create other files or directories to match the same permissions, they will possess a different value, potentially leading to inconsistencies.This PR applies a solution similar to #5847, albeit within the
getattrfunction ofNODEFS.