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

Compiling Rust bindings #14884

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

hgaiser
Copy link

@hgaiser hgaiser commented Mar 2, 2023

Description

The current value of VERSION_NUMBER is 1.15.0, but this isn't released yet. The Rust bindings use this version to decide what compiled package to download. Seeing as 1.15.0 isn't yet released, this fails to download. Downloading 1.14.0 works, but there's no way to override this behavior from other crates. Adding an option to set the version number using an environment variable gives more freedom in choosing the onnxruntime version.

In addition I experienced that including the version number also added a newline character at the end, which makes the path it generates invalid. To fix this I trimmed the version number, to remove any whitespaces (including newlines).

Lastly, the generated include dir didn't exist for me, so I corrected it to point to the include dir that did exist. I'm not sure what's going wrong here?

Motivation and Context

I wasn't able to use the Rust bindings in my crate. With these fixes I can use it.

Pinging @boydjohnson since I believe he was the main person working on moving the binding into this repository.

@hgaiser
Copy link
Author

hgaiser commented Mar 2, 2023

@microsoft-github-policy-service agree company="RoboHouse"

@boydjohnson
Copy link
Contributor

@hgaiser Thanks for this contribution. So the built packages that I have looked at for 1.14.0 and 1.14.1 have include/onnxruntime_c_api.h instead of include/core/session/onnxruntime_c_api.h which they used to have. So removing them was the right fix for the download option. It isn't the right thing for the compile option, because then the path is include/core/session/.... Can you modify prepare_libort_dir_compiled to add the paths removed from generate_bindings?

@hgaiser hgaiser changed the title Robohouse Compiling Rust bindings Mar 9, 2023
@hgaiser
Copy link
Author

hgaiser commented Mar 9, 2023

@hgaiser Thanks for this contribution. So the built packages that I have looked at for 1.14.0 and 1.14.1 have include/onnxruntime_c_api.h instead of include/core/session/onnxruntime_c_api.h which they used to have. So removing them was the right fix for the download option. It isn't the right thing for the compile option, because then the path is include/core/session/.... Can you modify prepare_libort_dir_compiled to add the paths removed from generate_bindings?

I don't think it's that simple, right? prepare_libort_dir_compiled isn't adding any paths currently, nor does it have access to the bindgen builder. Even if it did, this still wouldn't be valid.

Any idea why the compiled version has these paths, but the downloaded one does not?

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