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

rust: modify Rust crate to support Windows #41

Merged
merged 2 commits into from
Feb 9, 2022

Conversation

abrown
Copy link
Contributor

@abrown abrown commented Feb 9, 2022

This change has several parts: first, it splits out ITT bindings by OS.
The ITT bindings have OS-specific variables (ITT_OS, ITT_PLATFORM)
that encode which OS the bindings are for. This change creates separate
binding files for each OS. The JIT bindings are also not OS-agnostic and
are split out to the same directories. This change also removes libc
constants present in the generated bindings and ignores Windows carriage
returns during testing. Now that the bindings are generated correctly
for each OS, this change builds the C libraries and links to them in the
build.rs script on Windows. As a side effect, this removes the
force_32 feature--it was not applying the -m32 flag correctly during
compilation--but the feature can be re-added if needed in the future.

This change has several parts: first, it splits out ITT bindings by OS.
The ITT bindings have OS-specific variables (`ITT_OS`, `ITT_PLATFORM`)
that encode which OS the bindings are for. This change creates separate
binding files for each OS. The JIT bindings are also not OS-agnostic and
are split out to the same directories. This change also removes libc
constants present in the generated bindings and ignores Windows carriage
returns during testing. Now that the bindings are generated correctly
for each OS, this change builds the C libraries and links to them in the
`build.rs` script on Windows. As a side effect, this removes the
`force_32` feature--it was not applying the `-m32` flag correctly during
compilation--but the feature can be re-added if needed in the future.
This change builds the Rust crate and runs its tests on all supported
OS-es: Linux, macOS, Windows.
Copy link
Contributor

@jlb6740 jlb6740 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, this is good. Windows still isn't fully supported though right because of the symbolic link issue that remains with c-library. I think we said to investigate pulling in submodules that in effect are directories within repos and if that doesn't work just copy the c library files or if that is not desirable to have a script do it. One other comment: so this crate is really an exercise in using bindgen. The test should fail when ITTAPI C-library has an update that is now inconsistent with the bindgen results. Should we have a github actions that automatically rebuilds and publishes when that occurs?

@abrown
Copy link
Contributor Author

abrown commented Feb 9, 2022

Windows still isn't fully supported though right because of the symbolic link issue that remains with c-library

Yeah, I plan to submit some follow-on PRs to explain what Windows developers should do to hack on this library directly. But Windows users of this library should be good to build ittapi as a part of their projects.

The test should fail when ITTAPI C-library has an update that is now inconsistent with the bindgen results. Should we have a github actions that automatically rebuilds and publishes when that occurs?

Yeah, the test will fail. I think our workflow should be to manually update them for now (BLESS=1 cargo test) and commit the results--what is annoying is that we may need to do this separately on both Windows and Linux because of slight differences. I don't expect the bindings to change a lot but if they do and this task fails often then we should just disable the testing part (the cargo test step).

@abrown abrown merged commit 8d47378 into intel:master Feb 9, 2022
@abrown abrown deleted the crate-for-windows branch February 9, 2022 19:26
@abrown
Copy link
Contributor Author

abrown commented Feb 11, 2022

For the record: these changes (along with #40, #42, and #44) were a team effort! They came out of discussions with @jlb6740 and @bnjbvr and work on a private repository to get this all working prior to bringing it here--thanks @bnjbvr for all of the contributions there! Moving the files was easier than moving commits (the directory structure of the private repository was different), so only my name shows on the commits, but others helped make this happen (and can answer troubleshooting questions if needed 😁).

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.

2 participants