-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Set up ripgrep for compilation on non-unix, non-windows platforms #2787
Changes from 1 commit
2954e42
c57995e
9a3b871
c7ceb29
64be256
37a5fd5
5cb3883
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -853,6 +853,57 @@ impl HyperlinkPath { | |
} | ||
HyperlinkPath(result) | ||
} | ||
#[cfg(target_os = "wasi")] | ||
pub(crate) fn from_path(original_path: &Path) -> Option<HyperlinkPath> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The WASI spec seems to ban absolute paths and the stdlib doesn't implement When running on top of Windows I imagine you'd want to return a Windows path here, even though WASI is more Unix-like internally. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious how But I agree, for now, this should just always return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAICT it works like this:
So it's not as bad as I thought. Thanks to the emulation absolute paths should for the most part just work as long as access to that part of the filesystem is granted, particularly on Unix. (Except for missing syscalls I guess.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting. So |
||
use std::os::wasi::ffi::OsStrExt; | ||
|
||
// We canonicalize the path in order to get an absolute version of it | ||
// without any `.` or `..` or superfluous separators. Unfortunately, | ||
// this does also remove symlinks, and in theory, it would be nice to | ||
// retain them. Perhaps even simpler, we could just join the current | ||
// working directory with the path and be done with it. There was | ||
// some discussion about this on PR#2483, and there generally appears | ||
// to be some uncertainty about the extent to which hyperlinks with | ||
// things like `..` in them actually work. So for now, we do the safest | ||
// thing possible even though I think it can result in worse user | ||
// experience. (Because it means the path you click on and the actual | ||
// path that gets followed are different, even though they ostensibly | ||
// refer to the same file.) | ||
// | ||
// There's also the potential issue that path canonicalization is | ||
// expensive since it can touch the file system. That is probably | ||
// less of an issue since hyperlinks are only created when they're | ||
// supported, i.e., when writing to a tty. | ||
// | ||
// [1]: https://github.com/BurntSushi/ripgrep/pull/2483 | ||
let path = match original_path.canonicalize() { | ||
Ok(path) => path, | ||
Err(err) => { | ||
log::debug!( | ||
"hyperlink creation for {:?} failed, error occurred \ | ||
during path canonicalization: {}", | ||
original_path, | ||
err, | ||
); | ||
return None; | ||
} | ||
}; | ||
let bytes = path.as_os_str().as_bytes(); | ||
// This should not be possible since one imagines that canonicalization | ||
// should always return an absolute path. But it doesn't actually | ||
// appear guaranteed by POSIX, so we check whether it's true or not and | ||
// refuse to create a hyperlink from a relative path if it isn't. | ||
if !bytes.starts_with(b"/") { | ||
log::debug!( | ||
"hyperlink creation for {:?} failed, canonicalization \ | ||
returned {:?}, which does not start with a slash", | ||
original_path, | ||
path, | ||
); | ||
return None; | ||
} | ||
Some(HyperlinkPath::encode(bytes)) | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
|
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'm not a huge fan of giving wrong answers like this. And it seems like WASI can't really support hyperlinks anyway, due to the ban on absolute paths. So I think this change should probably be reverted.
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 problem I was trying to fix here, and that's also a question I have about setting up CI, is that the current version of the code causes a compilation error:
So if the goal of setting up CI is to get the same result after the PR as before, that's not going to happen.
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 I see. I think you just want
Err(io::Error::new(...))
, as suggested in the error message you're showing. This particular code path is probably entirely untested in CI at present, and thus the fact that it is wrong now and has always been wrong was never noticed.My main objection was to silently using a potentially incorrect value. I'm open to doing that in the future to get hyperlinks working in WASI after we've considered the possible alternatives. But since
std::fs::canonicalize
isn't going to work on WASI right now anyway, we should just avoid changing the semantic intent of this code.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've done the changes for
hostname.rs
. But then, for symmetry, I think I should have a branch#[cfg(not(any(windows, unix)))]
instead of#[cfg(target_os = "wasi")]
inhyperlink.rs
. But what shouldfrom_path()
return in that case?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.
None
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.
With a DEBUG log message. Mentioned here: #2787 (comment)