Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix win32 build #70

Merged
merged 9 commits into from
Apr 11, 2021
Merged

Conversation

krazijames
Copy link
Contributor

@krazijames krazijames commented Mar 20, 2021

Recently, I had to use onnxruntime crate on Windows 32bit but it doesn't seem to work because there are some mismatched types & function calling conventions between onnxruntime-sys and onnxruntime. So I fixed the binding generation config to use usize instead of u64, and added macros for choosing appropriate function calling conventions according to the target architecture, and also re-enabled CI for i686.

@MainRo
Copy link

MainRo commented Apr 9, 2021

I was just considering doing the same fix to support arm 32bits target. Hope this PR will be accepted!

Copy link
Owner

@nbigaouette nbigaouette left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!!

Can you add an entry in the changelog? Also, is the build.rs needs to be adapted to download a pre-built i686 windows version?

Thanks again!

- target: x86_64-pc-windows-msvc
os: windows-latest
- target: i686-pc-windows-msvc
os: windows-latest
Copy link
Owner

Choose a reason for hiding this comment

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

Why was the target and os inverted in the matrix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't invert this, only 3 jobs for each os will be executed ;(

@nbigaouette nbigaouette merged commit 8db3091 into nbigaouette:master Apr 11, 2021
nbigaouette added a commit that referenced this pull request Apr 11, 2021
nbigaouette added a commit that referenced this pull request Apr 11, 2021
@krazijames
Copy link
Contributor Author

Sorry for late, you merged this!
It seems to be already working for downloading a pre-built i686 windows version without any changes.
Thank you!

@nbigaouette
Copy link
Owner

Yeah I had a couple of cycles today so went on and merge this. Thanks for the clarifications, and the contribution!

Version 0.0.12 is released with this.

@krazijames krazijames deleted the fix-win32-build branch April 12, 2021 02:55
tc-wolf added a commit to tc-wolf/onnxruntime-rs that referenced this pull request May 12, 2021
* Update bindgen config to use `usize`

* Update macOS bindings

* Update Linux bindings

* Update Windows (x86_64) bindings

* Update Windows (x86) bindings

* Use `usize` instead of `u64`

* Use conditional function calling conventions

* Re-enable CI for Windows 32bit

* Fix CI matrix

* Add generated bindings for Apple M1

* On macOS Big Sur, homebrew installs to /opt/homebrew instead of /usr/local, adapt installation notes

* Feature is 'generate-bindings', not 'bindgen'. Change error message

* Add more paths to include directory, required when compilling and installing from source

* Formatting

* Compilation notes: environment variable was changed to 'ORT_LIB_LOCATION' (from 'ONNXRUNTIME_INSTALL_DIR')

* Add entry to changelog

* Add macOS aarch64 to README

* Add note about LD_LIBRARY_PATH in README

* Add a VSCode config to run clippy and format on save

* Clippy: Allow upper case acronyms, names coming from specific ML models

* Fix clippy lint: Change 'Into' impls to 'From'

* Add ORT_LIB_LOCATION note in readme for 'ORT_STRATEGY=system'

* Update changelog following nbigaouette#70

* Update macos/aarch64 bindings following nbigaouette#70

* Formatting

* Upgrade 'ndarray' to 0.15 (from 0.13)

* Upgrade 'ureq' to 2.1 (from 1.5)

* Add changelog entries

* Clippy: Box the larse ureq::Error in OrtDownloadError

* Release v0.0.12

* Return an Err on shape mismatch instead of panic

This way we let the user of the library decide how to handle the issue.

Co-authored-by: krazijames <krazijames@gmail.com>
Co-authored-by: Nicolas Bigaouette <nbigaouette@gmail.com>
Co-authored-by: Keith Lohnes <lohnesk@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants