-
Notifications
You must be signed in to change notification settings - Fork 201
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
add realpath.c to LIBC_TOP_HALF_MUSL_SOURCES
#473
Conversation
In WebAssembly#463, I added stubs for `statvfs`, `chmod`, etc. but forgot to add one for `realpath`, which is also required by `libc++`'s `<filesystem>` implementation. Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Providing such stubs does seems like a turnaround in policy for wasi-libc. Historically we have chosen instead so simply not provide implementations of functions that we don't support, and force downstream users of these functions to instead patch their codebases. I probably should have baught this up in #463. @sunfishcode I am wrong about my understanding on the status quo? Are we ok making an exception in this one case or are we now OK with stub functions in general? |
@sbc100 for context: WebAssembly/wasi-sdk#373 (comment) |
So its OK to add stubs as long as we plan on implemented them some day? That seem like quite a departure from the previous policy. I know #373 already landed, so maybe this discussion is simply just too late, but I'm not sure why we shouldn't instead push for a patch to libc++ to avoid depending on functions we can't / don't implement yet. That would seem to fit better with existing policy. |
I think a In the past I've tended to oppose stubs for things that were simply not implemented. Part of the motivation for this was that we had a lot of wide-open questions about WASI should even be, and what wasi-libc's roll in it should be, and I wanted to err on the side of being conservative. But, I've gotten a variety of feedback that that it often causes more trouble than benefit. And, we have a lot more answers about WASI now. And in the case of |
I thought we can just use |
libc-bottom-half/sources/posix.c
Outdated
// TODO: We plan to support this eventually in WASI, but not yet. | ||
// Meanwhile, we provide a stub so that libc++'s `<filesystem>` | ||
// implementation will build unmodified. | ||
return NULL; |
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.
realpath is supposed to set errno
when it fails.
I'm not sure which one would make sense here. The man pages says:
ERRORS
EACCES Read or search permission was denied for a component of the path prefix.
EINVAL path is NULL. (Before glibc 2.3, this error is also returned if resolved_path is
NULL.)
EIO An I/O error occurred while reading from the filesystem.
ELOOP Too many symbolic links were encountered in translating the pathname.
ENAMETOOLONG
A component of a pathname exceeded NAME_MAX characters, or an entire pathname ex‐
ceeded PATH_MAX characters.
ENOENT The named file does not exist.
ENOMEM Out of memory.
ENOTDIR
A component of the path prefix is not a directory.
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.
Good catch -- sorry I missed that. I was looking at the RETURN VALUES
section of the macOS man page, which didn't mention errno
, but I failed to read the ERRORS
section. In any case, I've updated this PR to use musl's implementation and removed the stub.
Honestly, I assumed it wouldn't, but I just tried adding it to |
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
realpath
stubLIBC_TOP_HALF_MUSL_SOURCES
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.
Oh nice! If that works lgtm
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.
It's cool that this works.
In #463, I added stubs for
statvfs
,chmod
, etc. and removed theifdef
around therealpath
declaration, but neglected to add arealpath
implementation. Now we use musl's implementation.