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

We need fs::realpath #11857

Closed
lilyball opened this issue Jan 27, 2014 · 16 comments
Closed

We need fs::realpath #11857

lilyball opened this issue Jan 27, 2014 · 16 comments

Comments

@lilyball
Copy link
Contributor

After an aborted attempt at implementing this directly (#11734), we need to investigate how to provide fs::realpath() correctly.

The current thinking is that for libgreen, we should defer to libuv. For libnative, on POSIX, we should use realpath(). On Windows, I don't know, but we should avoid writing this ourselves at all costs because there's a lot of complexity involved.

@bnoordhuis
Copy link
Contributor

The current thinking is that for libgreen, we should defer to libuv.

Minor correction: libuv doesn't have a realpath() implementation. The fs.realpath() and fs.realpathSync() functions in node.js are implemented in JavaScript.

@lilyball
Copy link
Contributor Author

@bnoordhuis That's unfortunate. Given that realpath() includes a number of filesystem calls (e.g. stat, readlink), does this mean that we do need to reimplement it ourselves in order to use libuv's implementation of those filesystem calls, or is calling the "real" realpath() still acceptable even when using libgreen?

@bnoordhuis
Copy link
Contributor

realpath() can block so I wouldn't call it from a green thread. You can use uv_queue_work() to offload the call to a thread from the thread pool. It would look something like this in C:

typedef struct {
  uv_work_t work_req;
  int errorno;
  char resolved_path[PATH_MAX];
  char path[1];
} realpath_req;

static void work_cb(uv_work_t *req);
static void done_cb(uv_work_t *req, int status);

void fs_realpath(const char *path) {
  size_t len = strlen(path);
  realpath_req *req = malloc(sizeof(*req) + len);
  memcpy(req->path, path, len + 1);
  uv_queue_work(uv_default_loop(), &req->work_req, work_cb, done_cb);
}

static void work_cb(uv_work_t *work_req) {
  realpath_req *req = container_of(work_req, realpath_req, work_req);
  req->errno = 0;
  if (realpath(req->path, req->resolved_path) == NULL)
    req->errorno = errno;
}

static void done_cb(uv_work_t *req, int status) {
  realpath_req *req = container_of(work_req, realpath_req, work_req);
  (void) &status;  // Only relevant when calling uv_cancel().
  // ...
  free(req);
}

@lilyball
Copy link
Contributor Author

cc @alexcrichton

alexcrichton referenced this issue in alexcrichton/rust Feb 23, 2014
This commit implements a layman's version of realpath() for metadata::loader to
use in order to not error on symlinks pointing to the same file.

Closes rust-lang#12459
alexcrichton added a commit to alexcrichton/rust that referenced this issue Apr 8, 2014
When calculating the sysroot, it's more accurate to use realpath() rather than
just one readlink() to account for any intermediate symlinks that the rustc
binary resolves itself to.

For rpath, realpath() is necessary because the rpath must dictate a relative
rpath from the destination back to the originally linked library, which works
more robustly if there are no symlinks involved.

Concretely, any binary generated on OSX into $TMPDIR requires an absolute rpath
because the temporary directory is behind a symlink with one layer of
indirection. This symlink causes all relative rpaths to fail to resolve.

cc rust-lang#11734
cc rust-lang#11857
alexcrichton added a commit to alexcrichton/rust that referenced this issue Apr 10, 2014
When calculating the sysroot, it's more accurate to use realpath() rather than
just one readlink() to account for any intermediate symlinks that the rustc
binary resolves itself to.

For rpath, realpath() is necessary because the rpath must dictate a relative
rpath from the destination back to the originally linked library, which works
more robustly if there are no symlinks involved.

Concretely, any binary generated on OSX into $TMPDIR requires an absolute rpath
because the temporary directory is behind a symlink with one layer of
indirection. This symlink causes all relative rpaths to fail to resolve.

cc rust-lang#11734
cc rust-lang#11857
@wycats
Copy link
Contributor

wycats commented Jun 9, 2014

The current state is not acceptable.

For Cargo, we had to pull in the realpath that is used by rustc: rust-lang/cargo@4c3f9da#diff-42830b961b225436d4e616dcc6832330R153

@wycats
Copy link
Contributor

wycats commented Jun 9, 2014

It doesn't make sense to me to have an implementation that we use internally in the compiler but to be unwilling to ship it even temporarily as part of std. If it's broken, that will mean the compiler is broken!

@lilyball
Copy link
Contributor Author

lilyball commented Jun 9, 2014

@wycats

That realpath() implementation looks broken to me. Notably, the MAX_LINKS_FOLLOWED really seems to limit the number of path components that can be links, but when resolving a link, it recursively runs realpath() on the link destination, despite that getting a fresh "links followed" counter.

It also isn't appending the resolved symlink path to the directory the symlink was in before calling the recursive realpath(), which is quite wrong. A relative symlink will then be considered relative to the cwd instead, which is of course not correct.

I would suggest that as a temporary workaround you use FFI to call POSIX realpath() instead (not sure about Windows, maybe just skip it there? The realpath() impl you linked to already short-circuits Windows).

@wycats
Copy link
Contributor

wycats commented Jun 10, 2014

Windows supports symlinks as of Vista, so that seems like a problem. I would suggest that getting this working correctly in the compiler is important.

@wycats
Copy link
Contributor

wycats commented Jun 10, 2014

In other words, either this is horribly broken in the compiler and needs to be fixed ASAP, or it's working well enough to include in the standard library for now.

@lilyball
Copy link
Contributor Author

There's a grey area in between those two extremes. It's definitely broken enough that it's not appropriate to put in the standard library. But it apparently works well enough for the limited use-case of the compiler, or this would have been an issue before now. Which is to say, it should be fixed, but it's not an ASAP kind of fix.

@wycats
Copy link
Contributor

wycats commented Jun 10, 2014

The "limited use case of the compiler" seems to be every symlink ever.

@lilyball
Copy link
Contributor Author

Hrm, looks like I actually misread the code. The assertion that it "isn't appending the resolved symlink path to the directory the symlink was in" is wrong. But the issue with MAX_LINKS_FOLLOWED is still legitimate.

@mahkoh
Copy link
Contributor

mahkoh commented Sep 11, 2014

With libgreen gone this should be easy.

#[cfg(unix)]
pub fn real_path(p: Path) -> Path {
    use libc::{c_char};
    use std::c_str::{CString};
    extern {
        fn realpath(path: *const c_char, resolved: *mut c_char) -> *const c_char;
    }
    let mut p = p.into_vec();
    p.push(0);
    let new_p = unsafe { realpath(p.as_ptr() as *const c_char, 0 as *mut c_char) };
    unsafe { Path::new(CString::new(new_p, true).as_bytes_no_nul()) }
}

#[cfg(windows)]
pub fn real_path(p: Path) -> Path {
    // TODO
    p
}

@aturon
Copy link
Member

aturon commented Jan 29, 2015

Note for posterity: look at GetFullPathNameW on Windows for drive-relative CWD.

@alexcrichton
Copy link
Member

Closing in favor of rust-lang/rfcs#939

@kornelski
Copy link
Contributor

For those landing from Google here:

realpath has been added as fs::canonicalize

bors added a commit to rust-lang-ci/rust that referenced this issue Dec 1, 2023
…flip1995

Add documentation update hint using `cargo collect-metadata`

This adds a little reminder to update the documentation in the book using `cargo collect-metadata` after changing the lint configuration since this can easily be missed (been there done that 🙈).

> Yeah a note would be good, would be good for us to see if we can make it automatic also

_Originally posted by `@Alexendoo` in rust-lang/rust-clippy#11757 (comment)

Regarding the automation Im not sure whats the best option here. I thought about a kind of a "semi-automated" way, e.g. a `cargo dev` command which runs the "is the documentation updated" check (and maybe other useful checks) locally and reminds the user of updating the documentation so this gets caught before pushing.

changelog: none
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

No branches or pull requests

7 participants