Set up ripgrep for compilation on non-unix, non-windows platforms #2787

holzschu · 2024-04-23T09:10:17Z

The code of ripgrep compiles on almost any kind of architecture, including WebAssembly, except in two tiny places related to hyperlinks:

when it searches for the local hostname in hostname.rs,
when it does path canonalization in hyperlink.rs.

In both cases, there is a path with #[cfg(unix)] and a path with #[cfg(windows)]. WebAssembly being neither unix nor windows, compilation fails. This PR adds a third path:

for localhost, it returns "localhost" instead of calling gethostname(), for all environments that are neither unix nor windows.
for path canonalization, it uses std::os::wasi::ffi::OsStrExt instead of std::os::unix::ffi::OsStrExt, which is not available; this part is specific for wasm32-wasi. It could be possible to deactivate path canonalization entirely for architectures that are neither unix nor windows, but I aimed for the smallest working change.

With these two changes, it's possible to compile ripgrep with cargo build --target wasm32-wasi (and, more importantly, it works in wasm32-wasi environments).

ltrzesniewski · 2024-04-23T09:39:39Z

It could be possible to deactivate path canonalization entirely for architectures that are neither unix nor windows

std::path::absolute should be stabilized soon, and I suppose it will replace canonicalization in ripgrep, which will both simplify and optimize this part of the code.

blyxxyz · 2024-04-23T10:28:13Z

crates/printer/src/hyperlink.rs

@@ -853,6 +853,57 @@ impl HyperlinkPath {
        }
        HyperlinkPath(result)
    }
+    #[cfg(target_os = "wasi")]
+    pub(crate) fn from_path(original_path: &Path) -> Option<HyperlinkPath> {


The WASI spec seems to ban absolute paths and the stdlib doesn't implement canonicalize. I don't know how black-and-white this is (especially in the long term) but right now this might as well be stubbed out with None.

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.

I'm curious how std::path::absolute will work here. It seems to depend on what std::env::current_dir will return, and that does seem to have an implementation on WASI: https://github.com/rust-lang/rust/blob/eff958c59e8c07ba0515e164b825c9001b242294/library/std/src/sys/pal/wasi/os.rs#L71-L95

But I agree, for now, this should just always return None and emit a DEBUG log that hyperlinks aren't supported on WASI.

AFAICT it works like this:

WASI syscalls take a directory descriptor and a path relative to that directory descriptor.

The WASI runtime gives the WASM binary a list of directory descriptors plus their actual original paths (preopens). So the runtime grants selective access. It could easily pretend the filesystem is smaller than it really is, chroot-style, but it doesn't have to: it can grant a /home/user descriptor with the /home/user path.

wasi-libc takes absolute paths and resolves them to a directory descriptor + relative path based on the longest matching prefix from the preopens, if any. (This is inspired by libpreopen.)

wasi-libc also emulates a working directory in WASM-space.

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

Interesting. So std::env::current_dir() works, and thus std::path::absolute might in turn work here as well.

BurntSushi

I think the idea here seems good, but as I mentioned in the comments, it seems like hyperlinks and WASI are fundamentally incompatible.

Also, can we add CI support WASI so that we don't regress here? You should be able to lift this nearly directly: https://github.com/BurntSushi/memchr/blob/20ef11fa92d8b393735f10906c436a8ce6e792a4/.github/workflows/ci.yml#L133-L163

If tests don't work, that's okay. But at least having a cargo build --verbose in CI would be nice.

BurntSushi · 2024-04-23T12:05:09Z

crates/printer/src/hyperlink.rs

@@ -853,6 +853,57 @@ impl HyperlinkPath {
        }
        HyperlinkPath(result)
    }
+    #[cfg(target_os = "wasi")]
+    pub(crate) fn from_path(original_path: &Path) -> Option<HyperlinkPath> {


I'm curious how std::path::absolute will work here. It seems to depend on what std::env::current_dir will return, and that does seem to have an implementation on WASI: https://github.com/rust-lang/rust/blob/eff958c59e8c07ba0515e164b825c9001b242294/library/std/src/sys/pal/wasi/os.rs#L71-L95

But I agree, for now, this should just always return None and emit a DEBUG log that hyperlinks aren't supported on WASI.

BurntSushi · 2024-04-23T12:06:19Z

crates/cli/src/hostname.rs

-            "hostname could not be found on unsupported platform",
-        )
+        // backup solution for systems that do not have gethostname():
+        Ok(OsString::from("localhost"))


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.

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:

Compiling grep-cli v0.1.10 (/Users/holzschu/src/Xcode_iPad/ripgrep_clone/crates/cli) error[E0308]: mismatched types --> crates/cli/src/hostname.rs:28:9 | 16 | pub fn hostname() -> io::Result<OsString> { | -------------------- expected `Result<OsString, std::io::Error>` because of return type ... 28 | / io::Error::new( 29 | | io::ErrorKind::Other, 30 | | "hostname could not be found on unsupported platform", 31 | | ) | |_________^ expected `Result<OsString, Error>`, found `Error` | = note: expected enum `Result<OsString, std::io::Error>` found struct `std::io::Error` help: try wrapping the expression in `Err` | 28 ~ Err(io::Error::new( 29 | io::ErrorKind::Other, 30 | "hostname could not be found on unsupported platform", 31 ~ )) | For more information about this error, try `rustc --explain E0308`.

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.

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.

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")] in hyperlink.rs. But what should from_path() return in that case?

With a DEBUG log message. Mentioned here: #2787 (comment)

holzschu · 2024-04-23T16:31:29Z

I've changed the files and the title of the PR.
Now, the goal is for ripgrep to compile without error on platforms that are neither unix nor windows (that includes WebAssembly, but there might be other platforms as well.

It will emit an error in hostname.rs and a debug message in hyperlinks.rs.

BurntSushi · 2024-04-23T16:34:30Z

.github/workflows/ci.yml

+    - name: Add wasm32-wasi target
+      run: rustup target add wasm32-wasi
+    - name: Basic build
+      run: cargo build --verbose


Did running tests via wasmtime fail? If so this is fine, but if they worked, then we should just include them here.

Indeed, running tests via wasmtime does fail (but compilation works):

error[E0433]: failed to resolve: could not find `unix` in `os` --> tests/util.rs:197:22 | 197 | use std::os::unix::fs::symlink; | ^^^^ could not find `unix` in `os` |

Ah fair enough. We can save that for another day.

BurntSushi · 2024-04-23T16:35:13Z

crates/printer/src/hyperlink.rs

+    // For other platforms (not windows, not unix), return None and log a debug message.
+    #[cfg(not(any(windows, unix)))]
+    pub(crate) fn from_path(original_path: &Path) -> Option<HyperlinkPath> {
+        log::debug!("Hyperlinks are not supported on this platform");


To be consistent with other log messages, could you please use "hyperlinks" instead of "Hyperlinks"?

This is done.

BurntSushi · 2024-04-23T16:35:32Z

crates/printer/src/hyperlink.rs

@@ -853,6 +853,13 @@ impl HyperlinkPath {
        }
        HyperlinkPath(result)
    }
+
+    // For other platforms (not windows, not unix), return None and log a debug message.


This should be using ///.

And please move this to be directly below the other from_path constructors.

These changes have been incorporated.

BurntSushi

Thanks!

holzschu · 2024-04-23T17:11:35Z

Thank you for your patience and pedagogy!

Set up ripgrep for compilation on WebAssembly (wasm32-wasi)

2954e42

blyxxyz reviewed Apr 23, 2024

View reviewed changes

BurntSushi requested changes Apr 23, 2024

View reviewed changes

N-Holzschuch added 3 commits April 23, 2024 16:22

Adding CI for wasm

c57995e

Return an error on non-unix, non-windows platforms

9a3b871

Setup the #[cfg(not(any(windows, unix)))] case for hyperlinks

c7ceb29

holzschu changed the title ~~Set up ripgrep for compilation on WebAssembly (wasm32-wasi)~~ Set up ripgrep for compilation on non-unix, non-windows platforms Apr 23, 2024

BurntSushi requested changes Apr 23, 2024

View reviewed changes

N-Holzschuch added 3 commits April 23, 2024 18:42

Moved from_path so it is directly below the other from_path constructors

64be256

Added tests of Wasm version with WASMTIME

37a5fd5

Disabled tests via wasmtime in CI

5cb3883

BurntSushi approved these changes Apr 23, 2024

View reviewed changes

BurntSushi merged commit bb8601b into BurntSushi:master Apr 23, 2024
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set up ripgrep for compilation on non-unix, non-windows platforms #2787

Set up ripgrep for compilation on non-unix, non-windows platforms #2787

holzschu commented Apr 23, 2024

ltrzesniewski commented Apr 23, 2024

blyxxyz Apr 23, 2024

BurntSushi Apr 23, 2024

blyxxyz Apr 23, 2024

BurntSushi Apr 23, 2024

BurntSushi left a comment

BurntSushi Apr 23, 2024

BurntSushi Apr 23, 2024

holzschu Apr 23, 2024

BurntSushi Apr 23, 2024

holzschu Apr 23, 2024

BurntSushi Apr 23, 2024

BurntSushi Apr 23, 2024

holzschu commented Apr 23, 2024

BurntSushi Apr 23, 2024

holzschu Apr 23, 2024

BurntSushi Apr 23, 2024

BurntSushi Apr 23, 2024

holzschu Apr 23, 2024

BurntSushi Apr 23, 2024

BurntSushi Apr 23, 2024

holzschu Apr 23, 2024

BurntSushi left a comment

holzschu commented Apr 23, 2024

Set up ripgrep for compilation on non-unix, non-windows platforms #2787

Set up ripgrep for compilation on non-unix, non-windows platforms #2787

Conversation

holzschu commented Apr 23, 2024

ltrzesniewski commented Apr 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BurntSushi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holzschu commented Apr 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BurntSushi left a comment

Choose a reason for hiding this comment

holzschu commented Apr 23, 2024