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

fix(rust): use unix/wasi instead of not windows for fd #3304

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

liamrosenfeld
Copy link
Contributor

@liamrosenfeld liamrosenfeld commented Apr 17, 2024

not(windows) includes non-wasi web assembly which causes issues when using the rust bindings from web non-wasi assembly.

In lib.rs, AsRawFd does not exist for wasm so it was causing the build to fail.

I also changed ffi.rs _ts_dup for consistency even though that does not cause a build issue.

This does lead to the situation where if you are not on either of those platforms, print_dot_graphs has no parameter to take a file. While odd, I think that does successfully convey that the current platform is not supported to use that function.

@clason
Copy link
Contributor

clason commented Apr 17, 2024

What about macos/darwin?

@liamrosenfeld
Copy link
Contributor Author

liamrosenfeld commented Apr 17, 2024

Darwin is included under Unix for Rust. What I did just realize might cause an issue though is this is essentially undoing #3293 which was a fix for WASI. I wonder if there's a way to have it be for either Unix or WASI but exclude platforms such as wasm32-unknown-unknown? Maybe target_family="unix" combined with target_os="wasi"?

@amaanq
Copy link
Member

amaanq commented Apr 17, 2024

What I did just realize might cause an issue though is this is essentially undoing #3293

Yeah I was going to bring that up, but I didn't realize your case was for wasm32-unknown-unknown. You could definitely combine the cfgs for unix + wasi to exclude wasm32 then

@maxbrunsfeld
Copy link
Contributor

maxbrunsfeld commented Apr 17, 2024

Yeah, the Rust standard library uses this attribute for the AsRawFd trait:

#[cfg(any(unix, target_os = "wasi"))]

https://doc.rust-lang.org/std/os/fd/trait.AsRawFd.html

@liamrosenfeld
Copy link
Contributor Author

Just pushed with that change

@amaanq
Copy link
Member

amaanq commented Apr 17, 2024

run cargo fmt --all @liamrosenfeld

not(windows) includes non-wasi web assembly which causes issues when using the rust bindings from non-wasi web assembly.

In lib.rs, `AsRawFd` does not exist for wasm so it was causing the build to fail.

I also changed ffi.rs `_ts_dup` for consistency even though that does not cause a build issue.

This does lead to the situation where if you are not on either of those platforms, print_dot_graphs has no parameter to take a file. While odd, I think that does successfully convey that the current platform is not supported to use that function.
@liamrosenfeld
Copy link
Contributor Author

Pushed with formatting fixes

@liamrosenfeld liamrosenfeld changed the title Use cfg(unix) instead of cfg(not(windows)) for fd in Rust Bindings fix(rust): use unix/wasi instead of not windows for fd Apr 17, 2024
@amaanq amaanq merged commit 0f125e2 into tree-sitter:master Apr 23, 2024
12 checks passed
@amaanq
Copy link
Member

amaanq commented Apr 23, 2024

thanks!

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.

None yet

4 participants