Skip to content
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

Merged
merged 2 commits into from
Feb 13, 2024

Conversation

dicej
Copy link
Contributor

@dicej dicej commented Feb 13, 2024

In #463, I added stubs for statvfs, chmod, etc. and removed the ifdef around the realpath declaration, but neglected to add a realpath implementation. Now we use musl's implementation.

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>
@sbc100
Copy link
Member

sbc100 commented Feb 13, 2024

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?

@dicej
Copy link
Contributor Author

dicej commented Feb 13, 2024

@sbc100 for context: WebAssembly/wasi-sdk#373 (comment)

@sbc100
Copy link
Member

sbc100 commented Feb 13, 2024

@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.

@sunfishcode
Copy link
Member

I think a realpath stub makes sense.

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 realpath specifically: I've also changed my stance, to supporting the idea that wasi-filesystem switch to being rooted. Assuming we do that, realpath would become something we could fully support. That makes it feel better to have a stub, because we're not saying "this is fundamentally unsupportable, do something else", but "this isn't supported yet".

@kateinoigakukun
Copy link
Contributor

I thought we can just use libc-top-half/musl/src/misc/realpath.c as userland implementation since we already have readlink with cwd emulation, doesn't it work?

// 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;
Copy link
Member

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.

Copy link
Contributor Author

@dicej dicej Feb 13, 2024

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.

@dicej
Copy link
Contributor Author

dicej commented Feb 13, 2024

I thought we can just use libc-top-half/musl/src/misc/realpath.c as userland implementation since we already have readlink with cwd emulation, doesn't it work?

Honestly, I assumed it wouldn't, but I just tried adding it to LIBC_TOP_HALF_MUSL_SOURCES and everything built fine, so maybe that's the way to go.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
@dicej dicej changed the title provide a realpath stub add realpath.c to LIBC_TOP_HALF_MUSL_SOURCES Feb 13, 2024
Copy link
Member

@sbc100 sbc100 left a 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

Copy link
Member

@sunfishcode sunfishcode left a 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.

@sunfishcode sunfishcode merged commit 212296e into WebAssembly:main Feb 13, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants