-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[browser][file system] Tests System.IO.FileSystem #39768
Conversation
{ | ||
protected static int geteuid() | ||
{ | ||
throw new PlatformNotSupportedException(); |
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.
What do you think about returning 0 here and eliminating the platform checks?
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 doing that and modifying the places that use geteuid
the tests fail with:
[FAIL] System.IO.Tests.File_AppendAllLinesAsync.WriteToReadOnlyFile
�[40m�[37mdbug�[39m�[22m�[49m: test[0]
System.UnauthorizedAccessException : Access to the path '/tmp/File_AppendAllLinesAsync_mqajhnoo.uqp/WriteToReadOnlyFile_105_93d848c1' is denied.
�[40m�[37mdbug�[39m�[22m�[49m: test[0]
---- System.IO.IOException : Permission denied
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.
The pal uses sendFile
and emscripten's implementation doesn't seem too friendly w/ read only files. The error message refers to not being able to write to the destination file.
If you skip this block
runtime/src/libraries/Native/Unix/System.Native/pal_io.c
Lines 1152 to 1191 in 05f17e3
#if HAVE_SENDFILE_4 | |
// On 32-bit, if you use 64-bit offsets, the last argument of `sendfile' will be a | |
// `size_t' a 32-bit integer while the `st_size' field of the stat structure will be off64_t. | |
// So `size' will have to be `uint64_t'. In all other cases, it will be `size_t'. | |
uint64_t size = (uint64_t)sourceStat.st_size; | |
// Note that per man page for large files, you have to iterate until the | |
// whole file is copied (Linux has a limit of 0x7ffff000 bytes copied). | |
while (size > 0) | |
{ | |
ssize_t sent = sendfile(outFd, inFd, NULL, (size >= SSIZE_MAX ? SSIZE_MAX : (size_t)size)); | |
if (sent < 0) | |
{ | |
if (errno != EINVAL && errno != ENOSYS) | |
{ | |
tmpErrno = errno; | |
close(outFd); | |
errno = tmpErrno; | |
return -1; | |
} | |
else | |
{ | |
break; | |
} | |
} | |
else | |
{ | |
assert((size_t)sent <= size); | |
size -= (size_t)sent; | |
} | |
} | |
if (size == 0) | |
{ | |
copied = true; | |
} | |
// sendfile couldn't be used; fall back to a manual copy below. This could happen | |
// if we're on an old kernel, for example, where sendfile could only be used | |
// with sockets and not regular files. | |
#endif // HAVE_SENDFILE_4 |
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 have gone through the build and it looks like this code block is already being skipped in the native pal.
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.
You're right. I was looking at the wrong place thinking it was included.
We should trace to see what call is bad.
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.
agreed
You also need to remove the test suite from |
@@ -24,6 +24,7 @@ protected virtual void Move(DirectoryInfo sourceDir, string destDir) | |||
#region UniversalTests | |||
|
|||
[Fact] | |||
[PlatformSpecific(~TestPlatforms.Browser)] |
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.
Same question here
Thanks Maxim was just waiting for PR #39763 because it would have failed anyway |
src/libraries/System.IO.FileSystem/tests/File/ReadWriteAllBytes.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem/tests/FileStream/ReadWriteSpan.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem/tests/File/ReadWriteAllBytes.cs
Outdated
Show resolved
Hide resolved
- WebAssembly (BROWSER) has dirent d_type but is not identifying correctly but by returning UNKNOWN the managed code properly stats the file to detect if entry is directory or not.
There were a few places in |
Could you check the files that are in the PR. I thought I got all of them. |
- PR #40310 works around the issue for now while waiting for fix emscripten-core/emscripten#11804 / emscripten-core/emscripten#11812
Currently the results are: |
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.
lgtm, just a couple of minor comments
Added 45
[PlatformSpecific(~TestPlatforms.Browser)]
checks to tests.Directory.CreateDirectory.cs
DirectoryWithComponentLongerThanMaxComponentAsPath_ThrowsException
for Browser platform.DirectoryWithComponentLongerThanMaxComponentAsPath_BrowserDoesNotThrowsException
for Browser platform in case this check is added in the future.Error number:#39955 - ResolvedFile locking not supported: #40065
A FIFO special file is similar to a pipe, except that it is created in a different way. Instead of being an anonymous communications channel, a FIFO special file is entered into the file system by calling mkfifo(). Pipes are not supported on WebAssembly. Platform specific skip for browser in the following tests.
File rename issue reporting wrong error:#40305PR #40310 works around the issue for now while waiting for fix emscripten-core/emscripten#11804 / emscripten-core/emscripten#11812
Depends on PR: #39763- merged