Fix case insensitive upload (#15245)#26369
Fix case insensitive upload (#15245)#26369AlexWalsh2 wants to merge 20 commits intoemscripten-core:mainfrom
Conversation
Signed-off-by: walshal <[email protected]>
test/test_core.py
Outdated
| @also_with_noderawfs | ||
| def test_fs_writev(self): | ||
| self.do_runf('fs/test_writev.c', 'success', cflags=['-sFORCE_FILESYSTEM']) | ||
| self.do_runf('fs/test_case_insensitive.c', 'success', cflags=['-sFORCE_FILESYSTEM', '-sCASE_INSENSITIVE_FS']) |
There was a problem hiding this comment.
This should be separate test case (function)
There was a problem hiding this comment.
I removed this test, other tests are in separate test cases
test/fs/test_case_insensitive.c
Outdated
| char buf1[] = "b=2\n"; | ||
| struct iovec iov[] = {{.iov_base = buf0, .iov_len = 4}, | ||
| {.iov_base = buf1, .iov_len = 4}}; | ||
| ssize_t nwritten = pwritev(fd, iov, 2, 0); |
There was a problem hiding this comment.
Using pwritev here seem needless complicated. Can you just use fwrite with a single string constant?
test/fs/test_case_insensitive.c
Outdated
| struct iovec iov2[] = {{.iov_base = buf3, .iov_len = 4}, | ||
| {.iov_base = buf4, .iov_len = 4}}; | ||
| ssize_t nwritten2 = pwritev(fd2, iov2, 2, 0); | ||
| assert(nwritten2 == 8); |
There was a problem hiding this comment.
Maybe write just 4 bytes (or some number that is not the same as the 8 above)?
test/fs/test_case_insensitive.c
Outdated
| close(fd); | ||
|
|
||
| printf("success\n"); | ||
| return 0; |
There was a problem hiding this comment.
How does this test case fail today (without the libfs patch)?
There was a problem hiding this comment.
This test does not test what I thought it did, I have replaced it with two better tests
Added a test that fails when preloaded files cause the JS to hang Addressed space after if from code review in PR Removed test_case_insensitive (does not test what I expected it to) Signed-off-by: walshal <[email protected]>
Adds test_insensitive_overwrite test to ensure, in case insensitive filesystems, inaccessible prior versions of files are not left around after overwrite Signed-off-by: walshal <[email protected]>
Add catch for throws coming out of file packager's async FS_createDataFile Add overwrite behavior to createDataFile instead of mknod so it can throw an EEXISTS error up if the files have the exact same data Add check for skipping duplicate node creation to the test_insensitive_overwrite test Signed-off-by: walshal <[email protected]>
Fix for test_file_packager Fix for test_insensitive_hang in instance and esm_integration Signed-off-by: walshal <[email protected]>
…mscripten into fix_case_insensitive_upload
Fix typos as indicated by clang-format in C and JS files Signed-off-by: walshal <[email protected]>
test/test_core.py
Outdated
| self.set_setting('DEFAULT_LIBRARY_FUNCS_TO_INCLUDE', ['$FS']) | ||
| self.set_setting('CASE_INSENSITIVE_FS', ['1']) | ||
| self.add_pre_run(read_file(test_file('fs/test_insensitive_overwrite.js'))) | ||
| self.do_run_in_out_file_test('fs/test_insensitive_overwrite.c') |
There was a problem hiding this comment.
I think its probably fine to put these tests in test_other.py.
You can even put them alongside the existing test_fs_icase there
There was a problem hiding this comment.
Should I set test_fs_icase to have the no_windows/no_mac requested for the other test?
There was a problem hiding this comment.
Only if the tests do not pass on mac / windows.
There was a problem hiding this comment.
If you mark them as @crossplatform then CI will run them on mac and windows and you can see if they fail there,.
There was a problem hiding this comment.
First thing, thank you for the suggestion regarding the crossplatform CI test - both tests seem to work fine on the CI's Mac and Windows.
Secondly, if I move the tests inside of test_fs_icase, then they get run with the experimental wasmfs, which causes test_overwrite_icase to fail (pre_run doesn't work the way I'm using it). Do we need to test it against the experimental filesystem?
| // canOwn this data in the filesystem, it is a slice into the heap that will never change | ||
| Module['FS_createDataFile'](name, null, data, true, true, true); | ||
| } catch(e) { | ||
| err(`Preloading file ${name} failed`, e); |
There was a problem hiding this comment.
Is there some reason why its important to swallow this error here? Is it needed for this change?
There was a problem hiding this comment.
There is a case (see #26327) where this error is not caught, causing the old code to hang. Is there a better place to catch it?
| break; | ||
| } | ||
| } | ||
| if (isDup) { |
There was a problem hiding this comment.
Why is it still and error when the contents are the same?
There was a problem hiding this comment.
We can swallow the error here, although then the caller would not know of the duplicate call
There was a problem hiding this comment.
Can you explain what you mean by "the duplicate call"?
With this change are we not saying that its OK to upload with FOO.txt and foo.txt without getting an error. Why would it matter if the contents of the files match?
There was a problem hiding this comment.
By "duplicate call", I mean the second call that is trying to do the same operation (same name same data - createDataFile("FOO.txt, "foo") when we already have "foo.txt" with "foo" inside). My apologies for any lack of clarity on my part.
I am uncertain if it matters - the idea with the current structure is that we basically pass the question up to the calling function via the throw of EEXIST so it can decide what to do in that case of a "duplicate call". Is this worth throwing up an exception or should the function do something else (say, skip to being done or not check and just overwrite)?
There was a problem hiding this comment.
But why would you want to silently ignore the error when the contents are different but hide the error when the contents are the same?
case1:
createDataFile("FOO.txt, "foo")
createDataFile("foo.txt, "foo")
case2:
createDataFile("FOO.txt, "foo")
createDataFile("foo.txt, "bar")
It seems like you are saying only one of these should be an error?
There was a problem hiding this comment.
I made that change based on this review of the previous code. That code handled both case1 and case2 the same (silently ignore/overwrite), is that preferable behavior? This seems to me like an architectural decision - I can implement whatever you want, or pick something on my own.
There was a problem hiding this comment.
I'm really not sure what the desired behaviour is based on #15245.
test_insensitive_hang renamed to test_crash_icase, moved to other, and given windows-only and macos-only flags as they have/can have case-insensitive filesystems by default test_insensitive_overwrite renamed to test_overwrite_icase, and moved to other createDataFile now converts its data to its storage form (char -> char code) before creating the FS node, simplifying data comparison and avoiding duplicated efforts Signed-off-by: walshal <[email protected]>
Refactored overwrite test in test_fs_icase to remove the stub .c file and the .out file, reducing it to a single .js file Signed-off-by: walshal <[email protected]>
Split test_overwrite_icase back out into its own test temporarily, to more easily tell if it is crossplatform Mark both new tests as crossplatform Signed-off-by: walshal <[email protected]>
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
So just the re-wind to the original problem.
Please correct me if my summary is wrong:
FS.createDataFiledoes not currently allow the overwriting of files.- If
CASE_INSENSITIVE_FSused thenFS.createDataFile(FOO.txt)followed byFS.createDataFile(foo.txt)will currently crash. - You have a file packager bundle that contains such a cominbation of files that it cannot currently be loaded when
CASE_INSENSITIVE_FSis set.
Is that right?
I wonder if we simply changed FS.createDataFile so that it always clobbers existing files.. would that be a bad breaking change for folks in the wild?
Keep test_overwrite_icase outside of test_fs_icase for now since it fails with wasmfs Move test_crash_icase inside of test_fs_icase Keep crossplatform flags on icase tests - they pass as of now Signed-off-by: walshal <[email protected]>
2 participants
When files with the same case-insensitive filename are uploaded, the newest file overwrites the older one. Includes test for #15245, could fix #26317