Expose correct symlink API on WASI

[RReverser]

As described in #68574, the currently exposed API for symlinks is, in fact, a thin wrapper around the corresponding syscall, and not suitable for public usage.

The reason is that the 2nd param in the call is expected to be a handle of a "preopened directory" (a WASI concept for exposing dirs), and the only way to retrieve such handle right now is by tinkering with a private __wasilibc_find_relpath API, which is an implementation detail and definitely not something we want users to call directly.

Making matters worse, the semantics of this param aren't obvious from its name (fd), and easy to misinterpret, resulting in people trying to pass a handle of the target file itself (as in vitiral/path_abs#50), which doesn't work as expected.

I did a codesearch among open-source repos, and the usage above is so far the only usage of this API at all, but we should fix it before more people start using it incorrectly.

While this is technically a breaking API change, I believe it's a justified one, as 1) it's OS-specific and 2) there was strictly no way to correctly use the previous form of the API, and if someone does use it, they're likely doing it wrong like in the example above.

The new API does not lead to the same confusion, as it mirrors std::os::unix::fs::symlink and std::os::windows::fs::symlink_{file,dir} variants by accepting source/target paths.

Fixes #68574.

r? @alexcrichton

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
skip untracked path cpu-usage.csv during rustfmt invocations
skip untracked path src/doc/book/ during rustfmt invocations
skip untracked path src/doc/rust-by-example/ during rustfmt invocations
skip untracked path src/llvm-project/ during rustfmt invocations
Diff in /checkout/library/std/src/sys/wasi/ext/fs.rs at line 507:
 /// This is similar to [`std::os::unix::fs::symlink`] and
 /// [`std::os::windows::fs::symlink_file`] and [`symlink_dir`](std::os::windows::fs::symlink_dir)
 /// counterparts.
-pub fn symlink<P: AsRef<Path>, U: AsRef<Path>>(
-    old_path: P,
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/checkout/library/std/src/sys/wasi/ext/fs.rs"` failed.
-    new_path: U,
-) -> io::Result<()> {
+pub fn symlink<P: AsRef<Path>, U: AsRef<Path>>(old_path: P, new_path: U) -> io::Result<()> {
     crate::sys::fs::symlink(old_path.as_ref(), new_path.as_ref())
 
 
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.
Build completed unsuccessfully in 0:00:18

[alexcrichton]

My personal thoughts on this issue haven't really changed since last I wrote which is that I think the best option for improving the state of things is to expose a function which, given a path, returns a file descriptor and a relative path. I'm not sure why the symlink API would be singled-out for changes here when there are many others of similar shape and size.

If the current signature is being actively misused in the wild then we should likely improve the documentation, add examples, or maybe even tweak the parameter order in the signature. The purpose of this module, however, is to be very close to the system layer, and what we have at the system layer I believe is this API.

[RReverser]

My personal thoughts on this issue haven't really changed since last I wrote

Ah, maybe I misunderstood your comments there, in particular:

I'm basically just saying that I would not personally pursue adding this method to the standard library. If you'd like to do so you can certainly do that, however.

seemed to suggest that it would be okay to expose if I make the PR?

---

I'm not sure why the symlink API would be singled-out for changes here when there are many others of similar shape and size.

I feel like it's singled-out right now instead, compared to all other filesystem methods on one side, and all other OS methods for symlinks on another side.

If the goal of the module is to be as close to raw system bindings, I can understand that and yes, this change would make it different.

However, it also makes absence of a single std::fs::soft_link even more unfortunate, because then there is no place in the stdlib for a high-level symlink API - neither in a shared namespace nor in OS-specific modules?

I do agree that the idea to "expose a function which, given a path, returns a file descriptor and a relative path" is good and useful too, it just still seems like the APIs as they stand right now are too easy to misunderstand / to misuse for people not familiar with WASI preopen system, which is probably most generic library authors who just want to create a symlink in a cross-platform fashion.

Hence, I'd still expose a symlink helper at least under a different name, regardless of the low-level API for splitting paths up.

[alexcrichton]

What I meant by that is you can always make a PR like this one, but you specifically tagged me for review here and my opinion hasn't changed about this in the meantime. I'm not the only reviewer here, of course though.

I think a symlink function under a different name seems reasonable, but the default name I'd leave for the raw WASI operations since that's the purpose of std::os::wasi, which is for WASI-specific things. Its goal is not to make cross-platform development easier, but if that's always of course a nice-to-have if possible.

[RReverser]

but you specifically tagged me for review here and my opinion hasn't changed about this in the meantime

Ah, I see, makes sense. I misinterpreted the original phrase as "I don't want to work on this myself, but open to some adding such methods".

Its goal is not to make cross-platform development easier, but if that's always of course a nice-to-have if possible.

That makes sense. I thought that users who want raw WASI access, would be encouraged to use the wasi crate directly instead - which provides raw access and which stdlib uses under the hood - while std::os::wasi would be a bit higher-level collection of OS-specific utils. But I can appreciate it's already not the case for many other utils.

I'm definitely open to renaming the symlink function above, while keeping original intact. I guess I'll wait for more reviews and suggestions on better naming or namespace meanwhile.

[alexcrichton]

I think there's definitely a point in the design space for libstd to not really expose much of WASI, especially as WASI grows with more and more features. In that sense it might be the best design for libstd to only expose the conveniences as you say, and otherwise recommend the wasi crate. If we were to do that, however, we'd probably want to delete most of the rest of this extension module instead of only changing one function.

[RReverser]

If we were to do that, however, we'd probably want to delete most of the rest of this extension module instead of only changing one function.

Yeah, if they didn't already exist, I'd be definitely leaning towards this option personally. I'm guessing now it would be too much of a breaking change?

Although maybe not really, given that most of other functions also rely on "preopen dir fd", which hasn't been exposed yet.

[alexcrichton]

AFAIK everything here is unstable, which means we can change it at any time.

[RReverser]

SGTM. What should we do about this PR meanwhile - should I rename the function, wait for more reviewers or abandon in favour of "rewrite all functions to be helpers" style? (I'm assuming the latter needs to be discussed with a wider audience... somewhere though)

[alexcrichton]

That's up to you, I don't personally have a preference of how best to go about this.

[RReverser]

Ok I went ahead and restored the old symlink API under its name, and made the new one under symlink_path.

I think this is most reasonable approach for now, particularly because the other two APIs - link and rename - can already be used with bare paths via std::fs::hard_link and std::fs::rename correspondingly, but there is no higher-level API for symlinks on WASI.

This change is the least invasive way to add such API and can be merged regardless of what we decide to do with syscall-like methods in the future IMO.

[alexcrichton]

@bors: r+

[bors]

📌 Commit f4b1bef has been approved by alexcrichton

[RReverser]

@alexcrichton Thanks!