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

Compile ittapi-rs' C code on Windows #38

Closed
bnjbvr opened this issue Jan 17, 2022 · 7 comments
Closed

Compile ittapi-rs' C code on Windows #38

bnjbvr opened this issue Jan 17, 2022 · 7 comments

Comments

@bnjbvr
Copy link
Contributor

bnjbvr commented Jan 17, 2022

I've noticed that the ittapi Rust wrappers don't build anything during execution of the build script on Windows: https://github.com/intel/ittapi/blob/master/ittapi-rs/build.rs#L8-L9

When trying to remove the OS-specific guard (that is, have build.rs compile on Windows with the same code that's for Linux), I get a compiler error with MSVC's 2022 compiler. Unfortunately, the error message itself seems to be lost, so I don't have any idea of what goes wrong under the hood.

I have no actual proof of this, but I actually suspect that the bindings may have to be different for Windows: could there be ABI differences, e.g. field alignment / ordering?

cc @jlb6740 @abrown

@bnjbvr bnjbvr changed the title Build ittapi-rs on Windows Compile ittapi-rs' C code on Windows Jan 17, 2022
@bnjbvr
Copy link
Contributor Author

bnjbvr commented Jan 17, 2022

The only information I get is that it returns exit code 2, fwiw.

@bnjbvr
Copy link
Contributor Author

bnjbvr commented Jan 17, 2022

Also credits to @Jake-Shadle for noticing that the OS cfg! might be busted in cross-compilation environments: we should try to use https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-build-scripts instead.

@bnjbvr
Copy link
Contributor Author

bnjbvr commented Jan 18, 2022

I think I understand a bit more what's going on, after using cargo build -vv the error's details are printed out to stderr, and it complains about the C file not being found. That's because on Windows, the symbolic links don't seem to work, so the src and include paths can't be read. On the other hand, if we use just relative paths to ../src and ../include, that compiles fine on both Windows and Linux.

However, with that relative path, we can't package the crate anymore, because it references files that are external to the source directory. With the symbolic links solution, cargo package seems to copy the content of the symlinked files, which seems sufficient. So that means that the current solution would work if we only packaged from a Linux install, and then the packaged crate would compile Just Fine on Linux & Windows.

It's not ideal though, because it makes local development and testing of ittapi-rs on Windows painful. Wonder if there would be other ways that work on all platforms, and allow testing/publishing from every platform.

@bnjbvr
Copy link
Contributor Author

bnjbvr commented Jan 19, 2022

A small reorganization of the code could maybe work: the interesting C source files would be moved to the Rust directory, and the C code itself would contain symlinks to those C files. Will try that.

@abrown
Copy link
Contributor

abrown commented Feb 9, 2022

@bnjbvr, with #41 merged and #44 in the queue, this issue should be on the way to being closed. I think the remaining part may be to (1) do some more testing in a Windows environment (I have done this but I would like a double-check if possible), and (2) publish a new version of the crate--perhaps v0.2.0? (cc: @jlb6740)

@bnjbvr
Copy link
Contributor Author

bnjbvr commented Feb 11, 2022

Could confirm it worked with the canonical doc example in wasmtime on Windows 👍

@abrown
Copy link
Contributor

abrown commented Feb 17, 2022

Closing, seee latest 0.2.0 release of the Rust crate.

@abrown abrown closed this as completed Feb 17, 2022
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 a pull request may close this issue.

2 participants