-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix case insensitive upload (#15245) #26369
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
base: main
Are you sure you want to change the base?
Changes from all commits
83790a7
11c9609
8303655
2841fba
69b327c
9a4f36f
1d1e6f0
519a05d
ab1dd58
b95f524
4ca1484
c7ae52d
dbafd47
b7a4d21
97524da
e95950f
399e6a9
f22817a
7a8f50d
5185579
bb86f51
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| /* | ||
| * Copyright 2026 The Emscripten Authors. All rights reserved. | ||
| * Emscripten is available under two separate licenses, the MIT license and the | ||
| * University of Illinois/NCSA Open Source License. Both these licenses can be | ||
| * found in the LICENSE file. | ||
| */ | ||
|
|
||
| #include <assert.h> | ||
| #include <stdio.h> | ||
|
|
||
| #include <emscripten.h> | ||
|
|
||
| void loadScript() { | ||
| printf("load2"); | ||
| FILE* file = fopen("file1.txt", "r"); | ||
|
|
||
| if (!file) { | ||
| assert(false); | ||
| } | ||
|
|
||
| while (!feof(file)) { | ||
| char c = fgetc(file); | ||
| if (c != EOF) { | ||
| putchar(c); | ||
| } | ||
| } | ||
| fclose(file); | ||
| exit(0); | ||
| } | ||
|
|
||
| void scriptLoadFail() { | ||
| printf("failed to load data_files.js\n"); | ||
| assert(false); | ||
| } | ||
|
|
||
| int main() { | ||
| emscripten_async_load_script("data_files.js", loadScript, scriptLoadFail); | ||
| return 99; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| FS.createDataFile('/', "file.txt", "foo"); | ||
| FS.createDataFile('/', "fILe.txt", "foo2"); | ||
| var fileContents = FS.readFile("/file.txt"); | ||
| out('file.txt: ' + fileContents); | ||
| var ret = FS.analyzePath('/file.txt'); | ||
| out('file.txt collison: ' + ret.object.name_next); | ||
| var errCode = 0; | ||
| try { | ||
| FS.createDataFile('/', "FIlE.txt", "foo2"); | ||
| } catch (e) { | ||
| errCode = e.errno; | ||
| } | ||
| out('errorCode: ' + errCode); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -683,8 +683,12 @@ def generate_preload_js(data_target, data_files, metadata): | |
| } catch (e) { | ||
| err(`Preloading file ${name} failed`, e); | ||
| }\n''' | ||
| create_data = '''// 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); | ||
| create_data = '''try { | ||
| // 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); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there some reason why its important to swallow this error here? Is it needed for this change?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| } | ||
| Module['removeRunDependency'](`fp ${name}`);''' | ||
|
|
||
| finish_handler = create_preloaded if options.use_preload_plugins else create_data | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it still and error when the contents are the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can swallow the error here, although then the caller would not know of the duplicate call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain what you mean by "the duplicate call"?
With this change are we not saying that its OK to upload with
FOO.txtandfoo.txtwithout getting an error. Why would it matter if the contents of the files match?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
case2:
It seems like you are saying only one of these should be an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really not sure what the desired behaviour is based on #15245.