-
Notifications
You must be signed in to change notification settings - Fork 24
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 windows linking #104
Fix windows linking #104
Conversation
rahulchaphalkar
commented
May 14, 2024
- Fix finding of OpenVINO 2024.0.0 on Windows
- Fix linking Windows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a couple of thoughts here before we merge this:
openvino-finder
API: this crate is already very confusing and I'm concerned that exposingfind_link
is going to make it more so. It's not the PRs fault: it's mainly that loading these libraries from the many locations OpenVINO stores them in is quite tricky and the terms that different operating systems use to distinguish between static, dynamic, and runtime linking are all different. So I think we need to add the handling for*.lib
files but I think we should expand the definition offind()
with an extra parameter that distinguishes between static and dynamic libraries. @rahulchaphalkar, if you allow me to push to this branch I can do that expansion and document what is going on here.- supported versions: the more I mull it over, the more easily I think, "let's drop the older versions of OpenVINO" from the CI matrix. Those Windows libraries apparently don't have
ov_core_read_model_from_memory_buffer
and making things work for the deprecatedov_core_read_model_from_memory
on certain versions is too much complexity. Users can stick with a v0.6.* version of the crate if they want to use OpenVINO v2023.1.0 and below. @rahulchaphalkar, I propose we (a) eliminate those older versions from the CI matrix and (b) include one "special" entry (like the APT one) for the oldest version we think should still work on Ubuntu as a marker point for what's possible. We should also include v2024.1.0 here since that is now released.
In the future, I think we need to figure out the macOS TODO, but that might be tricky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've modified @BTOdell's original code a bit but the idea remains the same. I think this should be good to merge.